[wireless] bunch of questions and notes on d80211

[wireless] bunch of questions and notes on d80211

[Johannes Berg]

This mail is supposed to just serve as a thread parent for a bunch of 
questions, small patches and observations I made playing with d80211 and 
looking through the source code over the weekend. Just so you can ignore 
the whole thread if you don't care :)

johannes

bcm43xx for d80211 softirq loop

[Johannes Berg]

[23750.726463] NETDEV WATCHDOG: wmaster0: transmit timed out
[23750.726482] wmaster0: resetting interface.
[23750.726490] bcm43xx_d80211: Controller RESET (IEEE reset) ...
[23750.753458] bcm43xx_d80211: select_wireless_core: cleanup
[23750.753477] bcm43xx_d80211: Radio turned off
[23750.753538] bcm43xx_d80211: DMA 0x0200 (RX) max used slots: 1/64
[23750.755208] bcm43xx_d80211: DMA 0x0260 (TX) max used slots: 0/512
[23750.755601] bcm43xx_d80211: DMA 0x0240 (TX) max used slots: 0/512
[23750.755992] bcm43xx_d80211: DMA 0x0220 (TX) max used slots: 0/512
[23750.756383] bcm43xx_d80211: DMA 0x0200 (TX) max used slots: 0/512
[23750.957250] bcm43xx_d80211: Radio turned on
[23751.129344] bcm43xx_d80211: Chip initialized
[23751.137239] bcm43xx_d80211: DMA initialized
[23751.137503] bcm43xx_d80211: Keys cleared
[23751.153242] bcm43xx_d80211: Selected 802.11 core (phytype 2)
[23751.153263] bcm43xx_d80211: Controller restarted


after this, ksoftirq started running wild...

Since oprofile doesn't show any function ever called for a tasklet, I 
started to investigate but couldn't find what was up. Then I removed the 
bcm43xx_dscape module and the kernel blew up right away.

Then I patched the kernel with a runaway tasklet patch (see mail on 
lkml) and got this (yeah, previous iteration of the patch where I forgot 
a "\n"):

[15214.574151] NETDEV WATCHDOG: wmaster0: transmit timed out
[15214.574166] wmaster0: resetting interface.
[15214.574174] bcm43xx_d80211: Controller RESET (IEEE reset) ...
[15214.591194] bcm43xx_d80211: select_wireless_core: cleanup
[15214.591209] bcm43xx_d80211: Radio turned off
[15214.591270] bcm43xx_d80211: DMA 0x0200 (RX) max used slots: 1/64
[15214.592901] bcm43xx_d80211: DMA 0x0260 (TX) max used slots: 0/512
[15214.593294] bcm43xx_d80211: DMA 0x0240 (TX) max used slots: 0/512
[15214.593685] bcm43xx_d80211: DMA 0x0220 (TX) max used slots: 0/512
[15214.594075] bcm43xx_d80211: DMA 0x0200 (TX) max used slots: 0/512
[15214.773961] bcm43xx_d80211: Radio turned on
[15214.945901] bcm43xx_d80211: Chip initialized
[15214.953815] bcm43xx_d80211: DMA initialized
[15214.954078] bcm43xx_d80211: Keys cleared
[15214.969864] bcm43xx_d80211: Selected 802.11 core (phytype 2)
[15214.969889] bcm43xx_d80211: Controller restarted
[15215.019746] tasklet 
drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c:4376 is scheduled but 
hasn't been enabled for too long!
<6>NETDEV WATCHDOG: wmaster0: transmit timed out
[15769.964164] wmaster0: resetting interface.
[15769.964175] bcm43xx_d80211: Controller RESET (IEEE reset) ...
[15769.995145] bcm43xx_d80211: select_wireless_core: cleanup
[15769.995166] bcm43xx_d80211: Radio turned off
[15769.995249] bcm43xx_d80211: DMA 0x0200 (RX) max used slots: 0/64
[15769.997222] bcm43xx_d80211: DMA 0x0260 (TX) max used slots: 0/512
[15769.997615] bcm43xx_d80211: DMA 0x0240 (TX) max used slots: 0/512
[15769.998006] bcm43xx_d80211: DMA 0x0220 (TX) max used slots: 0/512
[15769.998397] bcm43xx_d80211: DMA 0x0200 (TX) max used slots: 0/512
[15770.162013] bcm43xx_d80211: IRQ_READY timeout
[15770.162059] bcm43xx_d80211: core_up for active 802.11 core failed (-19)
[15770.162076] bcm43xx_d80211: Controller restart failed


Hence, I'm able to point to bcm43xx now ;) Sorry I can't give a better 
indication of what's up, I can try a different patch if you can come up 
with a good way of debugging it. This usually seems to happen here after 
an hour or two of not using the interface at all.

johannes

Re: bcm43xx for d80211 softirq loop

[Johannes Berg]

Here's the patch I used to find out which tasklet was giving me a hard 
time with bcm43xx and d80211. I don't really expect this to be applied 
anywhere but didn't want to sit on it either.

Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>

--- wireless-dev.orig/include/linux/interrupt.h    2006-08-13 
00:49:46.346865273 +0200
+++ wireless-dev/include/linux/interrupt.h    2006-08-13 
16:23:39.449545098 +0200
@@ -14,6 +14,7 @@
 #include <asm/atomic.h>
 #include <asm/ptrace.h>
 #include <asm/system.h>
+#include <linux/stringify.h>
 
 /*
  * These correspond to the IORESOURCE_IRQ_* defines in
@@ -267,6 +268,10 @@ struct tasklet_struct
     atomic_t count;
     void (*func)(unsigned long);
     unsigned long data;
+#ifdef CONFIG_DEBUG_TASKLETS
+    char *name;
+    unsigned int reschedule_count;
+#endif
 };
 
 #define DECLARE_TASKLET(name, func, data) \
@@ -348,8 +353,25 @@ static inline void tasklet_hi_enable(str
 
 extern void tasklet_kill(struct tasklet_struct *t);
 extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned 
int cpu);
-extern void tasklet_init(struct tasklet_struct *t,
-             void (*func)(unsigned long), unsigned long data);
+extern void _tasklet_init(struct tasklet_struct *t,
+              void (*func)(unsigned long), unsigned long data);
+#ifndef CONFIG_DEBUG_TASKLETS
+#define tasklet_init    _tasklet_init
+/* ignore name in the non-debug version, use a macro so the
+ * compiler can discard the string... */
+#define tasklet_init_named(t, func, data, name)    \
+    _tasklet_init(t, func, data)
+#else
+#define tasklet_init(t, func, data)    do {        \
+    _tasklet_init(t, func, data);            \
+    (t)->name = __FILE__ ":" __stringify(__LINE__);    \
+    } while (0)
+#define tasklet_init_named(t, func, data, name)    \
+    do {                    \
+    _tasklet_init(t, func, data);        \
+    (t)->name = (name);            \
+    } while (0)
+#endif
 
 /*
  * Autoprobing for irqs:
--- wireless-dev.orig/kernel/softirq.c    2006-08-13 00:56:37.206865273 
+0200
+++ wireless-dev/kernel/softirq.c    2006-08-13 16:34:31.079545098 +0200
@@ -385,9 +385,22 @@ static void tasklet_action(struct softir
                 if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
                     BUG();
                 t->func(t->data);
+#ifdef CONFIG_DEBUG_TASKLETS
+                t->reschedule_count = 0;
+#endif
                 tasklet_unlock(t);
                 continue;
             }
+#ifdef CONFIG_DEBUG_TASKLETS
+            /* 10000 is arbitrary... how much should it be?
+             * on my system, 10000 is quite a long time... */
+            if (t->reschedule_count++ > 10000) {
+                printk(KERN_ERR "tasklet %s is scheduled but hasn't been"
+                    " enabled for too long!\n", t->name);
+                tasklet_unlock(t);
+                continue;
+            }
+#endif
             tasklet_unlock(t);
         }
 
@@ -433,7 +446,7 @@ static void tasklet_hi_action(struct sof
 }
 
 
-void tasklet_init(struct tasklet_struct *t,
+void _tasklet_init(struct tasklet_struct *t,
           void (*func)(unsigned long), unsigned long data)
 {
     t->next = NULL;
@@ -443,7 +456,7 @@ void tasklet_init(struct tasklet_struct
     t->data = data;
 }
 
-EXPORT_SYMBOL(tasklet_init);
+EXPORT_SYMBOL(_tasklet_init);
 
 void tasklet_kill(struct tasklet_struct *t)
 {
--- wireless-dev.orig/lib/Kconfig.debug    2006-08-13 01:00:56.016865273 
+0200
+++ wireless-dev/lib/Kconfig.debug    2006-08-13 16:00:15.419545098 +0200
@@ -93,6 +93,16 @@ config SCHEDSTATS
       application, you can say N to avoid the very slight overhead
       this adds.
 
+config DEBUG_TASKLETS
+    bool "Debug runaway tasklets"
+    depends on DEBUG_KERNEL
+    help
+      If you say Y here, additional code will be inserted into the
+      lowlevel tasklet code in order to debug tasklets that are
+      scheduled but not enabled. If such a tasklet is found and
+      not enabled within a number of iterations, it is un-scheduled
+      and the kernel reports it.
+
 config DEBUG_SLAB
     bool "Debug slab memory allocations"
     depends on DEBUG_KERNEL && SLAB

Re: bcm43xx for d80211 softirq loop

[Michael Buesch]

On Monday 14 August 2006 10:10, Johannes Berg wrote:
> [23750.726463] NETDEV WATCHDOG: wmaster0: transmit timed out
> [23750.726482] wmaster0: resetting interface.
> [23750.726490] bcm43xx_d80211: Controller RESET (IEEE reset) ...
> [23750.753458] bcm43xx_d80211: select_wireless_core: cleanup
> [23750.753477] bcm43xx_d80211: Radio turned off
> [23750.753538] bcm43xx_d80211: DMA 0x0200 (RX) max used slots: 1/64
> [23750.755208] bcm43xx_d80211: DMA 0x0260 (TX) max used slots: 0/512
> [23750.755601] bcm43xx_d80211: DMA 0x0240 (TX) max used slots: 0/512
> [23750.755992] bcm43xx_d80211: DMA 0x0220 (TX) max used slots: 0/512
> [23750.756383] bcm43xx_d80211: DMA 0x0200 (TX) max used slots: 0/512
> [23750.957250] bcm43xx_d80211: Radio turned on
> [23751.129344] bcm43xx_d80211: Chip initialized
> [23751.137239] bcm43xx_d80211: DMA initialized
> [23751.137503] bcm43xx_d80211: Keys cleared
> [23751.153242] bcm43xx_d80211: Selected 802.11 core (phytype 2)
> [23751.153263] bcm43xx_d80211: Controller restarted
> 
> 
> after this, ksoftirq started running wild...
> 
> Since oprofile doesn't show any function ever called for a tasklet, I 
> started to investigate but couldn't find what was up. Then I removed the 
> bcm43xx_dscape module and the kernel blew up right away.
> 
> Then I patched the kernel with a runaway tasklet patch (see mail on 
> lkml) and got this (yeah, previous iteration of the patch where I forgot 
> a "\n"):
> 
> [15214.574151] NETDEV WATCHDOG: wmaster0: transmit timed out
> [15214.574166] wmaster0: resetting interface.
> [15214.574174] bcm43xx_d80211: Controller RESET (IEEE reset) ...
> [15214.591194] bcm43xx_d80211: select_wireless_core: cleanup
> [15214.591209] bcm43xx_d80211: Radio turned off
> [15214.591270] bcm43xx_d80211: DMA 0x0200 (RX) max used slots: 1/64
> [15214.592901] bcm43xx_d80211: DMA 0x0260 (TX) max used slots: 0/512
> [15214.593294] bcm43xx_d80211: DMA 0x0240 (TX) max used slots: 0/512
> [15214.593685] bcm43xx_d80211: DMA 0x0220 (TX) max used slots: 0/512
> [15214.594075] bcm43xx_d80211: DMA 0x0200 (TX) max used slots: 0/512
> [15214.773961] bcm43xx_d80211: Radio turned on
> [15214.945901] bcm43xx_d80211: Chip initialized
> [15214.953815] bcm43xx_d80211: DMA initialized
> [15214.954078] bcm43xx_d80211: Keys cleared
> [15214.969864] bcm43xx_d80211: Selected 802.11 core (phytype 2)
> [15214.969889] bcm43xx_d80211: Controller restarted
> [15215.019746] tasklet 
> drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c:4376 is scheduled but 
> hasn't been enabled for too long!
> <6>NETDEV WATCHDOG: wmaster0: transmit timed out
> [15769.964164] wmaster0: resetting interface.
> [15769.964175] bcm43xx_d80211: Controller RESET (IEEE reset) ...
> [15769.995145] bcm43xx_d80211: select_wireless_core: cleanup
> [15769.995166] bcm43xx_d80211: Radio turned off
> [15769.995249] bcm43xx_d80211: DMA 0x0200 (RX) max used slots: 0/64
> [15769.997222] bcm43xx_d80211: DMA 0x0260 (TX) max used slots: 0/512
> [15769.997615] bcm43xx_d80211: DMA 0x0240 (TX) max used slots: 0/512
> [15769.998006] bcm43xx_d80211: DMA 0x0220 (TX) max used slots: 0/512
> [15769.998397] bcm43xx_d80211: DMA 0x0200 (TX) max used slots: 0/512
> [15770.162013] bcm43xx_d80211: IRQ_READY timeout
> [15770.162059] bcm43xx_d80211: core_up for active 802.11 core failed (-19)
> [15770.162076] bcm43xx_d80211: Controller restart failed
> 
> 
> Hence, I'm able to point to bcm43xx now ;) Sorry I can't give a better 
> indication of what's up, I can try a different patch if you can come up 
> with a good way of debugging it. This usually seems to happen here after 
> an hour or two of not using the interface at all.

I saw this several times, too. I will start to debug this now.

-- 
Greetings Michael.

[d80211 rfc] link master interface from wiphy

[Johannes Berg]

I'd like to see a link from the wiphy to the master interface that 
belongs to it so one can tell this easily on systems that have multiple 
wireless devices. wpa_supplicant could use this, I guess. I think 
another link to wlan#ap should be created (or does wpa_supplicant set 
the name of that so it knows which one it will get?), or something like 
that anyway.

Here's a patch to just create the master link:

--- wireless-dev.orig/net/d80211/ieee80211.c    2006-08-11 
20:54:09.409674798 +0200
+++ wireless-dev/net/d80211/ieee80211.c    2006-08-11 21:26:22.629674798 
+0200
@@ -4449,6 +4449,7 @@ int ieee80211_register_hw(struct net_dev
         rtnl_unlock();
         goto fail_dev;
     }
+    sysfs_create_link(&local->class_dev.kobj, &dev->class_dev.kobj, 
"master");
     result = ieee80211_sysfs_add_netdevice(dev);
     rtnl_unlock();
     if (result < 0)


On the other hand, is there any real reason we have this code:
        ndev->base_addr = dev->base_addr;
        ndev->irq = dev->irq;
        ndev->mem_start = dev->mem_start;
        ndev->mem_end = dev->mem_end;
        ndev->flags = dev->flags & IFF_MULTICAST;
        SET_NETDEV_DEV(ndev, dev->class_dev.dev);

in ieee80211_if_add? Maybe we should make the virtual devices all 
children of the wiphy (struct ieee80211_local) instead of making them 
children of the physical device? I don't really know though. This is too 
dark magic for me ;)

However, I do know that I can trivially rename the wmaster0 interface 
using just 'ip link set wmaster0 name wlan3' and things will probably be 
very confusing for any program that relies on the naming to know which 
device is which. Hence, I think we need some symlinks here to be able to 
tell which device is which. Or maybe we should directly surface the 
ifindex in some sysfs attributes ;)

Comments welcome. Userspace comments as well, I'm programming something 
that'll use a bunch of interfaces (wmaster, a monitor one and a sta one 
probably) and I want the user to just select the physical interface, not 
all these three logical ones... (in fact, I'm creating the logical 
monitor interface myself in code).

johannes

PS: Yes, I do realize that doing
ip link set wlan0 name xxx
ip link set wmaster0 name wlan0
ip link set xxx name wmaster0

is confusing. But since it is possible things shouldn't fall over if the 
user decides for some weird local device naming. And wpa_supplicant 
shouldn't need to require being configured all the device names either. 
In fact IMHO just giving it a physical device (say via mac address) 
ought to be enough...

Re: [d80211 rfc] link master interface from wiphy

[Dan Williams]

On Mon, 2006-08-14 at 10:12 +0200, Johannes Berg wrote:
> I'd like to see a link from the wiphy to the master interface that 
> belongs to it so one can tell this easily on systems that have multiple 
> wireless devices. wpa_supplicant could use this, I guess. I think 
> another link to wlan#ap should be created (or does wpa_supplicant set 
> the name of that so it knows which one it will get?), or something like 
> that anyway.

HAL can certainly use this as well.  Two of the most useful things for
HAL (and by extension NetworkManager) were the 'device' and 'driver'
links, and this is certainly in the same class as those two.

Dan

> Here's a patch to just create the master link:
> 
> --- wireless-dev.orig/net/d80211/ieee80211.c    2006-08-11 
> 20:54:09.409674798 +0200
> +++ wireless-dev/net/d80211/ieee80211.c    2006-08-11 21:26:22.629674798 
> +0200
> @@ -4449,6 +4449,7 @@ int ieee80211_register_hw(struct net_dev
>          rtnl_unlock();
>          goto fail_dev;
>      }
> +    sysfs_create_link(&local->class_dev.kobj, &dev->class_dev.kobj, 
> "master");
>      result = ieee80211_sysfs_add_netdevice(dev);
>      rtnl_unlock();
>      if (result < 0)
> 
> 
> On the other hand, is there any real reason we have this code:
>         ndev->base_addr = dev->base_addr;
>         ndev->irq = dev->irq;
>         ndev->mem_start = dev->mem_start;
>         ndev->mem_end = dev->mem_end;
>         ndev->flags = dev->flags & IFF_MULTICAST;
>         SET_NETDEV_DEV(ndev, dev->class_dev.dev);
> 
> in ieee80211_if_add? Maybe we should make the virtual devices all 
> children of the wiphy (struct ieee80211_local) instead of making them 
> children of the physical device? I don't really know though. This is too 
> dark magic for me ;)
> 
> However, I do know that I can trivially rename the wmaster0 interface 
> using just 'ip link set wmaster0 name wlan3' and things will probably be 
> very confusing for any program that relies on the naming to know which 
> device is which. Hence, I think we need some symlinks here to be able to 
> tell which device is which. Or maybe we should directly surface the 
> ifindex in some sysfs attributes ;)
> 
> Comments welcome. Userspace comments as well, I'm programming something 
> that'll use a bunch of interfaces (wmaster, a monitor one and a sta one 
> probably) and I want the user to just select the physical interface, not 
> all these three logical ones... (in fact, I'm creating the logical 
> monitor interface myself in code).
> 
> johannes
> 
> PS: Yes, I do realize that doing
> ip link set wlan0 name xxx
> ip link set wmaster0 name wlan0
> ip link set xxx name wmaster0
> 
> is confusing. But since it is possible things shouldn't fall over if the 
> user decides for some weird local device naming. And wpa_supplicant 
> shouldn't need to require being configured all the device names either. 
> In fact IMHO just giving it a physical device (say via mac address) 
> ought to be enough...
> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [d80211 rfc] link master interface from wiphy

[Jiri Benc]

On Mon, 14 Aug 2006 10:12:01 +0200, Johannes Berg wrote:
> I'd like to see a link from the wiphy to the master interface that 
> belongs to it so one can tell this easily on systems that have multiple 
> wireless devices.

As wiphy and master interface are closely bind to each other, this makes
sense.

Btw, we will probably need some way to ask d80211 about all interfaces
belonging to given wiphy anyway. Crawling all network interfaces and
searching for correct wiphy symlinks is probably not the best way. I
think a new netlink interface can be used for this.

> wpa_supplicant could use this, I guess. I think 
> another link to wlan#ap should be created (or does wpa_supplicant set 
> the name of that so it knows which one it will get?), or something like 
> that anyway.

wmgmt# will go away in future. There is an ioctl to get its ifindex, so
no need for the link.

> On the other hand, is there any real reason we have this code:
>         ndev->base_addr = dev->base_addr;
>         ndev->irq = dev->irq;
>         ndev->mem_start = dev->mem_start;
>         ndev->mem_end = dev->mem_end;
>         ndev->flags = dev->flags & IFF_MULTICAST;
>         SET_NETDEV_DEV(ndev, dev->class_dev.dev);
> 
> in ieee80211_if_add? Maybe we should make the virtual devices all 
> children of the wiphy (struct ieee80211_local) instead of making them 
> children of the physical device? I don't really know though. This is too 
> dark magic for me ;)

What do you mean by "making the virtual devices all children of the
wiphy"? Currently, all virtual devices (of one physical device) have the
same pointer to ieee80211_local in their net_dev structure and pointers
to them are stored in the linked list in ieee80211_local.

> However, I do know that I can trivially rename the wmaster0 interface 
> using just 'ip link set wmaster0 name wlan3' and things will probably be 
> very confusing for any program that relies on the naming to know which 
> device is which.

Any program that relies on particular device names is broken.

> Comments welcome. Userspace comments as well, I'm programming something 
> that'll use a bunch of interfaces (wmaster, a monitor one and a sta one 
> probably) and I want the user to just select the physical interface, not 
> all these three logical ones... (in fact, I'm creating the logical 
> monitor interface myself in code).

Do you know about /sys/class/net/X/wiphy symlinks? But as I said,
crawling sysfs is not the best idea - among other things, it is subject
to race conditions.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [d80211 rfc] link master interface from wiphy

[Johannes Berg]

On Wed, 2006-08-16 at 19:05 +0200, Jiri Benc wrote:

> Btw, we will probably need some way to ask d80211 about all interfaces
> belonging to given wiphy anyway. Crawling all network interfaces and
> searching for correct wiphy symlinks is probably not the best way. I
> think a new netlink interface can be used for this.

Yeah, that should be fairly easy.

> > wpa_supplicant could use this, I guess. I think 
> > another link to wlan#ap should be created (or does wpa_supplicant set 
> > the name of that so it knows which one it will get?), or something like 
> > that anyway.
> 
> wmgmt# will go away in future. There is an ioctl to get its ifindex, so
> no need for the link.

Right, Jouni pointed that out already. Forget what I said :) Besides,
I'm already working hard on deprecating it :P

> What do you mean by "making the virtual devices all children of the
> wiphy"? Currently, all virtual devices (of one physical device) have the
> same pointer to ieee80211_local in their net_dev structure and pointers
> to them are stored in the linked list in ieee80211_local.

I was wondering why they in sysfs have their device link set to the
physical device instead of the master wiphy. I'm not sure if the latter
makes sense though.

> Do you know about /sys/class/net/X/wiphy symlinks?

yes, I'm actually using them :)

Thanks,
johannes

[PATCH] d80211: fix some 0 vs. NULL comparisons

[Johannes Berg]

This patch fixes places where pointers are compared against 0 and 
unifies it a bit.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

--- wireless-dev.orig/net/d80211/ieee80211_scan.c    2006-08-12 
10:11:21.069644280 +0200
+++ wireless-dev/net/d80211/ieee80211_scan.c    2006-08-12 
10:11:44.619644280 +0200
@@ -117,7 +117,7 @@ static void ieee80211_scan_start(struct
     struct ieee80211_channel *chan = NULL;
     int ret;
 
-    if (local->hw->passive_scan == 0) {
+    if (local->hw->passive_scan == NULL) {
         printk(KERN_DEBUG "%s: Scan handler called, yet the hardware "
                "does not support passive scanning. Disabled.\n",
                dev->name);
@@ -293,7 +293,7 @@ void ieee80211_init_scan(struct net_devi
     struct rate_control_extra extra;
 
     /* Only initialize passive scanning if the hardware supports it */
-    if (!local->hw->passive_scan) {
+    if (local->hw->passive_scan == NULL) {
         local->scan.skb = NULL;
         memset(&local->scan.tx_control, 0,
                sizeof(local->scan.tx_control));
@@ -344,7 +344,7 @@ void ieee80211_stop_scan(struct net_devi
 {
     struct ieee80211_local *local = dev->ieee80211_ptr;
 
-    if (local->hw->passive_scan != 0) {
+    if (local->hw->passive_scan != NULL) {
         del_timer_sync(&local->scan.timer);
         dev_kfree_skb(local->scan.skb);
         local->scan.skb = NULL;

Re: [PATCH] d80211: fix some 0 vs. NULL comparisons

[Johannes Berg]

Johannes Berg wrote:
> -    if (!local->hw->passive_scan) {
> +    if (local->hw->passive_scan == NULL) {

Alright, this is icky. I'll make another pass and change it all to if 
(x) or if (!x) instead of comparing to NULL. Don't hold your breath 
though, earliest next weekend.

johannes

Re: [PATCH] d80211: fix some 0 vs. NULL comparisons

[Jouni Malinen]

On Mon, Aug 14, 2006 at 03:20:58PM +0200, Johannes Berg wrote:
> Johannes Berg wrote:
> >-    if (!local->hw->passive_scan) {
> >+    if (local->hw->passive_scan == NULL) {
> 
> Alright, this is icky. I'll make another pass and change it all to if 
> (x) or if (!x) instead of comparing to NULL. Don't hold your breath 
> though, earliest next weekend.

Well.. I'm perfectly fine with comparing function pointers to NULL in
this kind of case.. In many cases, I find fp == NULL to be clearer than
!fp, but for the fp != NULL case I would rather not see != NULL.. Not
very consistent, but can't really help with that on this kind of coding
style opinions. Anyway, replacing 0 with NULL is a good change.

-- 
Jouni Malinen                                            PGP id EFC895FA

[PATCH] d80211: get rid of the WME bitfield

[Johannes Berg]

Somewhere, sometime, someone had to start getting rid of bitfields ;) 
This one seemed an easy target :P

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

--- wireless-dev.orig/net/d80211/wme.c    2006-08-12 10:43:01.809644280 
+0200
+++ wireless-dev/net/d80211/wme.c    2006-08-12 10:51:35.909644280 +0200
@@ -246,15 +246,13 @@ static int wme_qdiscop_enqueue(struct sk
     /* now we know the 1d priority, fill in the QoS header if there is one
      */
     if (WLAN_FC_IS_QOS_DATA(fc)) {
-        struct qos_control *qc = (struct qos_control *)
-            (skb->data + ieee80211_get_hdrlen(fc) - 2);
-        u8 *p = (u8 *) qc;
-        *p++ = 0; /* do this due to gcc's lack of optimization on
-               * bitfield ops */
-        *p = 0;
-        qc->tag1d = skb->priority;
+        /* hard-coded QOS control header length! */
+        u16 *p = (u16 *) (skb->data + ieee80211_get_hdrlen(fc) - 2);
+        u16 qos_hdr = skb->priority & QOS_CONTROL_TAG1D_MASK;
         if (local->wifi_wme_noack_test)
-            qc->ack_policy = 1;
+            qos_hdr |= QOS_CONTROL_ACK_POLICY_NOACK <<
+                    QOS_CONTROL_ACK_POLICY_SHIFT;
+        *p = cpu_to_le16(qos_hdr);
     }
 
     if (unlikely(queue >= local->hw->queues)) {
--- wireless-dev.orig/net/d80211/wme.h    2006-08-12 10:53:18.899644280 
+0200
+++ wireless-dev/net/d80211/wme.h    2006-08-12 10:53:21.629644280 +0200
@@ -24,26 +24,7 @@
 #define QOS_CONTROL_TID_MASK 0x0f
 #define QOS_CONTROL_ACK_POLICY_SHIFT 5
 
-/* This bit field structure should not be used; it can cause compiler to
- * generate unaligned accesses and inefficient code. */
-struct qos_control {
-#if defined(__LITTLE_ENDIAN_BITFIELD)
-    u8    tag1d:3, /* bits 0-2 */
-          reserved1:1,
-        eosp:1,
-        ack_policy:2,
-          reserved2:1;
-#elif defined (__BIG_ENDIAN_BITFIELD)
-    u8    reserved2:1,
-        ack_policy:2,
-        eosp:1,
-        reserved1:1,
-        tag1d:3; /* bits 0-2 */
-#else
-#error    "Please fix <asm/byteorder.h>"
-#endif
-    u8 reserved;
-} __attribute__ ((packed));
+#define QOS_CONTROL_TAG1D_MASK 0x07
 
 ieee80211_txrx_result
 ieee80211_rx_h_parse_qos(struct ieee80211_txrx_data *rx);

Re: [PATCH] d80211: get rid of the WME bitfield

[Jouni Malinen]

On Mon, Aug 14, 2006 at 10:15:14AM +0200, Johannes Berg wrote:
> Somewhere, sometime, someone had to start getting rid of bitfields ;) 

Yes and you wouldn't believe how many times I have had to complain about
this particular case internally with it always coming back as bitfield
even after showing that it caused a unaligned access on one target
platform..

However, I remember fixing this long time ago to use proper operation on
u8 values. It looks like I just have forgotten to submit those changes
to wireless-dev.git. There was quite a bit of additional cleanup, so it
would probably be best if I were to take a closer look at propose an
alternative patch for changing this and getting rid for struct
qos_control.

> +        u16 *p = (u16 *) (skb->data + ieee80211_get_hdrlen(fc) - 2);
> +        u16 qos_hdr = skb->priority & QOS_CONTROL_TAG1D_MASK;
>         if (local->wifi_wme_noack_test)
> -            qc->ack_policy = 1;
> +            qos_hdr |= QOS_CONTROL_ACK_POLICY_NOACK <<
> +                    QOS_CONTROL_ACK_POLICY_SHIFT;
> +        *p = cpu_to_le16(qos_hdr);

I think that this is always 16-bit aligned in the current
implementation, but I would be a bit careful with this type of change
here. This can be done easily with u8 since the qos_control field has 8
bits of used data and 8 bits of reserved field. My change was handling
this as two separate 8-bit values, so no issues with potential unaligned
accesses.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH] d80211: get rid of the WME bitfield

[Johannes Berg]

Jouni Malinen wrote:
> I think that this is always 16-bit aligned in the current
> implementation, but I would be a bit careful with this type of change
> here. This can be done easily with u8 since the qos_control field has 8
> bits of used data and 8 bits of reserved field. My change was handling
> this as two separate 8-bit values, so no issues with potential unaligned
> accesses.
>   
Good point, I forgot about that. Never mind this patch then, if I get to 
it I'll make a new one.

johannes

ieee80211_japan_5ghz / firmware etc.??

[Johannes Berg]

I think we need to get rid of this parameter and if really necessary 
have this information passed from the lowlevel driver to the stack.

johannes

ieee80211_set_encryption...

[Johannes Berg]

... is a big mess.

What's with all the comments saying 'maybe with blabla hardware that can 
be done in hw but disable here now' etc? Can't we just have a 'please 
decide' callback in the driver that tells us whether this can be done in 
hw or sw?

Or how about no_tkip_wmm_hwaccel? That seems pretty weird too.

I do realize that key management in the face of wpa2 and similar is 
difficult, but this seems overly complex. Comments?

johannes

Re: ieee80211_set_encryption...

[Jouni Malinen]

On Mon, Aug 14, 2006 at 10:16:53AM +0200, Johannes Berg wrote:
> ... is a big mess.

Yes, and so is the number of different ways this has been implemented in
hardware designs..

> What's with all the comments saying 'maybe with blabla hardware that can 
> be done in hw but disable here now' etc? Can't we just have a 'please 
> decide' callback in the driver that tells us whether this can be done in 
> hw or sw?

For some cases yes, but it is a bit difficult to come up with a generic
model that would work for everything. Supporting multiple BSSes in AP
mode makes some quite complex cases.

> Or how about no_tkip_wmm_hwaccel? That seems pretty weird too.

That's needed to allow TKIP hwaccel to be used for non-WMM case while
falling back to software for WMM. This is needed to workaround some
hardware issues. Sure, this could be hidden in the hardware driver, but
I would prefer to allow the 802.11 stack support software
encryption/decryption to keep the low-level drivers simpler (and to
avoid their authors from doing some silly copy-paste things with
encryption).

> I do realize that key management in the face of wpa2 and similar is 
> difficult, but this seems overly complex. Comments?

WPA2 is not the complex part; adding WMM into the picture with some
hardware design was and multi-BSS support adds quite a bit more
complexity here.

-- 
Jouni Malinen                                            PGP id EFC895FA

network manager confused with bcm43xx-d80211?

[Johannes Berg]

For whatever reason, nm always thinks that my default wlan0 device is a 
*wired* device. I do wonder where the bug is, but couldn't pinpoint it 
yet, any ideas? mm version is 0.6.3. I'd have thought it would go by the 
presence of wireless extensions, but if so it uses some call to detect 
it that isn't implemented here...

johannes

Re: network manager confused with bcm43xx-d80211?

[Dan Williams]

On Mon, 2006-08-14 at 10:18 +0200, Johannes Berg wrote:
> For whatever reason, nm always thinks that my default wlan0 device is a 
> *wired* device. I do wonder where the bug is, but couldn't pinpoint it 
> yet, any ideas? mm version is 0.6.3. I'd have thought it would go by the 
> presence of wireless extensions, but if so it uses some call to detect 
> it that isn't implemented here...

No, it asks HAL what the device type is actually.  HAL looks for the
interface in /proc/net/wireless currently, and if it's not there, says
the device is wired.  Run 'lshal', look for your wlan device, and see if
info.capabilities includes 'net.80211'.

So if the wlanX interface isn't in /proc/net/wireless, you lose.  I
think we discussed this in the private rt2x00 thread about a month ago?
Some patches came out of that one, specifically the SET_NETDEV_DEV ones
for rt2x00 and d80211.  The problem with d80211 was that it didn't
implement the get_wireless_stats() handler function, and that function
(I think?) is the one that lets the wlanX device show up
in /proc/net/wireless.

Dan

Re: network manager confused with bcm43xx-d80211?

[Johannes Berg]

Dan Williams wrote:
> No, it asks HAL what the device type is actually.  HAL looks for the
> interface in /proc/net/wireless currently, and if it's not there, says
> the device is wired.  Run 'lshal', look for your wlan device, and see if
> info.capabilities includes 'net.80211'.
>   
Yeah, I noticed it wasn't in /proc/net/wireless. Should've tipped me off 
I guess. And I probably also should've thought that nm would ask HAL. 
Looks like I wasn't really on track when I wrote this yesterday ;)
> So if the wlanX interface isn't in /proc/net/wireless, you lose.  I
> think we discussed this in the private rt2x00 thread about a month ago?
> Some patches came out of that one, specifically the SET_NETDEV_DEV ones
> for rt2x00 and d80211.  The problem with d80211 was that it didn't
> implement the get_wireless_stats() handler function, and that function
> (I think?) is the one that lets the wlanX device show up
> in /proc/net/wireless.
>   
Yeah, that sounds about right. Larry, do you remember/know more about 
this? I think you were working in this area?

johannes

Re: network manager confused with bcm43xx-d80211?

[Larry Finger]

Johannes Berg wrote:
> Dan Williams wrote:
> Yeah, I noticed it wasn't in /proc/net/wireless. Should've tipped me off 
> I guess. And I probably also should've thought that nm would ask HAL. 
> Looks like I wasn't really on track when I wrote this yesterday ;)
>> So if the wlanX interface isn't in /proc/net/wireless, you lose.  I
>> think we discussed this in the private rt2x00 thread about a month ago?
>> Some patches came out of that one, specifically the SET_NETDEV_DEV ones
>> for rt2x00 and d80211.  The problem with d80211 was that it didn't
>> implement the get_wireless_stats() handler function, and that function
>> (I think?) is the one that lets the wlanX device show up
>> in /proc/net/wireless.
>>   
> Yeah, that sounds about right. Larry, do you remember/know more about 
> this? I think you were working in this area?

Yes, get_wireless_stats is needed for the device to show up in /proc/net/wireless.

The RFCd patch to add wireless stats to d80211 received only minor comments, 
which have all been addressed. I will be submitting a proper patch to Linville's 
tree today. In addition, I have reached an agreement with the authors of all 
other drivers that use d80211 on a scheme that will implement the stats code for 
them as well. Unfortunately, none of them provide such useful quantities as 
noise values, but perhaps the prominent FIXMEs will spur action by those who 
have detailed device specifications.

Larry

Re: network manager confused with bcm43xx-d80211?

[Johannes Berg]

Larry Finger wrote:
> The RFCd patch to add wireless stats to d80211 received only minor 
> comments, which have all been addressed. I will be submitting a proper 
> patch to Linville's tree today. In addition, I have reached an 
> agreement with the authors of all other drivers that use d80211 on a 
> scheme that will implement the stats code for them as well. 
> Unfortunately, none of them provide such useful quantities as noise 
> values, but perhaps the prominent FIXMEs will spur action by those who 
> have detailed device specifications.

Sounds good to me, thanks :) I'll take a look at bcm43xx signal 
strength/quality reverse engineering, I promise. Just have to get to it, 
right now I'm using a rt2500usb device which doesn't seem to give any 
useful values.

johannes

d80211 and sta_aid for AP functionality

[Johannes Berg]

Hey,

I was looking through the d80211 code and noticed this comment and code:

        /* TODO: sta_aid could be replaced by 2008-bit large bitfield of
         * that could be used in TIM element generation. This would also
         * make TIM element generation a bit faster. */
        /* AID mapping to station data. NULL, if AID is free. AID is in the
         * range 1..2007 and sta_aid[i] corresponds to AID i+1. */
        struct sta_info *sta_aid[MAX_AID_TABLE_SIZE];

Note that this is part of struct ieee80211_if_ap which is in a union for 
each virtual device.

About 15 seconds later it almost blew me off my chair ;) I had realized 
that this is 8k kernel memory for each virtual device with absolutely no 
function in the common case! AX_AID_TABLE_SIZE is set to 2007 (the 
802.11 spec maximum)...

In fact, with this, the struct ieee80211_sub_if_data is 8560 bytes, 
without it it's a meager 588 bytes. Hence, when the interface is 
anything but an AP interface, we waste almost 8K. And even if it's an AP 
interface, who ever has 2007 stations associated?

Since the stations are saved in a hash-table (the hashing by the last 
byte of the mac address doesn't seem too efficient though...) all the 
sta_aids array needs to do is answer the following questions quickly:
 (a) is the AID N used?
 (b) what is the sta_info pointer for AID N?

Since userspace manages AIDs, we don't even need to be able to answer 
things like "what is the lowest unused AID?". I don't quite understand 
why we don't stop userspace from using the same AID for different 
stations, which will give surely result in trouble, but hey.

Looking through the code, I notice that above (a) and (b) can be 
collapsed into just keeping track of the TIM throughout the code. I 
suppose that's meant by the comment above? Then we can't add the check 
for saying 'hey you stupid userspace that AID is already used' but 
that's ok I guess.

If you can confirm that my analysis is correct I'd be willing to try 
making this change. This would at least go down to 251 bytes for the 
bitmap and thus reduce sub_if_data to about 760 bytes. The only 
complication I see with this right now is that of locking, but I haven't 
looked closely yet.

johannes

Re: d80211 and sta_aid for AP functionality

[Jiri Benc]

On Mon, 14 Aug 2006 10:19:24 +0200, Johannes Berg wrote:
> Looking through the code, I notice that above (a) and (b) can be 
> collapsed into just keeping track of the TIM throughout the code. I 
> suppose that's meant by the comment above? Then we can't add the check 
> for saying 'hey you stupid userspace that AID is already used' but 
> that's ok I guess.
> 
> If you can confirm that my analysis is correct I'd be willing to try 
> making this change.

You understand it right (or at least the same way as me :-)) and this
effort would be highly appreciated.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs

wlan#ap seems bogus

[Johannes Berg]

As far as I understand the entire point of the wlan#ap interface is to
receive all management relevant management frames. (If that's wrong, reply
now and don't read the rest)

Hence, I think it ought to be named 'wlan#mgmt' instead. However, I think
it's hard-coded existence is bogus.

How about we just add a new interface mode called MGT_MONITOR and
wpa_supplicant simply creates a new device via the regular sysfs mechanism,
and then sets that MGT_MONITOR mode via the relevant WEXT ioctl? iwconfig
doesn't even need to be taught about this mode except for display
purposes...

Also, like I mentioned previously, the device naming can be changed by the
user and there's no guarantee that wlan0ap corresponds to wmaster0. Etc. But
if wpa_supplicant simply created the device itself, it could give it any
name, say wlan0_mgt (if it was controlling wlan0), but if that name is
already taken it simply appends a number or something until it hits a device
name that's still free (or gives up after 100 tries or so...) :)

johannes

PS: In case you're wondering, this is the last mail in my series. Phew!

Re: wlan#ap seems bogus

[Johannes Berg]

Johannes Berg wrote:
> Hence, I think it ought to be named 'wlan#mgmt' instead.

I see it's actually called wmgmt# now. Sorry. The rest of this mail 
still holds though.

johannes

RE: wlan#ap seems bogus

[Simon Barber]

The purpose of the wlap0ap or wlap0mgmt interface is to communicate
between hostapd/wpa_supplicant and the kernel. What travels over this
interface is not quite pure 802.11 management frames - there is some
meta-data with each frame, and a few special case messages. E.G.
transmitted frames are returned back to user space to indicate
successful transmission (required so that the MLME state machine can be
correctly implemented in user space). I believe these messages form a
special management protocol between the kernel and user space, and that
netlink would be the best solution for this link. Switching to netlink
would allow these bogus 'network' interfaces to be removed alltogether.
Unfortunately I don't currently have the time to put together a patch to
do this.

Simon


-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Johannes Berg
Sent: Monday, August 14, 2006 7:04 AM
To: netdev@vger.kernel.org
Cc: Jouni Malinen; Jiri Benc
Subject: Re: wlan#ap seems bogus

Johannes Berg wrote:
> Hence, I think it ought to be named 'wlan#mgmt' instead.

I see it's actually called wmgmt# now. Sorry. The rest of this mail
still holds though.

johannes
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the
body of a message to majordomo@vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html

Re: wlan#ap seems bogus

[Johannes Berg]

Simon Barber wrote:
> The purpose of the wlap0ap or wlap0mgmt interface is to communicate
> between hostapd/wpa_supplicant and the kernel. What travels over this
> interface is not quite pure 802.11 management frames - there is some
> meta-data with each frame, and a few special case messages. E.G.
> transmitted frames are returned back to user space to indicate
> successful transmission (required so that the MLME state machine can be
> correctly implemented in user space). I believe these messages form a
> special management protocol between the kernel and user space, and that
> netlink would be the best solution for this link. Switching to netlink
> would allow these bogus 'network' interfaces to be removed alltogether.
> Unfortunately I don't currently have the time to put together a patch to
> do this.
>   
Ok, I'll try to remember that this is waiting on defining a complete new 
netlink API. I was just reading the code and this seemed odd, but I 
agree that no network interface is really needed and hence it can stay 
like this until we switch to a new API unifying it.

Thanks,
johannes

Re: wlan#ap seems bogus

[Jouni Malinen]

On Mon, Aug 14, 2006 at 10:22:34AM +0200, Johannes Berg wrote:
> As far as I understand the entire point of the wlan#ap interface is to
> receive all management relevant management frames. (If that's wrong, reply
> now and don't read the rest)
> 
> Hence, I think it ought to be named 'wlan#mgmt' instead. However, I think
> it's hard-coded existence is bogus.
> 
> How about we just add a new interface mode called MGT_MONITOR and
> wpa_supplicant simply creates a new device via the regular sysfs mechanism,
> and then sets that MGT_MONITOR mode via the relevant WEXT ioctl? iwconfig
> doesn't even need to be taught about this mode except for display
> purposes...

Isn't this already done? I don't remember what happened with client mode
case, but at least hostapd is requesting the management interface
dynamically at startup time.

> Also, like I mentioned previously, the device naming can be changed by the
> user and there's no guarantee that wlan0ap corresponds to wmaster0. Etc. But
> if wpa_supplicant simply created the device itself, it could give it any
> name, say wlan0_mgt (if it was controlling wlan0), but if that name is
> already taken it simply appends a number or something until it hits a device
> name that's still free (or gives up after 100 tries or so...) :)

Same here.. hostapd gets the ifindex from kernel and doesn't care about
the name.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: wlan#ap seems bogus

[Johannes Berg]

Jouni Malinen wrote:
>> How about we just add a new interface mode called MGT_MONITOR and
>> wpa_supplicant simply creates a new device via the regular sysfs mechanism,
>> and then sets that MGT_MONITOR mode via the relevant WEXT ioctl? iwconfig
>> doesn't even need to be taught about this mode except for display
>> purposes...
>>     
>
> Isn't this already done? I don't remember what happened with client mode
> case, but at least hostapd is requesting the management interface
> dynamically at startup time.
>
>   
Yes, it's requesting it, but via a private wext call. I'd like to see it 
generic, and allow multiple mgmt interfaces as well if the user/hw so 
wishes. IOW I'd like to remove the iwpriv call and instead use the 
equivalent of "echo -n wmgmt0 > /sys/class/ieee80211/phy0/add_iface ; 
iwconfig wmgmt0 mode management"

> Same here.. hostapd gets the ifindex from kernel and doesn't care about
> the name.
>   
Good point, I overlooked that.

johannes