Skip to contentSkip to conversation

d80211 LED handling

RFC In review Johannes Berg 2 participants 3 messages Aug 11, 2006 – Aug 18, 2006

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Johannes Berg

author
#0 lore raw

RFC 1/1 d80211 LED handling

Attached (sorry, can't seem to figure out how to convince thunderbird to not mangle things under windows) is a patch to make d80211 have some LED triggers.

However, I'm not sure where I should call the _init and _exit functions and if I really should be using the local struct. It seems I shouldn't be and should be using the master interface directly or something.

Anyway, food for though and comments.

johannes

include/net/d80211.h +0 -10
@@ -884,16 +884,6 @@ enum {
 	IEEE80211_TEST_PARAM_TX_ANT_SEL_RAW = 5,
 };
 
-/* ieee80211_tx_led called with state == 1 when the first frame is queued
- *   with state == 0 when the last frame is transmitted and tx queue is empty
- */
-void ieee80211_tx_led(int state, struct net_device *dev);
-/* ieee80211_rx_led is called each time frame is received, state is not used
- * (== 2)
- */
-void ieee80211_rx_led(int state, struct net_device *dev);
-
-
 /* IEEE 802.11 defines */
 
 #define FCS_LEN 4
net/d80211/Kconfig +7 -0
@@ -7,6 +7,13 @@ config D80211
 	This option enables the hardware independent IEEE 802.11
 	networking stack.
 
+config D80211_LEDS
+	bool "Enable LED triggers"
+	select LEDS_TRIGGERS
+	---help---
+	This option enables a few LED triggers for different
+	packet receive/transmit events.
+
 config D80211_DEBUG
 	bool "Enable debugging output"
 	depends on D80211
net/d80211/ieee80211.c +6 -19
@@ -31,7 +31,7 @@
 #include "tkip.h"
 #include "wme.h"
 #include "aes_ccm.h"
-
+#include "ieee80211_led.h"
 
 /* See IEEE 802.1H for LLC/SNAP encapsulation/decapsulation */
 /* Ethernet-II snap header (RFC1042 for most EtherTypes) */

@@ -1181,11 +1181,7 @@ static int __ieee80211_tx(struct ieee802
 		ret = local->hw->tx(local->mdev, skb, control);
 		if (ret)
 			return IEEE80211_TX_AGAIN;
-#ifdef IEEE80211_LEDS
-		if (local->tx_led_counter++ == 0) {
-			ieee80211_tx_led(1, local->mdev);
-		}
-#endif /* IEEE80211_LEDS */
+		ieee80211_led_tx(local, 1);
 	}
 	if (tx->u.tx.extra_frag) {
 		control->use_rts_cts = 0;

@@ -1210,11 +1206,7 @@ static int __ieee80211_tx(struct ieee802
 					    control);
 			if (ret)
 				return IEEE80211_TX_FRAG_AGAIN;
-#ifdef IEEE80211_LEDS
-			if (local->tx_led_counter++ == 0) {
-				ieee80211_tx_led(1, local->mdev);
-			}
-#endif /* IEEE80211_LEDS */
+			ieee80211_led_tx(local, 1);
 			tx->u.tx.extra_frag[i] = NULL;
 		}
 		kfree(tx->u.tx.extra_frag);

@@ -2998,10 +2990,8 @@ ieee80211_rx_h_defragment(struct ieee802
 		rx->sta->rx_packets++;
 	if (is_multicast_ether_addr(hdr->addr1))
 		rx->local->dot11MulticastReceivedFrameCount++;
-#ifdef IEEE80211_LEDS
         else
-		ieee80211_rx_led(2, rx->dev);
-#endif /* IEEE80211_LEDS */
+		ieee80211_led_rx(rx->local);
 	return TXRX_CONTINUE;
 }
 

@@ -4104,11 +4094,8 @@ void ieee80211_tx_status(struct net_devi
 		rate_control_tx_status(dev, skb, status);
 	}
 
-#ifdef IEEE80211_LEDS
-        if (local->tx_led_counter && (local->tx_led_counter-- == 1)) {
-                ieee80211_tx_led(0, dev);
-        }
-#endif /* IEEE80211_LEDS */
+	ieee80211_led_tx(local, 0);
+
         /* SNMP counters
 	 * Fragments are passed to low-level drivers as separate skbs, so these
 	 * are actually fragments, not frames. Update frame counters only for
net/d80211/ieee80211_dev.c +1 -0
@@ -13,6 +13,7 @@
 #include <linux/netdevice.h>
 #include <net/d80211.h>
 #include "ieee80211_i.h"
+#include "ieee80211_led.h"
 
 struct ieee80211_dev_list {
 	struct list_head list;
net/d80211/ieee80211_i.h +4 -1
@@ -460,7 +460,10 @@ struct ieee80211_local {
         u32 dot11TransmittedFrameCount;
         u32 dot11WEPUndecryptableCount;
 
-        int tx_led_counter;
+#ifdef CONFIG_D80211_LEDS
+	int tx_led_counter, rx_led_counter;
+	struct led_trigger *tx_led, *rx_led;
+#endif
 
 	u32 channel_use;
 	u32 channel_use_raw;
net/d80211/ieee80211_led.c +58 -20

90 lines not on this page · raw message

net/d80211/ieee80211_led.h new file +30 -0
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2006, Johannes Berg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/leds.h>
+#include "ieee80211_i.h"
+
+#ifdef CONFIG_D80211_LEDS
+extern void ieee80211_led_rx(struct ieee80211_local *local);
+extern void ieee80211_led_tx(struct ieee80211_local *local, int q);
+extern void ieee80211_led_init(struct ieee80211_local *local);
+extern void ieee80211_led_exit(struct ieee80211_local *local);
+#else
+static inline void ieee80211_led_rx(struct ieee80211_local *local)
+{
+}
+static inline void ieee80211_led_tx(struct ieee80211_local *local, int q)
+{
+}
+static inline void ieee80211_led_init(struct ieee80211_local *local)
+{
+}
+static inline void ieee80211_led_exit(struct ieee80211_local *local)
+{
+}
+#endif
#1 lore raw
Quoted 3 lines · Johannes Berg

However, I'm not sure where I should call the _init and _exit functions and if I really should be using the local struct. It seems I shouldn't be and should be using the master interface directly or something.

Leds are not interface-specific but card-specific, so using ieee80211_local is the right way.

Comments:

How is the driver supposed to add its led handlers?

Using generic led triggers is probably interesting for cases when system has some separate led. It won't help drivers which need to handle leds on the card itself. In the former case you may want all of wireless cards in the system to be connected to one led. I don't see how this is easily possible with this patch. In the latter case, you want to call callback provided by the driver and not to use generic led triggers at all.

net/d80211/Kconfig config D80211 · line 12

Quoted 4 lines

[...]

net/d80211/Kconfig +3 -0
+config D80211_LEDS
+	bool "Enable LED triggers"
+	select LEDS_TRIGGERS

Is it really necessary to have this as a configurable option?

net/d80211/ieee80211_led.c line 42

Quoted 2 lines

[...]

net/d80211/ieee80211_led.c +1 -0
+	name = (char*) local->rx_led+1;

This doesn't seem correct.

net/d80211/ieee80211_led.c line 43

Quoted 1 line
net/d80211/ieee80211_led.c +1 -0
+	snprintf(name, IFNAMSIZ + 2, "%srx", local->mdev->name);

Name of interface may change at any time. Using it for fixed identifier is not a good idea. In addition, nothing prevents you from ending up with two different led triggers with the same name:

1. modprobe card1
2. ieee80211_led_init(card1) <- card1 is "wmaster0"
3. ip link set wmaster0 name mysupercard
4. modprobe card2
5. ieee80211_led_init(card2) <- card2 is "wmaster0"

net/d80211/ieee80211_led.c line 44

Quoted 2 lines
net/d80211/ieee80211_led.c +1 -0
+	if (led_trigger_register(local->rx_led)) {
[...]

Thanks,

Jiri

Signature · 2 lines
Jiri Benc
SUSE Labs
author
#2 lore raw
Quoted 1 line · Jiri Benc

How is the driver supposed to add its led handlers?

The driver just adds an LED device. And then we only need to set the default trigger correctly.

Quoted 5 lines

Using generic led triggers is probably interesting for cases when system has some separate led. It won't help drivers which need to handle leds on the card itself. In the former case you may want all of wireless cards in the system to be connected to one led. I don't see how this is easily possible with this patch.

No, that's not possible at all. But we could have a generic 'd80211-all-cards' trigger I suppose.

Quoted 3 lines

In the latter case, you want to call
callback provided by the driver and not to use generic led triggers at all.

Why not use the generic trigger anyway? With the right default...

Quoted 6 lines

net/d80211/Kconfig config D80211 · line 12

Quoted 4 lines

[...]

net/d80211/Kconfig +3 -0
+config D80211_LEDS
+	bool "Enable LED triggers"
+	select LEDS_TRIGGERS

Is it really necessary to have this as a configurable option?

Probably not.

Quoted 4 lines

net/d80211/ieee80211_led.c line 42

Quoted 2 lines

[...]

net/d80211/ieee80211_led.c +1 -0
+	name = (char*) local->rx_led+1;

This doesn't seem correct.

Hm, yeah, it probably needs to be (char*) (local->rx_led+1).

Quoted 11 lines

net/d80211/ieee80211_led.c line 43

Quoted 1 line
net/d80211/ieee80211_led.c +1 -0
+	snprintf(name, IFNAMSIZ + 2, "%srx", local->mdev->name);

Name of interface may change at any time. Using it for fixed identifier is not a good idea. In addition, nothing prevents you from ending up with two different led triggers with the same name:

1. modprobe card1
2. ieee80211_led_init(card1) <- card1 is "wmaster0"
3. ip link set wmaster0 name mysupercard
4. modprobe card2
5. ieee80211_led_init(card2) <- card2 is "wmaster0"

Good point. I guess we'll want to have the wiphy in there instead.

johannes