bunch of questions and notes on d80211

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

[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

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.

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

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

Greetings Michael.

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:

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

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

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.

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

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.

Any program that relies on particular device names is broken.

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

Yeah, that should be fairly easy.

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

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.

yes, I'm actually using them :)

Thanks,
johannes

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

@@ -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;

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

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

@@ -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)) {

@@ -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);

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.

Jouni Malinen                                            PGP id EFC895FA

Good point, I forgot about that. Never mind this patch then, if I get to it I'll make a new one.

johannes

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

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

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

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.

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

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

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

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

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 ;)

Yeah, that sounds about right. Larry, do you remember/know more about this? I think you were working in this area?

johannes

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

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

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

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

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!

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

johannes

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: [email protected] [mailto:[email protected]] On Behalf Of Johannes Berg
Sent: Monday, August 14, 2006 7:04 AM
To: [email protected]
Cc: Jouni Malinen; Jiri Benc
Subject: Re: wlan#ap seems bogus

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 [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

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

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.

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

Jouni Malinen                                            PGP id EFC895FA

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"

Good point, I overlooked that.

johannes