mac80211: Add radio led trigger

PATCH Superseded Ivo van Doorn 3 participants 10 messages Jan 6, 2008 – Jan 9, 2008 mac80211

Share & tools

Ivo van Doorn

author

2008-01-06 13:10 UTC #0 lore raw

PATCH 1/1 mac80211: Add radio led trigger

Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.

Signed-off-by Ivo van Doorn <[email protected]>

---

include/net/mac80211.h +29 -0

index 1d3d4f3..42499f6 100644
@@ -1173,6 +1173,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_tx_led_name(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_rx_led_name(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_assoc_led_name(struct ieee80211_hw *hw);
+extern char *__ieee80211_get_radio_led_name(struct ieee80211_hw *hw);
 #endif
 /**
  * ieee80211_get_tx_led_name - get name of TX LED

@@ -1212,6 +1213,16 @@ static inline char *ieee80211_get_rx_led_name(struct ieee80211_hw *hw)
 #endif
 }
 
+/**
+ * ieee80211_get_assoc_led_name - get name of association LED
+ *
+ * mac80211 creates a association LED trigger for each wireless hardware
+ * that can be used to drive LEDs if your driver registers a LED device.
+ * This function returns the name (or %NULL if not configured for LEDs)
+ * of the trigger so you can automatically link the LED device.
+ *
+ * @hw: the hardware to get the LED trigger name for
+ */
 static inline char *ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
 {
 #ifdef CONFIG_MAC80211_LEDS

@@ -1221,6 +1232,24 @@ static inline char *ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
 #endif
 }
 
+/**
+ * ieee80211_get_radio_led_name - get name of radio LED
+ *
+ * mac80211 creates a radio change LED trigger for each wireless hardware
+ * that can be used to drive LEDs if your driver registers a LED device.
+ * This function returns the name (or %NULL if not configured for LEDs)
+ * of the trigger so you can automatically link the LED device.
+ *
+ * @hw: the hardware to get the LED trigger name for
+ */
+static inline char *ieee80211_get_radio_led_name(struct ieee80211_hw *hw)
+{
+#ifdef CONFIG_MAC80211_LEDS
+	return __ieee80211_get_radio_led_name(hw);
+#else
+	return NULL;
+#endif
+}
 
 /* Register a new hardware PHYMODE capability to the stack. */
 int ieee80211_register_hwmode(struct ieee80211_hw *hw,

net/mac80211/ieee80211.c +3 -0

index 5aa9c2d..df64b6e 100644
@@ -219,6 +219,7 @@ static int ieee80211_open(struct net_device *dev)
 		if (res)
 			return res;
 		ieee80211_hw_config(local);
+		ieee80211_led_radio(local, 1);
 	}
 
 	switch (sdata->vif.type) {

@@ -390,6 +391,8 @@ static int ieee80211_stop(struct net_device *dev)
 		if (local->ops->stop)
 			local->ops->stop(local_to_hw(local));
 
+		ieee80211_led_radio(local, 0);
+
 		tasklet_disable(&local->tx_pending_tasklet);
 		tasklet_disable(&local->tasklet);
 	}

net/mac80211/ieee80211_i.h +3 -2

index 083c432..e4d2f66 100644
@@ -513,8 +513,9 @@ struct ieee80211_local {
 
 #ifdef CONFIG_MAC80211_LEDS
 	int tx_led_counter, rx_led_counter;
-	struct led_trigger *tx_led, *rx_led, *assoc_led;
-	char tx_led_name[32], rx_led_name[32], assoc_led_name[32];
+	struct led_trigger *tx_led, *rx_led, *assoc_led, *radio_led;
+	char tx_led_name[32], rx_led_name[32],
+	     assoc_led_name[32], radio_led_name[32];
 #endif
 
 	u32 channel_use;

net/mac80211/ieee80211_ioctl.c +2 -0

index be80f44..b28eb8f 100644
@@ -21,6 +21,7 @@
 
 #include <net/mac80211.h>
 #include "ieee80211_i.h"
+#include "ieee80211_led.h"
 #include "ieee80211_rate.h"
 #include "wpa.h"
 #include "aes_ccm.h"

@@ -651,6 +652,7 @@ static int ieee80211_ioctl_siwtxpower(struct net_device *dev,
 	if (local->hw.conf.radio_enabled != !(data->txpower.disabled)) {
 		local->hw.conf.radio_enabled = !(data->txpower.disabled);
 		need_reconfig = 1;
+		ieee80211_led_radio(local, local->hw.conf.radio_enabled);
 	}
 
 	if (need_reconfig) {

net/mac80211/ieee80211_led.c +35 -0

index 4cf89af..f401484 100644
@@ -43,6 +43,16 @@ void ieee80211_led_assoc(struct ieee80211_local *local, bool associated)
 		led_trigger_event(local->assoc_led, LED_OFF);
 }
 
+void ieee80211_led_radio(struct ieee80211_local *local, bool enabled)
+{
+	if (unlikely(!local->radio_led))
+		return;
+	if (enabled)
+		led_trigger_event(local->radio_led, LED_FULL);
+	else
+		led_trigger_event(local->radio_led, LED_OFF);
+}
+
 void ieee80211_led_init(struct ieee80211_local *local)
 {
 	local->rx_led = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);

@@ -77,10 +87,25 @@ void ieee80211_led_init(struct ieee80211_local *local)
 			local->assoc_led = NULL;
 		}
 	}
+
+	local->radio_led = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
+	if (local->radio_led) {
+		snprintf(local->radio_led_name, sizeof(local->radio_led_name),
+			 "%sradio", wiphy_name(local->hw.wiphy));
+		local->radio_led->name = local->radio_led_name;
+		if (led_trigger_register(local->radio_led)) {
+			kfree(local->radio_led);
+			local->radio_led = NULL;
+		}
+	}
 }
 
 void ieee80211_led_exit(struct ieee80211_local *local)
 {
+	if (local->radio_led) {
+		led_trigger_unregister(local->radio_led);
+		kfree(local->radio_led);
+	}
 	if (local->assoc_led) {
 		led_trigger_unregister(local->assoc_led);
 		kfree(local->assoc_led);

@@ -95,6 +120,16 @@ void ieee80211_led_exit(struct ieee80211_local *local)
 	}
 }
 
+char *__ieee80211_get_radio_led_name(struct ieee80211_hw *hw)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	if (local->radio_led)
+		return local->radio_led_name;
+	return NULL;
+}
+EXPORT_SYMBOL(__ieee80211_get_radio_led_name);
+
 char *__ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);

net/mac80211/ieee80211_led.h +6 -0

index 0feb226..77b1e1b 100644
@@ -16,6 +16,8 @@ extern void ieee80211_led_rx(struct ieee80211_local *local);
 extern void ieee80211_led_tx(struct ieee80211_local *local, int q);
 extern void ieee80211_led_assoc(struct ieee80211_local *local,
 				bool associated);
+extern void ieee80211_led_radio(struct ieee80211_local *local,
+				bool enabled);
 extern void ieee80211_led_init(struct ieee80211_local *local);
 extern void ieee80211_led_exit(struct ieee80211_local *local);
 #else

@@ -29,6 +31,10 @@ static inline void ieee80211_led_assoc(struct ieee80211_local *local,
 				       bool associated)
 {
 }
+static inline void ieee80211_led_radio(struct ieee80211_local *local,
+				       bool enabled)
+{
+}
 static inline void ieee80211_led_init(struct ieee80211_local *local)
 {
 }

Tomas Winkler

2008-01-06 18:19 UTC #1 lore raw

Quoted 5 lines · Ivo van Doorn

Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.

I wonder how this go together with RF KILL led?

Tomas

Michael Buesch

2008-01-06 18:23 UTC #2 lore raw

Quoted 7 lines · On Sunday 06 January 2008 19:19:54 Tomas Winkler wrote:

Quoted 5 lines · Ivo van Doorn
Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.
I wonder how this go together with RF KILL led?

The rfkill LED is a seperate LED.
This LED is the software status of the radio. Not too useful, but hey. :)

Signature · 1 line

Greetings Michael.

Ivo van Doorn

author

2008-01-06 18:26 UTC #3 lore raw

In reply to Tomas Winkler

Quoted 7 lines · Tomas Winkler

Quoted 5 lines · Ivo van Doorn
Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.
I wonder how this go together with RF KILL led?

They would be competing I fear, but not all devices have a rfkill button, which makes the mac80211 radio led trigger more usefull I think.

Ivo

Ivo van Doorn

author

2008-01-07 18:44 UTC #5 lore raw

Quoted 19 lines · Tomas Winkler

Quoted 8 lines · Ivo van Doorn
Quoted 6 lines · Tomas Winkler
Quoted 5 lines · Ivo van Doorn
Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.
Quoted 5 lines
Quoted 1 line
I wonder how this go together with RF KILL led?
They would be competing I fear, but not all devices have a rfkill button, which makes the mac80211 radio led trigger more usefull I think.
What inquires me is in the following snippet is would you lit the led if even if rfkill HW switch is on?

net/mac80211/ieee80211.c static int ieee80211_open(struct net_device *dev) · line 224

Quoted 6 lines

net/mac80211/ieee80211.c +1 -0

@@ -219,6 +219,7 @@ static int ieee80211_open(struct net_device *dev)
               if (res)
                       return res;
               ieee80211_hw_config(local);
+               ieee80211_led_radio(local, 1);
       }

Good point. I have updated the patch to make it check the radio_enabled configuration parameter.

Ivo

Ivo van Doorn

author

2008-01-07 18:45 UTC #6 lore raw

PATCH v2 1/1 mac80211: Add radio led trigger

In reply to Ivo van Doorn

Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.

v2: Check hw.conf.radio_enabled when calling start().

Signed-off-by Ivo van Doorn <[email protected]>

---

include/net/mac80211.h +29 -0

index 1d3d4f3..42499f6 100644
@@ -1173,6 +1173,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_tx_led_name(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_rx_led_name(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_assoc_led_name(struct ieee80211_hw *hw);
+extern char *__ieee80211_get_radio_led_name(struct ieee80211_hw *hw);
 #endif
 /**
  * ieee80211_get_tx_led_name - get name of TX LED

@@ -1212,6 +1213,16 @@ static inline char *ieee80211_get_rx_led_name(struct ieee80211_hw *hw)
 #endif
 }
 
+/**
+ * ieee80211_get_assoc_led_name - get name of association LED
+ *
+ * mac80211 creates a association LED trigger for each wireless hardware
+ * that can be used to drive LEDs if your driver registers a LED device.
+ * This function returns the name (or %NULL if not configured for LEDs)
+ * of the trigger so you can automatically link the LED device.
+ *
+ * @hw: the hardware to get the LED trigger name for
+ */
 static inline char *ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
 {
 #ifdef CONFIG_MAC80211_LEDS

@@ -1221,6 +1232,24 @@ static inline char *ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
 #endif
 }
 
+/**
+ * ieee80211_get_radio_led_name - get name of radio LED
+ *
+ * mac80211 creates a radio change LED trigger for each wireless hardware
+ * that can be used to drive LEDs if your driver registers a LED device.
+ * This function returns the name (or %NULL if not configured for LEDs)
+ * of the trigger so you can automatically link the LED device.
+ *
+ * @hw: the hardware to get the LED trigger name for
+ */
+static inline char *ieee80211_get_radio_led_name(struct ieee80211_hw *hw)
+{
+#ifdef CONFIG_MAC80211_LEDS
+	return __ieee80211_get_radio_led_name(hw);
+#else
+	return NULL;
+#endif
+}
 
 /* Register a new hardware PHYMODE capability to the stack. */
 int ieee80211_register_hwmode(struct ieee80211_hw *hw,

net/mac80211/ieee80211.c +3 -0

index 5aa9c2d..df64b6e 100644
@@ -219,6 +219,7 @@ static int ieee80211_open(struct net_device *dev)
 		if (res)
 			return res;
 		ieee80211_hw_config(local);
+		ieee80211_led_radio(local, local->hw.conf.radio_enabled);
 	}
 
 	switch (sdata->vif.type) {

@@ -390,6 +391,8 @@ static int ieee80211_stop(struct net_device *dev)
 		if (local->ops->stop)
 			local->ops->stop(local_to_hw(local));
 
+		ieee80211_led_radio(local, 0);
+
 		tasklet_disable(&local->tx_pending_tasklet);
 		tasklet_disable(&local->tasklet);
 	}

net/mac80211/ieee80211_i.h +3 -2

index 083c432..e4d2f66 100644
@@ -513,8 +513,9 @@ struct ieee80211_local {
 
 #ifdef CONFIG_MAC80211_LEDS
 	int tx_led_counter, rx_led_counter;
-	struct led_trigger *tx_led, *rx_led, *assoc_led;
-	char tx_led_name[32], rx_led_name[32], assoc_led_name[32];
+	struct led_trigger *tx_led, *rx_led, *assoc_led, *radio_led;
+	char tx_led_name[32], rx_led_name[32],
+	     assoc_led_name[32], radio_led_name[32];
 #endif
 
 	u32 channel_use;

net/mac80211/ieee80211_ioctl.c +2 -0

index be80f44..b28eb8f 100644
@@ -21,6 +21,7 @@
 
 #include <net/mac80211.h>
 #include "ieee80211_i.h"
+#include "ieee80211_led.h"
 #include "ieee80211_rate.h"
 #include "wpa.h"
 #include "aes_ccm.h"

@@ -651,6 +652,7 @@ static int ieee80211_ioctl_siwtxpower(struct net_device *dev,
 	if (local->hw.conf.radio_enabled != !(data->txpower.disabled)) {
 		local->hw.conf.radio_enabled = !(data->txpower.disabled);
 		need_reconfig = 1;
+		ieee80211_led_radio(local, local->hw.conf.radio_enabled);
 	}
 
 	if (need_reconfig) {

net/mac80211/ieee80211_led.c +35 -0

index 4cf89af..f401484 100644
@@ -43,6 +43,16 @@ void ieee80211_led_assoc(struct ieee80211_local *local, bool associated)
 		led_trigger_event(local->assoc_led, LED_OFF);
 }
 
+void ieee80211_led_radio(struct ieee80211_local *local, bool enabled)
+{
+	if (unlikely(!local->radio_led))
+		return;
+	if (enabled)
+		led_trigger_event(local->radio_led, LED_FULL);
+	else
+		led_trigger_event(local->radio_led, LED_OFF);
+}
+
 void ieee80211_led_init(struct ieee80211_local *local)
 {
 	local->rx_led = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);

@@ -77,10 +87,25 @@ void ieee80211_led_init(struct ieee80211_local *local)
 			local->assoc_led = NULL;
 		}
 	}
+
+	local->radio_led = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
+	if (local->radio_led) {
+		snprintf(local->radio_led_name, sizeof(local->radio_led_name),
+			 "%sradio", wiphy_name(local->hw.wiphy));
+		local->radio_led->name = local->radio_led_name;
+		if (led_trigger_register(local->radio_led)) {
+			kfree(local->radio_led);
+			local->radio_led = NULL;
+		}
+	}
 }
 
 void ieee80211_led_exit(struct ieee80211_local *local)
 {
+	if (local->radio_led) {
+		led_trigger_unregister(local->radio_led);
+		kfree(local->radio_led);
+	}
 	if (local->assoc_led) {
 		led_trigger_unregister(local->assoc_led);
 		kfree(local->assoc_led);

@@ -95,6 +120,16 @@ void ieee80211_led_exit(struct ieee80211_local *local)
 	}
 }
 
+char *__ieee80211_get_radio_led_name(struct ieee80211_hw *hw)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	if (local->radio_led)
+		return local->radio_led_name;
+	return NULL;
+}
+EXPORT_SYMBOL(__ieee80211_get_radio_led_name);
+
 char *__ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
 {
 	struct ieee80211_local *local = hw_to_local(hw);

net/mac80211/ieee80211_led.h +6 -0

index 0feb226..77b1e1b 100644
@@ -16,6 +16,8 @@ extern void ieee80211_led_rx(struct ieee80211_local *local);
 extern void ieee80211_led_tx(struct ieee80211_local *local, int q);
 extern void ieee80211_led_assoc(struct ieee80211_local *local,
 				bool associated);
+extern void ieee80211_led_radio(struct ieee80211_local *local,
+				bool enabled);
 extern void ieee80211_led_init(struct ieee80211_local *local);
 extern void ieee80211_led_exit(struct ieee80211_local *local);
 #else

@@ -29,6 +31,10 @@ static inline void ieee80211_led_assoc(struct ieee80211_local *local,
 				       bool associated)
 {
 }
+static inline void ieee80211_led_radio(struct ieee80211_local *local,
+				       bool enabled)
+{
+}
 static inline void ieee80211_led_init(struct ieee80211_local *local)
 {
 }

Tomas Winkler

2008-01-09 00:18 UTC #7 lore raw

Quoted 27 lines · Ivo van Doorn

Some devices have a seperate LED which indicates if the radio is enabled or not. This adds a LED trigger to mac80211 where drivers can hook into when they are interested in radio status changes.
v2: Check hw.conf.radio_enabled when calling start().
Signed-off-by Ivo van Doorn <[email protected]>
---
include/net/mac80211.h +1 -0
index 1d3d4f3..42499f6 100644
@@ -1173,6 +1173,7 @@ int ieee80211_register_hw(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_tx_led_name(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_rx_led_name(struct ieee80211_hw *hw);
 extern char *__ieee80211_get_assoc_led_name(struct ieee80211_hw *hw);
+extern char *__ieee80211_get_radio_led_name(struct ieee80211_hw *hw);
 #endif
 /**
  * ieee80211_get_tx_led_name - get name of TX LED
@@ -1212,6 +1213,16 @@ static inline char *ieee80211_get_rx_led_name(struct ieee80211_hw *hw)
 #endif
 }
 

include/net/mac80211.h static int ieee80211_ioctl_siwtxpower(struct net_device *dev, · line 655

Quoted 4 lines

include/net/mac80211.h +1 -0

@@ -651,6 +652,7 @@ static int ieee80211_ioctl_siwtxpower(struct net_device *dev,
        if (local->hw.conf.radio_enabled != !(data->txpower.disabled)) {
                local->hw.conf.radio_enabled = !(data->txpower.disabled);
                need_reconfig = 1;
+               ieee80211_led_radio(local, local->hw.conf.radio_enabled);

Not sure but isn't it cleaner to push it to ieee80211_hw_config(). This will cover also ieee80211_open and maybe in the future also cfg80211
Will it be so bad to call led_radio even the led is already lit?

Thanks
Tomas

Ivo van Doorn

author

2008-01-09 16:51 UTC #8 lore raw

Hi,

Quoted 10 lines

net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_siwtxpower(struct net_device *dev, · line 655
Quoted 5 lines
net/mac80211/ieee80211_ioctl.c +1 -0
@@ -651,6 +652,7 @@ static int ieee80211_ioctl_siwtxpower(struct net_device *dev,
        if (local->hw.conf.radio_enabled != !(data->txpower.disabled)) {
                local->hw.conf.radio_enabled = !(data->txpower.disabled);
                need_reconfig = 1;
+               ieee80211_led_radio(local, local->hw.conf.radio_enabled);
Not sure but isn't it cleaner to push it to ieee80211_hw_config(). This will cover also ieee80211_open and maybe in the future also cfg80211
Will it be so bad to call led_radio even the led is already lit?

Well that means it will be called quite often, and although it would perhaps not matter that much, I don't consider it a clean solution.

We could move it to ieee80211_hw_config() eventually, but I think we should first implement some kind of "what config option changed" handling. That would benefit drivers as well since they should now handle that to prevent configuring _everything_ when hw_config() is called.

Ivo

Tomas Winkler

2008-01-09 17:07 UTC #9 lore raw

Quoted 13 lines · Ivo van Doorn

Hi,
Quoted 10 lines
net/mac80211/ieee80211_ioctl.c static int ieee80211_ioctl_siwtxpower(struct net_device *dev, · line 655
Quoted 5 lines
net/mac80211/ieee80211_ioctl.c +1 -0
@@ -651,6 +652,7 @@ static int ieee80211_ioctl_siwtxpower(struct net_device *dev,
        if (local->hw.conf.radio_enabled != !(data->txpower.disabled)) {
                local->hw.conf.radio_enabled = !(data->txpower.disabled);
                need_reconfig = 1;
+               ieee80211_led_radio(local, local->hw.conf.radio_enabled);
Not sure but isn't it cleaner to push it to ieee80211_hw_config(). This will cover also ieee80211_open and maybe in the future also cfg80211
Will it be so bad to call led_radio even the led is already lit?

Quoted 2 lines

Well that means it will be called quite often, and although it would perhaps not matter that much, I don't consider it a clean solution.

If you have one physical led mapped to few triggers as it is on current laptops it will be probably complicate few things so after all I'm fine with how it is now.

Quoted 4 lines

We could move it to ieee80211_hw_config() eventually, but I think we should first implement some kind of "what config option changed" handling. That would benefit drivers as well since they should now handle that to prevent configuring _everything_ when hw_config() is called.

You are talking from my heart.

Tomas

Quoted 2 lines

Ivo