compat-wireless and minstrel

I have two systems under test, both Dell laptops (a Latitude D630 and an Inspiron 600m) both running Ubuntu 9.10 with the latest updates, and bleeding edge compat-wireless-2009-11-02. I'm using identical AR9170 based D-Link DWA-160 USB 802.11adapters. I'm using nuttcp to measure throughput. I'm running in ad-hoc mode. Both machines have the same ar9170 files in /lib/firmware. The machines are sitting about 5 feet apart in my office.

I'm having occasional problems where throughput drops through the floor (0.5Mbps - 1.5Mbps). When I cat /sys/kernel/debug/ieee80211/*/stations/*/rc_stats, one of the machines lists the full set of rates, but the other only lists 1M and 54M. After a period of time, that machine drops 54M and lists only one rate (1Mbps), and the throughput listed by nuttcp drops accordingly. I assume that, for whatever reason, the rates drop off the list and minstrel uses the only one left available to it.

If I modify include/net/mac80211.h and force the inline function rate_supported to always return 1, this fixes the problem. However, I think this is a band aid around some other issue.

Any clues or ideas what the real issue might be here?

Quoted 17 lines · On Wednesday 04 November 2009 02:13:49 Adam Wozniak wrote:

I have two systems under test, both Dell laptops (a Latitude D630 and an Inspiron 600m) both running Ubuntu 9.10 with the latest updates, and bleeding edge compat-wireless-2009-11-02.  I'm using identical AR9170 based D-Link DWA-160 USB 802.11adapters.  I'm using nuttcp to measure throughput.  I'm running in ad-hoc mode.  Both machines have the same ar9170 files in /lib/firmware.  The machines are sitting about 5 feet apart in my office.

I'm having occasional problems where throughput drops through the floor (0.5Mbps - 1.5Mbps).  When I cat /sys/kernel/debug/ieee80211/*/stations/*/rc_stats, one of the machines lists the full set of rates, but the other only lists 1M and 54M.  After a period of time, that machine drops 54M and lists only one rate (1Mbps), and the throughput listed by nuttcp drops accordingly.  I assume that, for whatever reason, the rates drop off the list and minstrel uses the only one left available to it.

Quoted 5 lines

If I modify include/net/mac80211.h and force the inline function rate_supported to always return 1, this fixes the problem.  However, I think this is a band aid around some other issue.

Any clues or ideas what the real issue might be here?

no, but there are a few knobs you can play with...

First of: more debug data

1. enable some ibss-related verbose messages open config.mk (in your compat-wireless directory) and get rid of the leading "# " <-- "sharp + space" in front of # CONFIG_MAC80211_IBSS_DEBUG=y (rebuild, reload, retest & checkout dmesg for a _flood_ of potential interesting data)

Quoted 17 lines · On Wednesday 04 November 2009 02:13:49 Adam Wozniak wrote:

I have two systems under test, both Dell laptops (a Latitude D630 and an Inspiron 600m) both running Ubuntu 9.10 with the latest updates, and bleeding edge compat-wireless-2009-11-02. I'm using identical AR9170 based D-Link DWA-160 USB 802.11adapters. I'm using nuttcp to measure throughput. I'm running in ad-hoc mode. Both machines have the same ar9170 files in /lib/firmware. The machines are sitting about 5 feet apart in my office.

I'm having occasional problems where throughput drops through the floor (0.5Mbps - 1.5Mbps). When I cat /sys/kernel/debug/ieee80211/*/stations/*/rc_stats, one of the machines lists the full set of rates, but the other only lists 1M and 54M. After a period of time, that machine drops 54M and lists only one rate (1Mbps), and the throughput listed by nuttcp drops accordingly. I assume that, for whatever reason, the rates drop off the list and minstrel uses the only one left available to it.

Quoted 5 lines

If I modify include/net/mac80211.h and force the inline function rate_supported to always return 1, this fixes the problem. However, I think this is a band aid around some other issue.

Any clues or ideas what the real issue might be here?

Quoted 1 line

/sys/kernel/debug/ieee80211/*/stations/*/rc_stats

Hi,

Quoted 8 lines · Christian Lamparter

Quoted 7 lines · On Wednesday 04 November 2009 02:13:49 Adam Wozniak wrote:

I have two systems under test, both Dell laptops (a Latitude D630 and an Inspiron 600m) both running Ubuntu 9.10 with the latest updates, and bleeding edge compat-wireless-2009-11-02. I'm using identical AR9170 based D-Link DWA-160 USB 802.11adapters. I'm using nuttcp to measure throughput. I'm running in ad-hoc mode. Both machines have the same ar9170 files in /lib/firmware. The machines are sitting about 5 feet apart in my office.

Quoted 9 lines

Quoted 9 lines

I'm having occasional problems where throughput drops through the floor (0.5Mbps - 1.5Mbps). When I cat /sys/kernel/debug/ieee80211/*/stations/*/rc_stats, one of the machines lists the full set of rates, but the other only lists 1M and 54M. After a period of time, that machine drops 54M and lists only one rate (1Mbps), and the throughput listed by nuttcp drops accordingly. I assume that, for whatever reason, the rates drop off the list and minstrel uses the only one left available to it.

Quoted 5 lines

Quoted 5 lines

If I modify include/net/mac80211.h and force the inline function rate_supported to always return 1, this fixes the problem. However, I think this is a band aid around some other issue.

Any clues or ideas what the real issue might be here?

My guess::

When an adhoc node (call it A) merges with a second adhoc node (call it
B) there is a capability comparison.
Node A looks at the rates supported by B and says,
"I must only transmit at rates supported by B"

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

=====

There is no evidence that Minstrel is doing anything wrong.

Quoted 14 lines · On Wednesday 04 November 2009 22:01:39 Derek Smithies wrote:

Hi,

Quoted 10 lines · Christian Lamparter

Quoted 8 lines · On Wednesday 04 November 2009 02:13:49 Adam Wozniak wrote:

I have two systems under test, both Dell laptops (a Latitude D630 and an Inspiron 600m) both running Ubuntu 9.10 with the latest updates, and bleeding edge compat-wireless-2009-11-02. I'm using identical AR9170 based D-Link DWA-160 USB 802.11adapters. I'm using nuttcp to measure throughput. I'm running in ad-hoc mode. Both machines have the same ar9170 files in /lib/firmware. The machines are sitting about 5 feet apart in my office.

by the way: I forgot to ask, but which firmware do you use?
If you still have *two - stage*, then get rid of it.
Since one-stage fws contain a few fixes for most temporarily MAC/BB-hiccups.

Quoted 15 lines

Quoted 9 lines

Quoted 9 lines

I'm having occasional problems where throughput drops through the floor (0.5Mbps - 1.5Mbps). When I cat /sys/kernel/debug/ieee80211/*/stations/*/rc_stats, one of the machines lists the full set of rates, but the other only lists 1M and 54M. After a period of time, that machine drops 54M and lists only one rate (1Mbps), and the throughput listed by nuttcp drops accordingly. I assume that, for whatever reason, the rates drop off the list and minstrel uses the only one left available to it.

Quoted 6 lines

Quoted 6 lines

If I modify include/net/mac80211.h and force the inline function rate_supported to always return 1, this fixes the problem. However, I think this is a band aid around some other issue.

Any clues or ideas what the real issue might be here?

Quoted 9 lines

My guess::

When an adhoc node (call it A) merges with a second adhoc node (call it B) there is a capability comparison. Node A looks at the rates supported by B and says,
"I must only transmit at rates supported by B"

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

well, that's the thing... it sounds like something in cfg80211/mac80211 has gone wrong. Since ibss supported/basic rates IEs should always include all mandatory rates for the given band & mode. Therefore you should see the 2Mbit, 11Mbit, 6MBit, 12Mbit 24Mbit rates in rc_stats array as well.

Quoted 2 lines

=====

=====

Quoted 2 lines

There is no evidence that Minstrel is doing anything wrong.

?but no one said it was minstrel fault? And it clearly isn't.

But something OT: do you have already thoughts about _extending_ minstrel to support 802.11n MCS rates?

The current endeavor is stuck and needs a kick-start.
This is partly because of a hen-egg problem: no driver <-> no 11n rc. But it should be easy to get 11n capable hw now (e.g. Mikrotik's R52N) and
ath9k should be the perfect testing platform right now.

nbd has/had some thought about grouping rates and options (e.g SGI/40MHz) together to reduce the number of rates to improve the _search for best tp_ time. But dunno, maybe he has already something better than the proof-of-concept I wrote earlier.

Regards,
Chr

$ ls -la /lib/firmware/ar9170*

(quoted diff) +0 -3

-rw-r--r-- 1 root root 83968 2009-10-17 15:55 /lib/firmware/ar9170-1.fw
-rw-r--r-- 1 root root  3508 2009-10-17 15:55 /lib/firmware/ar9170-2.fw
-rw-r--r-- 1 root root 15960 2009-10-17 15:55 /lib/firmware/ar9170.fw

It is unclear to me which are actually used.

Quoted 7 lines · Adam Wozniak

$ ls -la /lib/firmware/ar9170*

(quoted diff) +0 -3

-rw-r--r-- 1 root root 83968 2009-10-17 15:55 /lib/firmware/ar9170-1.fw
-rw-r--r-- 1 root root  3508 2009-10-17 15:55 /lib/firmware/ar9170-2.fw
-rw-r--r-- 1 root root 15960 2009-10-17 15:55 /lib/firmware/ar9170.fw

It is unclear to me which are actually used.

By default the ar9170.fw is tried first, if that fails then the others are tried. The 2-stage firmware will not be tried if your device cannot handle it but right now only the AVM Fritz devices can't handle the 2-stage firmware.

Christian should we just remove 2-stage fw support?

Luis

My guess from this is that I'm using the single stage:

# dmesg | grep ar9170
[  273.608189] usb 2-1: firmware: requesting ar9170.fw
[  274.019650] Registered led device: ar9170-phy1::tx
[  274.019698] Registered led device: ar9170-phy1::assoc
[  274.019751] usbcore: registered new interface driver ar9170usb

Quoted 6 lines · Adam Wozniak

$ ls -la /lib/firmware/ar9170*

(quoted diff) +0 -3

-rw-r--r-- 1 root root 83968 2009-10-17 15:55 /lib/firmware/ar9170-1.fw
-rw-r--r-- 1 root root  3508 2009-10-17 15:55 /lib/firmware/ar9170-2.fw
-rw-r--r-- 1 root root 15960 2009-10-17 15:55 /lib/firmware/ar9170.fw

Quoted 1 line

It is unclear to me which are actually used.

By default the ar9170.fw is tried first, if that fails then the others are tried. The 2-stage firmware will not be tried if your device cannot handle it but right now only the AVM Fritz devices can't handle the 2-stage firmware.

Christian should we just remove 2-stage fw support?

Quoted 14 lines · On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:

Quoted 7 lines · Adam Wozniak

$ ls -la /lib/firmware/ar9170*

(quoted diff) +0 -3

-rw-r--r-- 1 root root 83968 2009-10-17 15:55 /lib/firmware/ar9170-1.fw
-rw-r--r-- 1 root root  3508 2009-10-17 15:55 /lib/firmware/ar9170-2.fw
-rw-r--r-- 1 root root 15960 2009-10-17 15:55 /lib/firmware/ar9170.fw

It is unclear to me which are actually used.

By default the ar9170.fw is tried first, if that fails then the others are tried. The 2-stage firmware will not be tried if your device cannot handle it but right now only the AVM Fritz devices can't handle the 2-stage firmware.

Quoted 1 line

Christian should we just remove 2-stage fw support?

I've moved the two-stage fw into the legacy section some time ago :) Maybe a add printk?

Quoted 15 lines · Christian Lamparter

Quoted 14 lines · On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:

Quoted 7 lines · Adam Wozniak

$ ls -la /lib/firmware/ar9170*

(quoted diff) +0 -3

-rw-r--r-- 1 root root 83968 2009-10-17 15:55 /lib/firmware/ar9170-1.fw
-rw-r--r-- 1 root root  3508 2009-10-17 15:55 /lib/firmware/ar9170-2.fw
-rw-r--r-- 1 root root 15960 2009-10-17 15:55 /lib/firmware/ar9170.fw

It is unclear to me which are actually used.

By default the ar9170.fw is tried first, if that fails then the others are tried. The 2-stage firmware will not be tried if your device cannot handle it but right now only the AVM Fritz devices can't handle the 2-stage firmware.

Quoted 4 lines

Quoted 1 line

Christian should we just remove 2-stage fw support?

I've moved the two-stage fw into the legacy section some time ago :) Maybe a add printk?

Oh sorry didn't notice, I meant complete removal of its support on the driver though :)

Quoted 16 lines · On Wednesday 04 November 2009 23:20:07 Luis R. Rodriguez wrote:

Quoted 15 lines · Christian Lamparter

Quoted 14 lines · On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:

Quoted 7 lines · Adam Wozniak

$ ls -la /lib/firmware/ar9170*

(quoted diff) +0 -3

-rw-r--r-- 1 root root 83968 2009-10-17 15:55 /lib/firmware/ar9170-1.fw
-rw-r--r-- 1 root root  3508 2009-10-17 15:55 /lib/firmware/ar9170-2.fw
-rw-r--r-- 1 root root 15960 2009-10-17 15:55 /lib/firmware/ar9170.fw

It is unclear to me which are actually used.

By default the ar9170.fw is tried first, if that fails then the others are tried. The 2-stage firmware will not be tried if your device cannot handle it but right now only the AVM Fritz devices can't handle the 2-stage firmware.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Christian should we just remove 2-stage fw support?

I've moved the two-stage fw into the legacy section some time ago :) Maybe a add printk?

Oh sorry didn't notice, I meant complete removal of its support on the driver though :)

Oh, I've no problem removing two-stage fw support.

But then, I don't know much about usability :-D and I've enough of people banging their heads against each other.

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

Reading through the 802.11 spec, it appears to me that "Supported rates" (and "Extended Supported Rates" when number of rates > 8) is REQUIRED for all management frames except authentication, deauthentication, and action frames. (IEEE 802.11-2007, 7.2.3)

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

Looking at mac80211/tx.c ieee80211_beacon_get_tim, it is not clear to me how (or if) this rate information is being set for ad-hoc beacons.

Quoted 2 lines · Derek Smithies

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

Hi,

Quoted 1 line

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

it was mostly conjecture. I had done some work with an earlier pull of compat wireless in adhoc. the rates tried by minstrel were bad - it was stuck at the slowest rate.

I too had found that the fix was to adjust the array so that all rates were marked as supported.

Derek

Quoted 12 lines · Adam Wozniak

Reading through the 802.11 spec, it appears to me that "Supported rates" (and "Extended Supported Rates" when number of rates > 8) is REQUIRED for all management frames except authentication, deauthentication, and action frames. (IEEE 802.11-2007, 7.2.3)

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

Looking at mac80211/tx.c ieee80211_beacon_get_tim, it is not clear to me how (or if) this rate information is being set for ad-hoc beacons.

Quoted 2 lines · Derek Smithies

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

Is it possible this is the problem? (note supp_rates is used at the bottom of the function, outside the "if")

Is it possible this is the problem? (note supp_rates is used at the bottom of the function, outside the "if")

*** a/net/mac80211/ibss.c 2009-11-02 09:11:36.000000000 -0800
--- b/net/mac80211/ibss.c 2009-11-10 16:31:46.291563951 -0800
***************
*** 246,254 ****

    if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
        return;

    if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
        memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {

(quoted diff) +2 -2

-         supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
        rcu_read_lock();
 
--- 246,255 ----
    if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
        return;
 
+     supp_rates = ieee80211_sta_get_rates(local, elems, band);
+
    if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
        memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
 
        rcu_read_lock();

Quoted 24 lines · Derek Smithies

Hi,

Quoted 1 line

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

it was mostly conjecture. I had done some work with an earlier pull of compat wireless in adhoc. the rates tried by minstrel were bad - it was stuck at the slowest rate.

I too had found that the fix was to adjust the array so that all rates were marked as supported.

Derek

Quoted 12 lines · Adam Wozniak

Reading through the 802.11 spec, it appears to me that "Supported rates" (and "Extended Supported Rates" when number of rates > 8) is REQUIRED for all management frames except authentication, deauthentication, and action frames. (IEEE 802.11-2007, 7.2.3)

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

Looking at mac80211/tx.c ieee80211_beacon_get_tim, it is not clear to me how (or if) this rate information is being set for ad-hoc beacons.

Quoted 2 lines · Derek Smithies

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

Hi,

Quoted 1 line

Is it possible this is the problem? (note supp_rates is used at the bottom of the function, outside the "if")

Could be. Did it fix your problem?

Derek.

Quoted 56 lines · Adam Wozniak

Is it possible this is the problem? (note supp_rates is used at the bottom of the function, outside the "if")

*** a/net/mac80211/ibss.c 2009-11-02 09:11:36.000000000 -0800
--- b/net/mac80211/ibss.c 2009-11-10 16:31:46.291563951 -0800
***************
*** 246,254 ****

    if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
        return;

    if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
        memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {

(quoted diff) +2 -2

-         supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
        rcu_read_lock();
 
--- 246,255 ----
    if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
        return;
 
+     supp_rates = ieee80211_sta_get_rates(local, elems, band);
+
    if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
        memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
 
        rcu_read_lock();

Quoted 24 lines · Derek Smithies

Hi,

Quoted 1 line

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

it was mostly conjecture. I had done some work with an earlier pull of compat wireless in adhoc. the rates tried by minstrel were bad - it was stuck at the slowest rate.

I too had found that the fix was to adjust the array so that all rates were marked as supported.

Derek

Quoted 12 lines · Adam Wozniak

Reading through the 802.11 spec, it appears to me that "Supported rates" (and "Extended Supported Rates" when number of rates > 8) is REQUIRED for all management frames except authentication, deauthentication, and action frames. (IEEE 802.11-2007, 7.2.3)

Do you know which frames in the mac80211 code are missing this required information? Or was that conjecture?

Looking at mac80211/tx.c ieee80211_beacon_get_tim, it is not clear to me how (or if) this rate information is being set for ad-hoc beacons.

Quoted 2 lines · Derek Smithies

Some management frames don't contain a full report of the rates supported by the sender.
My view is that node A (in this example) is incorrectly determining that B only supports the 1mb/sec rate. Consequently, node A fills the rate_supported array with one rate - 1mb/sec.

I was hoping for more of an "ah-ha!" response. =)

It worked well initially, but when I let it run overnight it fell back into that same failure mode.

Quoted 5 lines · On Thursday 12 November 2009 20:43:22 Adam Wozniak wrote:

I was hoping for more of an "ah-ha!" response. =)

It worked well initially, but when I let it run overnight it fell back into that same failure mode.

great... :\

what about this patch?
---

net/mac80211/ibss.c +4 -3

index fbffce9..bde89f7 100644
@@ -250,6 +250,9 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 	    memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
 		supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
+		/* make sure mandatory rates are always added */
+		supp_rates |= ieee80211_mandatory_rates(local, band);
+
 		rcu_read_lock();
 
 		sta = sta_info_get(local, mgmt->sa);

@@ -257,9 +260,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 			u32 prev_rates;
 
 			prev_rates = sta->sta.supp_rates[band];
-			/* make sure mandatory rates are always added */
-			sta->sta.supp_rates[band] = supp_rates |
-				ieee80211_mandatory_rates(local, band);
+			sta->sta.supp_rates[band] = supp_rates;
 
 #ifdef CONFIG_MAC80211_IBSS_DEBUG
 			if (sta->sta.supp_rates[band] != prev_rates)

I see what you're doing there. That didn't quite work, but I'm fairly confident this one will. I'm running my long term test now. Note the added call to rate_control_init() when the rate is updated. This *should* be rate_control_rate_update, but it doesn't look to me like that method is implemented in minstrel or the PID code.

*** compat-wireless-2009-11-09/net/mac80211/ibss.c 2009-11-08 21:15:06.000000000 -0800
--- compat-wireless-2009-11-09b/net/mac80211/ibss.c 2009-11-12
14:29:16.308550923 -0800
***************
*** 246,254 ****

     if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
         return;

     if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
         memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {

(quoted diff) +5 -2

-         supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
         rcu_read_lock();
 
--- 246,258 ----
     if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
         return;
 
+     supp_rates = ieee80211_sta_get_rates(local, elems, band);
+
+     /* make sure mandatory rates are always added */
+     supp_rates = supp_rates |= ieee80211_mandatory_rates(local, band);
+
     if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
         memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
 
         rcu_read_lock();

***************
*** 257,268 ****

             u32 prev_rates;

             prev_rates = sta->sta.supp_rates[band];

! /* make sure mandatory rates are always added */
! sta->sta.supp_rates[band] = supp_rates |
! ieee80211_mandatory_rates(local, band);

#ifdef CONFIG_MAC80211_IBSS_DEBUG
! if (sta->sta.supp_rates[band] != prev_rates)

                 printk(KERN_DEBUG "%s: updated supp_rates set "
                     "for %pM based on beacon info (0x%llx | "
                     "0x%llx -> 0x%llx)\n",
--- 261,270 ----
             u32 prev_rates;

             prev_rates = sta->sta.supp_rates[band];

! sta->sta.supp_rates[band] = supp_rates;

#ifdef CONFIG_MAC80211_IBSS_DEBUG

!             if (sta->sta.supp_rates[band] != prev_rates) {
                 printk(KERN_DEBUG "%s: updated supp_rates set "
                     "for %pM based on beacon info (0x%llx | "
                     "0x%llx -> 0x%llx)\n",

***************
*** 271,276 ****
--- 273,284 ----
(unsigned long long) prev_rates, (unsigned long long) supp_rates, (unsigned long long) sta->sta.supp_rates[band]);

(quoted diff) +6 -0

+
+                 /* TODO: implement rate_update in minstrel/pid,
+                  * then change this to rate_control_rate_update.
+                  */
+                 rate_control_rate_init(sta);
+             }
 #endif
         } else
             ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, 

supp_rates);

Quoted 40 lines · Christian Lamparter

Quoted 5 lines · On Thursday 12 November 2009 20:43:22 Adam Wozniak wrote:

I was hoping for more of an "ah-ha!" response. =)

It worked well initially, but when I let it run overnight it fell back into that same failure mode.

great... :\

what about this patch?
---

net/mac80211/ibss.c +3 -0

index fbffce9..bde89f7 100644
@@ -250,6 +250,9 @@ static void ieee80211_rx_bss_info(struct 
 ieee80211_sub_if_data *sdata,
         memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
         supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
+        /* make sure mandatory rates are always added */
+        supp_rates |= ieee80211_mandatory_rates(local, band);
+
         rcu_read_lock();
 

         sta = sta_info_get(local, mgmt->sa);

net/mac80211/ibss.c +1 -3

@@ -257,9 +260,7 @@ static void ieee80211_rx_bss_info(struct 
 ieee80211_sub_if_data *sdata,
             u32 prev_rates;
 
             prev_rates = sta->sta.supp_rates[band];
-            /* make sure mandatory rates are always added */
-            sta->sta.supp_rates[band] = supp_rates |
-                ieee80211_mandatory_rates(local, band);
+            sta->sta.supp_rates[band] = supp_rates;
 
 #ifdef CONFIG_MAC80211_IBSS_DEBUG

             if (sta->sta.supp_rates[band] != prev_rates)

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Whoops, patch was slightly incorrect. This is better:

*** compat-wireless-2009-11-09/net/mac80211/ibss.c 2009-11-08
21:15:06.000000000 -0800
--- compat-wireless-2009-11-09b/net/mac80211/ibss.c 2009-11-12
14:39:12.391545084 -0800
***************
*** 246,254 ****

it would help if you used diff -u
johannes

I've traced it down to this bit in rx.c in prepare_for_handlers():

        case NL80211_IFTYPE_ADHOC:
                [ stuff deleted ]
                } else if (!rx->sta) {
                        int rate_idx;
                        if (rx->status->flag & RX_FLAG_HT)
                                rate_idx = 0; /* TODO: HT rates */
                        else
                                rate_idx = rx->status->rate_idx;

                        rx->sta = ieee80211_ibss_add_sta(sdata, bssid, 
hdr->addr2,
                                BIT(rate_idx));
                }
                break;

I don't think this is right. I know the issue is here, because if I change to "BIT(rate_idx) | 0xfff" the problem corrects. Either we need to (a) set it properly here or (b) make sure something else happens before or after. I'm not sure we have enough context here to do (a).

Quoted 17 lines · Adam Wozniak

I've traced it down to this bit in rx.c in prepare_for_handlers():

        case NL80211_IFTYPE_ADHOC:
                [ stuff deleted ]
                } else if (!rx->sta) {
                        int rate_idx;
                        if (rx->status->flag & RX_FLAG_HT)
                                rate_idx = 0; /* TODO: HT rates */
                        else
                                rate_idx = rx->status->rate_idx;

                        rx->sta = ieee80211_ibss_add_sta(sdata, bssid, 
hdr->addr2,
                                BIT(rate_idx));
                }
                break;

Quoted 2 lines

I don't think this is right. I know the issue is here, because if I change to "BIT(rate_idx) | 0xfff" the problem corrects. Either we need to (a) set it properly here or (b) make sure something else happens before or after. I'm not sure we have enough context here to do (a).

Hmm. This BIT(..) was basically here to ensure we have at least a single good rate absent other information. If we do not receive a probe or beacon frame from that peer at least once, but add it to our IBSS only due to a single received data frame, we add the bitrate that this frame was transmitted at so we have one rate that we know it can transmit (and presumably also receive).

What should happen is that once it starts beaconing we pick up a beacon and extend our set of known good rates.

johannes

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

Quoted 2 lines · Adam Wozniak

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

No, if they all behave correctly beaconing will be distributed.

johannes

This assumption seems too stoichastic. Reading 802.11-2007 section 11.1.2.2, it doesn't seem that we're guaranteed to always receive beacons from all stations. Stations will cancel their pending beacon transmission if they receive a beacon before their random delay times out. In the extreme case where the number of stations is very large, it seems possible that you may never hear beacons for some stations.

This assumption seems too stoichastic. Reading 802.11-2007 section 11.1.2.2, it doesn't seem that we're guaranteed to always receive beacons from all stations. Stations will cancel their pending beacon transmission if they receive a beacon before their random delay times out. In the extreme case where the number of stations is very large, it seems possible that you may never hear beacons for some stations.

Quoted 8 lines · Johannes Berg

Quoted 1 line · Adam Wozniak

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

No, if they all behave correctly beaconing will be distributed.

johannes

Hi,
Statistics is a wonderful thing.
Every node is required to send a beacon at time
X+r, X+x+r, X+2x+r, X+3x+r, ......

All nodes are agreed on the value of x (which is the beacon interval). All nodes are agreed on the value of X
r is a random value, and is (from memory) 20 slots long.

given this, all nodes work (to borrow an analogy from music) in time, or beat in sync with each other.

Now, if a node hears a beacon on its BSSID inside that r period, the node will not transmit a beacon. This way, for a 20 node network in a room, you should only get 1 (or sometimes 2) beacons transmitted every beacon interval.

If we assume that every node correctly attempts to send a beacon somewhere in that period r, and that somewhere is randomly distributed, then we will hear a beacon from most nodes, which is good enough.

Consider the case of three nodes, A, B, C.

A and B are turned on, and create an Adhoc network. A and B agree on
a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID
and so start sending a beacon. Inside this beacon is BSSID. The BSSID is a random value. The particular random value used implies acceptance of a,b,c,d. Look at the name. Basic Service Set ID. The basic service set is the collection of those values a,b,c,d.

Now, node C is turned on. A is positioned such that A&C cannot communicate. However, B can communicate with A&C.

C is turned on, detects the presence of B, likes B's beacons, and agrees on all the settings in B's beacons. In other words, C likes and agrees with a,b,c,d. So C starts sending beacons with the same BSSID as B.
At this point, it does not matter that A cannot set C's beacons. A and C have agreed on the BSSID.

Change the story a little bit.
In this locality, there is often a burst of 1ms of noise every 2ms. This means that most beacons are shotdown. However, data packets at 54Mbit/sec get through.
A&B saw each others beacons and agreed on a BSSID. C was turned on,
and agreed with the BSSID of B.

C sends out data packets, with the BSSID of B. A sees the datapackets of C. Since the datapackets of C have A's BSSID, A has to accept them.

Now, you see where this is all going. What is the meaning of a beacon containing a BSSID of all zero ? Further, you see that all nodes do need to send a beacon. This makes node discovery a little easier. Even though A and C cannot see each others beacons, they should still communicate as they have the same agreed on BSSID.

Derek.

Quoted 14 lines · Adam Wozniak

This assumption seems too stoichastic. Reading 802.11-2007 section 11.1.2.2, it doesn't seem that we're guaranteed to always receive beacons from all stations. Stations will cancel their pending beacon transmission if they receive a beacon before their random delay times out. In the extreme case where the number of stations is very large, it seems possible that you may never hear beacons for some stations.

Quoted 8 lines · Johannes Berg

Quoted 1 line · Adam Wozniak

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

No, if they all behave correctly beaconing will be distributed.

johannes

Quoted 4 lines

a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID

In your example where A&C can pass data, but not hear each other's beacons, how is rate information passed between them? ProbeReq/Resp ?

Quoted 61 lines · Derek Smithies

Hi,
Statistics is a wonderful thing.
Every node is required to send a beacon at time
X+r, X+x+r, X+2x+r, X+3x+r, ......

All nodes are agreed on the value of x (which is the beacon interval). All nodes are agreed on the value of X
r is a random value, and is (from memory) 20 slots long.

given this, all nodes work (to borrow an analogy from music) in time, or beat in sync with each other.

Now, if a node hears a beacon on its BSSID inside that r period, the node will not transmit a beacon. This way, for a 20 node network in a room, you should only get 1 (or sometimes 2) beacons transmitted every beacon interval.

If we assume that every node correctly attempts to send a beacon somewhere in that period r, and that somewhere is randomly distributed, then we will hear a beacon from most nodes, which is good enough.

Consider the case of three nodes, A, B, C.

A and B are turned on, and create an Adhoc network. A and B agree on
a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID
and so start sending a beacon. Inside this beacon is BSSID. The BSSID is a random value. The particular random value used implies acceptance of a,b,c,d. Look at the name. Basic Service Set ID. The basic service set is the collection of those values a,b,c,d.

Now, node C is turned on. A is positioned such that A&C cannot communicate. However, B can communicate with A&C.

C is turned on, detects the presence of B, likes B's beacons, and agrees on all the settings in B's beacons. In other words, C likes and agrees with a,b,c,d. So C starts sending beacons with the same BSSID as B.
At this point, it does not matter that A cannot set C's beacons. A and C have agreed on the BSSID.

Change the story a little bit.
In this locality, there is often a burst of 1ms of noise every 2ms. This means that most beacons are shotdown. However, data packets at 54Mbit/sec get through.
A&B saw each others beacons and agreed on a BSSID. C was turned on, and
agreed with the BSSID of B.

C sends out data packets, with the BSSID of B. A sees the datapackets of C. Since the datapackets of C have A's BSSID, A has to accept them.

Now, you see where this is all going. What is the meaning of a beacon containing a BSSID of all zero ? Further, you see that all nodes do need to send a beacon. This makes node discovery a little easier. Even though A and C cannot see each others beacons, they should still communicate as they have the same agreed on BSSID.

Derek.

Quoted 14 lines · Adam Wozniak

This assumption seems too stoichastic. Reading 802.11-2007 section 11.1.2.2, it doesn't seem that we're guaranteed to always receive beacons from all stations. Stations will cancel their pending beacon transmission if they receive a beacon before their random delay times out. In the extreme case where the number of stations is very large, it seems possible that you may never hear beacons for some stations.

Quoted 8 lines · Johannes Berg

Quoted 1 line · Adam Wozniak

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

No, if they all behave correctly beaconing will be distributed.

johannes

Hi,

Referring back to my letter below, you see that when A&B agree on their BSSID, this implies that A&B have agreed on:

Quoted 4 lines

Quoted 4 lines

a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID

When B&C start talking, and C adopts the same BSSID as B, then we have (by inference) that C has agreed to the same rates as A. There is no need to pass rate information between C and A.

===========================
Ok, now here is a silly example. Suppose C supports bitrates b4,b5 and b6.

B does not support bitrates b4,b5 and b6.

Further, A does support bitrates b4,b5,b6

This could be that B only does 1, 2, 5.5 and 11MBits/sec. But A&C do everything up to 54Mbit/sec.

B has caused a problem cause he is deficient (not doing G-Rates)

In one view, B should report that he has all the rates of A. B would then hope that A's rate control algorithm will detect that B cannot do the G-rates. Minstrel will do this fine. Stepup-Stepdown rate algorithms will struggle here if their table is constructed in the wrong order.

For this network, the BSSID defines the basic service set. you cannot have nodes on the same BSSID who report as handling different rate sets.

Derek.

Quoted 66 lines · Adam Wozniak

In your example where A&C can pass data, but not hear each other's beacons, how is rate information passed between them? ProbeReq/Resp ?

Quoted 60 lines · Derek Smithies

Hi,
Statistics is a wonderful thing.
Every node is required to send a beacon at time
X+r, X+x+r, X+2x+r, X+3x+r, ......

All nodes are agreed on the value of x (which is the beacon interval). All nodes are agreed on the value of X
r is a random value, and is (from memory) 20 slots long.

given this, all nodes work (to borrow an analogy from music) in time, or beat in sync with each other.

Now, if a node hears a beacon on its BSSID inside that r period, the node will not transmit a beacon. This way, for a 20 node network in a room, you should only get 1 (or sometimes 2) beacons transmitted every beacon interval.

If we assume that every node correctly attempts to send a beacon somewhere in that period r, and that somewhere is randomly distributed, then we will hear a beacon from most nodes, which is good enough.

Consider the case of three nodes, A, B, C.

A and B are turned on, and create an Adhoc network. A and B agree on
a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID
and so start sending a beacon. Inside this beacon is BSSID. The BSSID is a random value. The particular random value used implies acceptance of a,b,c,d. Look at the name. Basic Service Set ID. The basic service set is the collection of those values a,b,c,d.

Now, node C is turned on. A is positioned such that A&C cannot communicate. However, B can communicate with A&C.

C is turned on, detects the presence of B, likes B's beacons, and agrees on all the settings in B's beacons. In other words, C likes and agrees with a,b,c,d. So C starts sending beacons with the same BSSID as B.
At this point, it does not matter that A cannot set C's beacons. A and C have agreed on the BSSID.

Change the story a little bit.
In this locality, there is often a burst of 1ms of noise every 2ms. This means that most beacons are shotdown. However, data packets at 54Mbit/sec get through. A&B saw each others beacons and agreed on a BSSID. C was turned on, and agreed with the BSSID of B.

C sends out data packets, with the BSSID of B. A sees the datapackets of C. Since the datapackets of C have A's BSSID, A has to accept them.

Now, you see where this is all going. What is the meaning of a beacon containing a BSSID of all zero ? Further, you see that all nodes do need to send a beacon. This makes node discovery a little easier. Even though A and C cannot see each others beacons, they should still communicate as they have the same agreed on BSSID.

Derek.

Quoted 14 lines · Adam Wozniak

This assumption seems too stoichastic. Reading 802.11-2007 section 11.1.2.2, it doesn't seem that we're guaranteed to always receive beacons from all stations. Stations will cancel their pending beacon transmission if they receive a beacon before their random delay times out. In the extreme case where the number of stations is very large, it seems possible that you may never hear beacons for some stations.

Quoted 8 lines · Johannes Berg

Quoted 1 line · Adam Wozniak

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

No, if they all behave correctly beaconing will be distributed.

johannes

It seems like the code I pointed out earlier in rx.c is certainly wrong then; bitrates used in absence of other information should not be guessed based on the received rate, but should come instead from the BSSID. Correct?

Although, once you start these silly examples, it seems to me like the best thing to do is just assume the receiver can handle anything and let minstrel sort out what works and what doesn't. This has obvious problems for PID and similar rate algorithms.

It seems like the code I pointed out earlier in rx.c is certainly wrong then; bitrates used in absence of other information should not be guessed based on the received rate, but should come instead from the BSSID. Correct?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID

Although, once you start these silly examples, it seems to me like the best thing to do is just assume the receiver can handle anything and let minstrel sort out what works and what doesn't. This has obvious problems for PID and similar rate algorithms.

This has obvious problems for PID and similar rate algorithms.

Quoted 103 lines · Derek Smithies

Hi,

Referring back to my letter below, you see that when A&B agree on their BSSID, this implies that A&B have agreed on:

Quoted 4 lines

Quoted 4 lines

a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID

When B&C start talking, and C adopts the same BSSID as B, then we have (by inference) that C has agreed to the same rates as A. There is no need to pass rate information between C and A.

===========================
Ok, now here is a silly example. Suppose C supports bitrates b4,b5 and b6.

B does not support bitrates b4,b5 and b6.

Further, A does support bitrates b4,b5,b6

This could be that B only does 1, 2, 5.5 and 11MBits/sec. But A&C do everything up to 54Mbit/sec.

B has caused a problem cause he is deficient (not doing G-Rates)

In one view, B should report that he has all the rates of A. B would then hope that A's rate control algorithm will detect that B cannot do the G-rates. Minstrel will do this fine. Stepup-Stepdown rate algorithms will struggle here if their table is constructed in the wrong order.

For this network, the BSSID defines the basic service set. you cannot have nodes on the same BSSID who report as handling different rate sets.

Derek.

Quoted 66 lines · Adam Wozniak

In your example where A&C can pass data, but not hear each other's beacons, how is rate information passed between them? ProbeReq/Resp ?

Quoted 60 lines · Derek Smithies

Hi,
Statistics is a wonderful thing.
Every node is required to send a beacon at time
X+r, X+x+r, X+2x+r, X+3x+r, ......

All nodes are agreed on the value of x (which is the beacon interval). All nodes are agreed on the value of X
r is a random value, and is (from memory) 20 slots long.

given this, all nodes work (to borrow an analogy from music) in time, or beat in sync with each other.

Now, if a node hears a beacon on its BSSID inside that r period, the node will not transmit a beacon. This way, for a 20 node network in a room, you should only get 1 (or sometimes 2) beacons transmitted every beacon interval.

If we assume that every node correctly attempts to send a beacon somewhere in that period r, and that somewhere is randomly distributed, then we will hear a beacon from most nodes, which is good enough.

Consider the case of three nodes, A, B, C.

A and B are turned on, and create an Adhoc network. A and B agree on
a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID
and so start sending a beacon. Inside this beacon is BSSID. The BSSID is a random value. The particular random value used implies acceptance of a,b,c,d. Look at the name. Basic Service Set ID. The basic service set is the collection of those values a,b,c,d.

Now, node C is turned on. A is positioned such that A&C cannot communicate. However, B can communicate with A&C.

C is turned on, detects the presence of B, likes B's beacons, and agrees on all the settings in B's beacons. In other words, C likes and agrees with a,b,c,d. So C starts sending beacons with the same BSSID as B.
At this point, it does not matter that A cannot set C's beacons. A and C have agreed on the BSSID.

Change the story a little bit.
In this locality, there is often a burst of 1ms of noise every 2ms. This means that most beacons are shotdown. However, data packets at 54Mbit/sec get through. A&B saw each others beacons and agreed on a BSSID. C was turned on, and agreed with the BSSID of B.

C sends out data packets, with the BSSID of B. A sees the datapackets of C. Since the datapackets of C have A's BSSID, A has to accept them.

Now, you see where this is all going. What is the meaning of a beacon containing a BSSID of all zero ? Further, you see that all nodes do need to send a beacon. This makes node discovery a little easier. Even though A and C cannot see each others beacons, they should still communicate as they have the same agreed on BSSID.

Derek.

Quoted 14 lines · Adam Wozniak

This assumption seems too stoichastic. Reading 802.11-2007 section 11.1.2.2, it doesn't seem that we're guaranteed to always receive beacons from all stations. Stations will cancel their pending beacon transmission if they receive a beacon before their random delay times out. In the extreme case where the number of stations is very large, it seems possible that you may never hear beacons for some stations.

Quoted 8 lines · Johannes Berg

Quoted 1 line · Adam Wozniak

If we have only three stations in an ad-hoc network, where all three can hear the other two, only one of them should be beaconing, correct?

No, if they all behave correctly beaconing will be distributed.

johannes

Yes.
that is what the BSSID is - the definition of the basis service set. To
use a particular BSSID is an implicit acceptance of the

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

a)supported rates
b)the value of X, the value of x
c)the channel
d)the ESSSID

from an existing network.

No, point a) is incorrect -- it is the _basic_ rate set only, not the entire rate set. Hence not knowing which rates a station actually supports.

In either case, having minstrel use all rates doesn't seem like it should hurt much. This is the patch I'm running with now, and it seems to work. The changes in ibss.c make sure supp_rates is initialized properly, and that rate control is called when a change occurs. If other stuff is working right (?) this should help PID and other rate control algorithms. The changes in rc80211_minstrel.c make it ignore the supported rates set and just try everything.

diff -upr compat-wireless-2009-11-17/net/mac80211/ibss.c compat-wireless-2009-11-17a/net/mac80211/ibss.c
--- compat-wireless-2009-11-17/net/mac80211/ibss.c 2009-11-16
21:17:21.000000000 -0800
+++ compat-wireless-2009-11-17a/net/mac80211/ibss.c 2009-11-17
09:01:54.287720290 -0800

net/mac80211/ibss.c 2009-11-17 09:01:54.287720290 -0800 renamed from net/mac80211/ibss.c 2009-11-16 21:17:21.000000000 -0800 +18 -5

@@ -246,9 +246,13 @@ static void ieee80211_rx_bss_info(struct
    if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
        return;
 
+    supp_rates = ieee80211_sta_get_rates(local, elems, band);
+
+    /* make sure mandatory rates are always added */
+    supp_rates |= ieee80211_mandatory_rates(local, band);
+
    if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
        memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
-        supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
        rcu_read_lock();
 

@@ -257,12 +261,10 @@ static void ieee80211_rx_bss_info(struct
            u32 prev_rates;
 
            prev_rates = sta->sta.supp_rates[band];
-            /* make sure mandatory rates are always added */
-            sta->sta.supp_rates[band] = supp_rates |
-                ieee80211_mandatory_rates(local, band);
+            sta->sta.supp_rates[band] = supp_rates;
 
+            if (sta->sta.supp_rates[band] != prev_rates) {
 #ifdef CONFIG_MAC80211_IBSS_DEBUG
-            if (sta->sta.supp_rates[band] != prev_rates)
                printk(KERN_DEBUG "%s: updated supp_rates set "
                    "for %pM based on beacon info (0x%llx | "
                    "0x%llx -> 0x%llx)\n",

@@ -272,6 +274,8 @@ static void ieee80211_rx_bss_info(struct
                    (unsigned long long) supp_rates,
                    (unsigned long long) sta->sta.supp_rates[band]);
 #endif
+                rate_control_rate_init(sta);
+            }
        } else
            ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, 
 supp_rates);

@@ -415,6 +419,15 @@ struct sta_info *ieee80211_ibss_add_sta(
    sta->sta.supp_rates[band] = supp_rates |
            ieee80211_mandatory_rates(local, band);
 
+#ifdef CONFIG_MAC80211_IBSS_DEBUG
+    printk(KERN_DEBUG "%s: initialized supp_rates set "
+        "for %pM (0x%llx) (band %d)\n",
+        sdata->dev->name,
+        sta->sta.addr,
+        (unsigned long long) sta->sta.supp_rates[band],
+        band);
+#endif
+
    rate_control_rate_init(sta);
 
    if (sta_info_insert(sta))

diff -upr compat-wireless-2009-11-17/net/mac80211/rc80211_minstrel.c compat-wireless-2009-11-17a/net/mac80211/rc80211_minstrel.c --- compat-wireless-2009-11-17/net/mac80211/rc80211_minstrel.c

2009-11-16 21:17:21.000000000 -0800
+++ compat-wireless-2009-11-17a/net/mac80211/rc80211_minstrel.c    
2009-11-17 09:02:25.544628968 -0800

net/mac80211/rc80211_minstrel.c 2009-11-17 09:02:25.544628968 -0800 renamed from net/mac80211/rc80211_minstrel.c 2009-11-16 21:17:21.000000000 -0800 +0 -2

@@ -395,8 +395,6 @@ minstrel_rate_init(void *priv, struct ie
        unsigned int tx_time_single;
        unsigned int cw = mp->cw_min;
 
-        if (!rate_supported(sta, sband->band, i))
-            continue;
        n++;
        memset(mr, 0, sizeof(*mr));

What's the next step in making sure this patch makes it into the real world?

Previously, not all code paths set supp_rates, and a rate change did not reinitialize the rate control layer.
This patch fixes those issues.

Signed-off-by Adam Wozniak <[email protected]>

---

net/mac80211/ibss.c +0 -0

index 10d1385..474f66d 100644
@@ -246,9 +246,13 @@ static void ieee80211_rx_bss_info(struct 
 ieee80211_sub_if_data *sdata,
     if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)

net/mac80211/ibss.c +3 -1

+            if (sta->sta.supp_rates[band] != prev_rates) {
 #ifdef CONFIG_MAC80211_IBSS_DEBUG
-            if (sta->sta.supp_rates[band] != prev_rates)
                 printk(KERN_DEBUG "%s: updated supp_rates set "
                     "for %pM based on beacon info (0x%llx | "
                     "0x%llx -> 0x%llx)\n",

@@ -272,6 +274,8 @@ static void ieee80211_rx_bss_info(struct 
 ieee80211_sub_if_data *sdata,
                     (unsigned long long) supp_rates,
                     (unsigned long long) sta->sta.supp_rates[band]);
 #endif
+                rate_control_rate_init(sta);
+            }
         } else
             ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, 

supp_rates);

Previously, not all code paths set supp_rates, and a rate change did not reinitialize the rate control layer. This patch fixes those issues. (Hopefully not whitespace damaged this time)

Signed-off-by Adam Wozniak <[email protected]>

---

net/mac80211/ibss.c +7 -5

index 10d1385..474f66d 100644
@@ -246,9 +246,13 @@ static void ieee80211_rx_bss_info(struct
 ieee80211_sub_if_data *sdata,
 	if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
 		return;
 
+	supp_rates = ieee80211_sta_get_rates(local, elems, band);
+
+	/* make sure mandatory rates are always added */
+	supp_rates |= ieee80211_mandatory_rates(local, band);
+
 	if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
 	    memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
-		supp_rates = ieee80211_sta_get_rates(local, elems, band);
 
 		rcu_read_lock();

@@ -257,12 +261,10 @@ static void ieee80211_rx_bss_info(struct
 ieee80211_sub_if_data *sdata,
 			u32 prev_rates;
 
 			prev_rates = sta->sta.supp_rates[band];
-			/* make sure mandatory rates are always added */
-			sta->sta.supp_rates[band] = supp_rates |
-				ieee80211_mandatory_rates(local, band);
+			sta->sta.supp_rates[band] = supp_rates;
 
+			if (sta->sta.supp_rates[band] != prev_rates) {
 #ifdef CONFIG_MAC80211_IBSS_DEBUG
-			if (sta->sta.supp_rates[band] != prev_rates)
 				printk(KERN_DEBUG "%s: updated supp_rates set "
 				    "for %pM based on beacon info (0x%llx | "

net/mac80211/ibss.c +11 -0

 				    "0x%llx -> 0x%llx)\n",

@@ -272,6 +274,8 @@ static void ieee80211_rx_bss_info(struct
 ieee80211_sub_if_data *sdata,
 				    (unsigned long long) supp_rates,
 				    (unsigned long long) sta->sta.supp_rates[band]);
 #endif
+				rate_control_rate_init(sta);
+			}
 		} else
 			ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, supp_rates);

@@ -415,6 +419,15 @@ struct sta_info *ieee80211_ibss_add_sta(struct
 ieee80211_sub_if_data *sdata,
 	sta->sta.supp_rates[band] = supp_rates |
 			ieee80211_mandatory_rates(local, band);
 
+#ifdef CONFIG_MAC80211_IBSS_DEBUG
+	printk(KERN_DEBUG "%s: initialized supp_rates set "
+		"for %pM (0x%llx) (band %d)\n",
+		sdata->dev->name,
+		sta->sta.addr,
+		(unsigned long long) sta->sta.supp_rates[band],
+		band);
+#endif
+
 	rate_control_rate_init(sta);
 

if (sta_info_insert(sta))

In Ad-Hoc mode, it is possible for minstrel to be initialized without complete rate information. In these cases, throughput suffers because not all rates are tried. This patch tells minstrel to try all rates. This won't cause problems, because unsupported rates will simply fail, causing minstrel to set their probability to 0%.

Signed-off-by Adam Wozniak <[email protected]>

---

net/mac80211/rc80211_minstrel.c +0 -2

index 6e5d68b..3a8fe30 100644
@@ -395,8 +395,6 @@ minstrel_rate_init(void *priv, struct 
 ieee80211_supported_band *sband,
         unsigned int tx_time_single;
         unsigned int cw = mp->cw_min;
 
-        if (!rate_supported(sta, sband->band, i))
-            continue;
         n++;
         memset(mr, 0, sizeof(*mr));

Both your patches are completely whitespace damaged.

Quoted 1 line · Johannes Berg

Both your patches are completely whitespace damaged.

Can you explain what you mean by that?

I really don't like this patch anyhow though, it's at best a hackish workaround around an issue that should just be fixed instead.

Will take another look at the other patch tomorrow.

I do not believe 1/2 was a hack. It really was possible to get to the end of that function and use that variable without it being assigned the correct value. Also, it is important to reinitialize the rate control layer when the list of usable rates changes.

Quoted 2 lines · Adam Wozniak

I do not believe 1/2 was a hack. It really was possible to get to the end of that function and use that variable without it being assigned the correct value. Also, it is important to reinitialize the rate control layer when the list of usable rates changes.

Ah, indeed. But I still don't think the rate control _init() function should be called again.

Quoted 10 lines · Johannes Berg

Quoted 5 lines · Adam Wozniak

I do not believe 1/2 was a hack. It really was possible to get to the end of that function and use that variable without it being assigned the correct value. Also, it is important to reinitialize the rate control layer when the list of usable rates changes.

Ah, indeed. But I still don't think the rate control _init() function should be called again.

My first thought was to use rate_control_ops.rate_update (via rate.h's rate_control_rate_update() function).

However, neither minstrel nor pid_algo implement it, and the only place it is actually used is from ieee80211_enable_ht() (in mlme.c) when the channel type changes.

So the only way (currently) to "update" is call _init() again.

Or you can probably call rate_exit and rate_init but I don't know how the algorithms would react if you actually ask it for a rate during that race window?

Quoted 4 lines · Johannes Berg

Or you can probably call rate_exit and rate_init but I don't know how the algorithms would react if you actually ask it for a rate during that race window?

There is no rate_exit in rate_control_ops, only init and update. There is an rc80211_minstrel_exit() in rc80211_minstrel.c, but that just unregisters the module, which I don't think is what you want.

However, that reminds me that I actually wanted to get rid of rate_init() since it's mostly not useful to have it separate from alloc_sta()... oh well.

I suppose if we document clearly that rate_init maybe called multiple times and audit the existing algorithms the patch could be ok.

Quoted 7 lines · Johannes Berg

However, that reminds me that I actually wanted to get rid of rate_init() since it's mostly not useful to have it separate from alloc_sta()... oh well.

I suppose if we document clearly that rate_init maybe called multiple times and audit the existing algorithms the patch could be ok.

As far as I can tell, minstrel_rate_init does not do any allocation. The pid_algo version does not appear to do any allocation either.

Quoted 2 lines · Adam Wozniak

As far as I can tell, minstrel_rate_init does not do any allocation. The pid_algo version does not appear to do any allocation either.

There are three more -- iwlwifi (2x) and ath9k

Quoted 8 lines · Johannes Berg

Quoted 3 lines · Adam Wozniak

As far as I can tell, minstrel_rate_init does not do any allocation. The pid_algo version does not appear to do any allocation either.

There are three more -- iwlwifi (2x) and ath9k

Ugh. Why aren't these in with the mac80211 code? Do they rely on something hardware specific?

I could not find any allocations in the rate_init in any of these:

Ok, but can they all deal with calling get_rate and rate_init at the same time? I think that could happen now, no?

In Ad-Hoc mode, it is possible for minstrel to be initialized without complete rate information.
In these cases, throughput suffers because not all rates are tried. This patch tells minstrel to try all rates. This won't cause problems, because unsupported rates will simply fail, causing minstrel to set their probability to 0%. (Hopefully not whitespace damaged this time)

Signed-off-by Adam Wozniak <[email protected]>

---

net/mac80211/rc80211_minstrel.c +0 -2

index 6e5d68b..3a8fe30 100644
@@ -395,8 +395,6 @@ minstrel_rate_init(void *priv, struct
 ieee80211_supported_band *sband,
 		unsigned int tx_time_single;
 		unsigned int cw = mp->cw_min;
 
-		if (!rate_supported(sta, sband->band, i))
-			continue;
 		n++;
 		memset(mr, 0, sizeof(*mr));

I see what you're doing there.
That didn't quite work, but I'm fairly confident this one will.

Quoted 3 lines · On Thursday 12 November 2009 23:38:07 Adam Wozniak wrote:

I see what you're doing there.
That didn't quite work, but I'm fairly confident this one will.

yep, what about the attached version? ...it's slightly different...

BTW: Use "diff -up" or "diff -uprN" to create patches. (or git :) )
---

net/mac80211/ibss.c +6 -4

index fbffce9..7c6c170 100644
@@ -246,9 +246,12 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 	if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
 		return;
 
+	/* make sure mandatory rates are always added */
+	supp_rates = ieee80211_mandatory_rates(local, band);
+
 	if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
 	    memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
-		supp_rates = ieee80211_sta_get_rates(local, elems, band);
+		supp_rates |= ieee80211_sta_get_rates(local, elems, band);
 
 		rcu_read_lock();
 

@@ -257,9 +260,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 			u32 prev_rates;
 
 			prev_rates = sta->sta.supp_rates[band];
-			/* make sure mandatory rates are always added */
-			sta->sta.supp_rates[band] = supp_rates |
-				ieee80211_mandatory_rates(local, band);
+			sta->sta.supp_rates[band] = supp_rates;
 
 #ifdef CONFIG_MAC80211_IBSS_DEBUG
 			if (sta->sta.supp_rates[band] != prev_rates)

@@ -272,6 +273,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 				    (unsigned long long) supp_rates,
 				    (unsigned long long) sta->sta.supp_rates[band]);
 #endif
+			rate_control_rate_init(sta);
 		} else
 			ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, supp_rates);
 

Quoted 3 lines · On Thursday 12 November 2009 23:38:07 Adam Wozniak wrote:

I see what you're doing there.
That didn't quite work, but I'm fairly confident this one will.

yep, what about the attached version? ...it's slightly different...

BTW: Use "diff -up" or "diff -uprN" to create patches. (or git :) )
---

net/mac80211/ibss.c +6 -4

index fbffce9..7c6c170 100644
@@ -246,9 +246,12 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 	if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
 		return;
 
+	/* make sure mandatory rates are always added */
+	supp_rates = ieee80211_mandatory_rates(local, band);
+
 	if (sdata->vif.type == NL80211_IFTYPE_ADHOC && elems->supp_rates &&
 	    memcmp(mgmt->bssid, sdata->u.ibss.bssid, ETH_ALEN) == 0) {
-		supp_rates = ieee80211_sta_get_rates(local, elems, band);
+		supp_rates |= ieee80211_sta_get_rates(local, elems, band);
 
 		rcu_read_lock();
 

@@ -257,9 +260,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 			u32 prev_rates;
 
 			prev_rates = sta->sta.supp_rates[band];
-			/* make sure mandatory rates are always added */
-			sta->sta.supp_rates[band] = supp_rates |
-				ieee80211_mandatory_rates(local, band);
+			sta->sta.supp_rates[band] = supp_rates;
 
 #ifdef CONFIG_MAC80211_IBSS_DEBUG
 			if (sta->sta.supp_rates[band] != prev_rates)

@@ -272,6 +273,7 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 				    (unsigned long long) supp_rates,
 				    (unsigned long long) sta->sta.supp_rates[band]);
 #endif
+			rate_control_rate_init(sta);
 		} else
 			ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, supp_rates);