[RFC] d80211 LED handling

[RFC] d80211 LED handling

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

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

[-- Attachment #2: d80211-led.patch --]
[-- Type: text/plain, Size: 7732 bytes --]

--- wireless-dev.orig/include/net/d80211.h	2006-08-10 20:02:20.159652863 +0200
+++ wireless-dev/include/net/d80211.h	2006-08-10 20:02:22.439652863 +0200
@@ -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
--- wireless-dev.orig/net/d80211/Kconfig	2006-08-10 20:02:20.199652863 +0200
+++ wireless-dev/net/d80211/Kconfig	2006-08-10 20:02:22.439652863 +0200
@@ -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
--- wireless-dev.orig/net/d80211/ieee80211.c	2006-08-10 20:02:20.279652863 +0200
+++ wireless-dev/net/d80211/ieee80211.c	2006-08-10 20:02:22.449652863 +0200
@@ -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
--- wireless-dev.orig/net/d80211/ieee80211_dev.c	2006-08-10 20:02:20.309652863 +0200
+++ wireless-dev/net/d80211/ieee80211_dev.c	2006-08-10 21:54:44.300216256 +0200
@@ -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;
--- wireless-dev.orig/net/d80211/ieee80211_i.h	2006-08-10 20:02:20.399652863 +0200
+++ wireless-dev/net/d80211/ieee80211_i.h	2006-08-10 20:03:22.939652863 +0200
@@ -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;
--- wireless-dev.orig/net/d80211/ieee80211_led.c	2006-08-10 20:02:20.429652863 +0200
+++ wireless-dev/net/d80211/ieee80211_led.c	2006-08-10 20:25:19.659652863 +0200
@@ -1,32 +1,70 @@
 /*
- * Copyright 2002-2004, Instant802 Networks, Inc.
+ * Copyright 2006, Johannes Berg <johannes@sipsolutions.net>
  *
  * 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/config.h>
-#include <linux/netdevice.h>
-#include <linux/types.h>
-
-#ifdef CONFIG_OAP_LEDS_WLAN
-extern void leds_wlan_set(int unit, int tx, int state);
-#endif
-
-void ieee80211_rx_led(int state, struct net_device *dev) {
-#ifdef CONFIG_OAP_LEDS_WLAN
-	static unsigned int count = 0;
+/* just for IFNAMSIZ */
+#include <linux/if.h>
+#include "ieee80211_led.h"
+
+void ieee80211_led_rx(struct ieee80211_local *local)
+{
+	if (unlikely(!local->rx_led))
+		return;
+	if (local->rx_led_counter++ % 2 == 0)
+		led_trigger_event(local->rx_led, LED_OFF);
+	else
+		led_trigger_event(local->rx_led, LED_FULL);
+}
 
-	if (state == 2) {
-		leds_wlan_set(0, 0, (++count) & 1);
-	}
-#endif
+/* q is 1 if a packet was enqueued, 0 if it has been transmitted */
+void ieee80211_led_tx(struct ieee80211_local *local, int q)
+{
+	if (unlikely(!local->tx_led))
+		return;
+	/* not sure how this is supposed to work ... */
+	local->tx_led_counter += 2*q-1;
+	if (local->tx_led_counter % 2 == 0)
+		led_trigger_event(local->tx_led, LED_OFF);
+	else
+		led_trigger_event(local->tx_led, LED_FULL);
 }
 
-void ieee80211_tx_led(int state, struct net_device *dev) {
-#ifdef CONFIG_OAP_LEDS_WLAN
-        leds_wlan_set(0, 1, state);
-#endif
+void ieee80211_led_init(struct ieee80211_local *local)
+{
+	char *name;
+	local->rx_led = kzalloc(sizeof(struct led_trigger) + IFNAMSIZ + 2, GFP_KERNEL);
+	if (!local->rx_led)
+		return;
+	name = (char*) local->rx_led+1;
+	snprintf(name, IFNAMSIZ + 2, "%srx", local->mdev->name);
+	if (led_trigger_register(local->rx_led)) {
+		kfree(local->rx_led);
+		local->rx_led = NULL;
+	}
+
+	local->tx_led = kzalloc(sizeof(struct led_trigger) + IFNAMSIZ + 2, GFP_KERNEL);
+	if (!local->tx_led)
+		return;
+	name = (char*) local->tx_led+1;
+	snprintf(name, IFNAMSIZ + 2, "%stx", local->mdev->name);
+	if (led_trigger_register(local->tx_led)) {
+		kfree(local->tx_led);
+		local->tx_led = NULL;
+	}
 }
 
+void ieee80211_led_exit(struct ieee80211_local *local)
+{
+	if (local->tx_led) {
+		led_trigger_unregister(local->tx_led);
+		kfree(local->tx_led);
+	}
+	if (local->rx_led) {
+		led_trigger_unregister(local->rx_led);
+		kfree(local->rx_led);
+	}
+}
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ wireless-dev/net/d80211/ieee80211_led.h	2006-08-10 20:24:54.849652863 +0200
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2006, Johannes Berg <johannes@sipsolutions.net>
+ *
+ * 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

Re: [RFC] d80211 LED handling

[Jiri Benc]

On Fri, 11 Aug 2006 09:55:53 +0200, Johannes Berg wrote:
> 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.

> [...]
> +config D80211_LEDS
> +	bool "Enable LED triggers"
> +	select LEDS_TRIGGERS

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

> [...]
> +	name = (char*) local->rx_led+1;

This doesn't seem correct.

> +	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"

> +	if (led_trigger_register(local->rx_led)) {
> [...]

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [RFC] d80211 LED handling

[Johannes Berg]

On Thu, 2006-08-17 at 17:30 +0200, Jiri Benc wrote:

> 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.

> 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.

>  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...

> > [...]
> > +config D80211_LEDS
> > +	bool "Enable LED triggers"
> > +	select LEDS_TRIGGERS
> 
> Is it really necessary to have this as a configurable option?

Probably not.

> > [...]
> > +	name = (char*) local->rx_led+1;
> 
> This doesn't seem correct.

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

> > +	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