compat-wireless and minstrel

compat-wireless and minstrel

[Adam Wozniak]

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?

Re: compat-wireless and minstrel

[Christian Lamparter]

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

2. do you think you can get a capture any beacons, auth, assoc, reassoc 
   management frames form the misbehaving setup?

3. cat /sys/kernel/debug/ieee80211/*/stations/*/rc_stats perhaps?
   (yeah, I know... sort of pointless, but people have different believes
    and somehow they have the bad tendency to really _stick_ to it!)

Regards,
	Chr

Re: compat-wireless and minstrel

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 7:53 AM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> 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.
>>
>> 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)

If you can just use wireless-testing directly for debugging I
recommend that as we don't use mconf and friends for cofiguration
selection so we cannot be sure the config changes you make to enable
something are indeed agreeable by current upstream Kconfig rules. That
is, unless you know what you are doing I'd advise against using
debugging on compat-wireless.

  Luis

Re: compat-wireless and minstrel

[Derek Smithies]

Hi,
On Wed, 4 Nov 2009, Christian Lamparter wrote:

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

The original poster's report of examining
>> /sys/kernel/debug/ieee80211/*/stations/*/rc_stats
is very important, as it shows exactly what minstrel is doing.

When examining network problems, one should always check the rc_stats 
file. This tells you much about what is going on in the radio medium.

Derek.

Derek Smithies Ph.D. 
IndraNet Technologies Ltd. 
ph +64 3 365 6485 
Web: http://www.indranet-technologies.com/

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: compat-wireless and minstrel

[Christian Lamparter]

On Wednesday 04 November 2009 22:01:39 Derek Smithies wrote:
> Hi,
> On Wed, 4 Nov 2009, Christian Lamparter wrote:
> 
> > 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.

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

> 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

Re: compat-wireless and minstrel

[Adam Wozniak]

$ ls -la /lib/firmware/ar9170*
-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.  I will try removing the 
two stage firmware files and see what happens.

Christian Lamparter wrote:
> On Wednesday 04 November 2009 22:01:39 Derek Smithies wrote:
>   
>> Hi,
>> On Wed, 4 Nov 2009, Christian Lamparter wrote:
>>
>>     
>>> 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.
>
>   
>>>> 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?
>>>>         
>> 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.
>  
>   
>> =====
>>     
> =====
>
>   
>> 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
>   

Re: compat-wireless and minstrel

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 1:46 PM, Adam Wozniak <awozniak@irobot.com> wrote:
>
> $ ls -la /lib/firmware/ar9170*
> -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

Re: compat-wireless and minstrel

[Adam Wozniak]

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


Luis R. Rodriguez wrote:
> On Wed, Nov 4, 2009 at 1:46 PM, Adam Wozniak <awozniak@irobot.com> wrote:
>   
>> $ ls -la /lib/firmware/ar9170*
>> -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
>   

Re: compat-wireless and minstrel

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 1:53 PM, Adam Wozniak <awozniak@irobot.com> wrote:
> 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

Affirmative.

  Luis

Re: compat-wireless and minstrel

[Christian Lamparter]

On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:
> On Wed, Nov 4, 2009 at 1:46 PM, Adam Wozniak <awozniak@irobot.com> wrote:
> >
> > $ ls -la /lib/firmware/ar9170*
> > -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?

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

Regards,
	Chr

Re: compat-wireless and minstrel

[Luis R. Rodriguez]

On Wed, Nov 04, 2009 at 11:18:13PM +0100, Christian Lamparter wrote:
> On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:
> > On Wed, Nov 4, 2009 at 1:46 PM, Adam Wozniak <awozniak@irobot.com> wrote:
> > >
> > > $ ls -la /lib/firmware/ar9170*
> > > -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?
> 
> 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 :)

  Luis

Re: compat-wireless and minstrel

[Christian Lamparter]

On Wednesday 04 November 2009 23:20:07 Luis R. Rodriguez wrote:
> On Wed, Nov 04, 2009 at 11:18:13PM +0100, Christian Lamparter wrote:
> > On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:
> > > On Wed, Nov 4, 2009 at 1:46 PM, Adam Wozniak <awozniak@irobot.com> wrote:
> > > >
> > > > $ ls -la /lib/firmware/ar9170*
> > > > -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?
> > 
> > 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.

Regards,
	Chr

Re: compat-wireless and minstrel

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:31 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Wednesday 04 November 2009 23:20:07 Luis R. Rodriguez wrote:
>> On Wed, Nov 04, 2009 at 11:18:13PM +0100, Christian Lamparter wrote:
>> > On Wednesday 04 November 2009 22:50:46 Luis R. Rodriguez wrote:
>> > > On Wed, Nov 4, 2009 at 1:46 PM, Adam Wozniak <awozniak@irobot.com> wrote:
>> > > >
>> > > > $ ls -la /lib/firmware/ar9170*
>> > > > -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?
>> >
>> > 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.

Ok cool.

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

Not sure I got this part.

 Luis

Re: compat-wireless and minstrel

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

Derek Smithies wrote:
> 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.

Re: compat-wireless and minstrel

[Derek Smithies]

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

On Tue, 10 Nov 2009, Adam Wozniak wrote:

> 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.
>
> Derek Smithies wrote:
>> 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.
>
>
>

-- 
Derek Smithies Ph.D.
IndraNet Technologies Ltd.
ph +64 3 365 6485
Web: http://www.indranet-technologies.com/

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: compat-wireless and minstrel

[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) {
-         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();
 


Derek Smithies wrote:
> Hi,
>> 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
>
> On Tue, 10 Nov 2009, Adam Wozniak wrote:
>
>> 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.
>>
>> Derek Smithies wrote:
>>> 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.
>>
>>
>>
>

Re: compat-wireless and minstrel

[Derek Smithies]

Hi,

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

On Tue, 10 Nov 2009, Adam Wozniak wrote:

> 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) {
> -         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();
>
>
>
> Derek Smithies wrote:
>> Hi,
>>> 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
>> 
>> On Tue, 10 Nov 2009, Adam Wozniak wrote:
>> 
>>> 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.
>>> 
>>> Derek Smithies wrote:
>>>> 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.
>>> 
>>> 
>>> 
>> 
>
>
>

-- 
Derek Smithies Ph.D.
IndraNet Technologies Ltd.
ph +64 3 365 6485
Web: http://www.indranet-technologies.com/

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: compat-wireless and minstrel

[Adam Wozniak]

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.

Derek Smithies wrote:
> Hi,
>
>> 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.
>
> On Tue, 10 Nov 2009, Adam Wozniak wrote:
>
>> 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) {
>> -         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();
>>
>>
>>
>> Derek Smithies wrote:
>>> Hi,
>>>> 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
>>>
>>> On Tue, 10 Nov 2009, Adam Wozniak wrote:
>>>
>>>> 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.
>>>>
>>>> Derek Smithies wrote:
>>>>> 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.
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>

Re: compat-wireless and minstrel

[Christian Lamparter]

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?
---
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index fbffce9..bde89f7 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -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)

Re: compat-wireless and minstrel

[Adam Wozniak]

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) {
-         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]);
+
+                 /* 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);



Christian Lamparter wrote:
> 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?
> ---
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index fbffce9..bde89f7 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -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)
>   

Re: compat-wireless and minstrel

[Adam Wozniak]

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 ****
      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) {
-         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;
 
+             if (sta->sta.supp_rates[band] != prev_rates) {
  #ifdef CONFIG_MAC80211_IBSS_DEBUG
                  printk(KERN_DEBUG "%s: updated supp_rates set "
                      "for %pM based on beacon info (0x%llx | "
                      "0x%llx -> 0x%llx)\n",
***************
*** 272,277 ****
--- 274,285 ----
                      (unsigned long long) supp_rates,
                      (unsigned long long) sta->sta.supp_rates[band]);
  #endif
+
+                 /* TODO: implement rate_update in minstrel/pid,
+                  * then change this to rate_control_rate_update.
+                  */
+                 rate_control_rate_init(sta);
+             }
          } else
              ieee80211_ibss_add_sta(sdata, mgmt->bssid, mgmt->sa, 
supp_rates);
 



Adam Wozniak wrote:
> 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) {
> -         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]);
> +
> +                 /* 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);
>
>
>
> Christian Lamparter wrote:
>> 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?
>> ---
>> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
>> index fbffce9..bde89f7 100644
>> --- a/net/mac80211/ibss.c
>> +++ b/net/mac80211/ibss.c
>> @@ -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)
>>   
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: compat-wireless and minstrel

[Johannes Berg]

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

On Thu, 2009-11-12 at 14:41 -0800, Adam Wozniak wrote:
> 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 ****

[snip]

it would help if you used diff -u

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: compat-wireless and minstrel

[Adam Wozniak]

Johannes Berg wrote:
> it would help if you used diff -u
> johannes
>   
Will do.

I think the patch below is most correct.  However, I still have problems 
when a third station joins the ad-hoc network.
When the third station joins, it doesn't always show a complete set of 
bitrates in the rc_stats files for the other two stations.  For one (I 
assume this is the beacon OR whoever sent a probe response) it shows 
all, but for the other, it shows only one or two.  This is remarkably 
similar to the failure mode I was seeing before, so much so that there 
were probably cases where I confused the two issues.
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).

Thoughts?

patch for ibss.c :

--- compat-wireless-2009-11-09/net/mac80211/ibss.c    2009-11-08 
21:15:06.000000000 -0800
+++ compat-wireless-2009-11-09d/net/mac80211/ibss.c    2009-11-13 
14:17:37.935556965 -0800
@@ -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))

Re: compat-wireless and minstrel

[Johannes Berg]

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

On Fri, 2009-11-13 at 14:35 -0800, Adam Wozniak wrote:

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: compat-wireless and minstrel

[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?  If 
this is true, it's not clear to me how the non beaconing stations will 
update their rate information about each other based on the beacon.  It 
seems like, in the case we're "absent other information", we also need 
to send a probe request, OR we need to get the bitrate information from 
the next probe request we receive from them (neither of which we seem to 
be doing).

Johannes Berg wrote:
> On Fri, 2009-11-13 at 14:35 -0800, Adam Wozniak wrote:
>
>   
>> 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).
>>     
>
> 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
>   

Re: compat-wireless and minstrel

[Johannes Berg]

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

On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: compat-wireless and minstrel

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

Johannes Berg wrote:
> On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
>   
>> 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
>   

Re: compat-wireless and minstrel

[Johannes Berg]

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

On Mon, 2009-11-16 at 09:57 -0800, Adam Wozniak wrote:
> 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.

That is indeed possible, shouldn't happen in the scenario you were
outlining (three stations) though. It's more likely that one of them
doesn't implement beaconing correctly.

It would make sense either way though to send probe requests and
reacting to them and probe responses, if you want to implement that.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Adhoc networking, was Re: compat-wireless and minstrel

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


On Mon, 16 Nov 2009, Adam Wozniak wrote:

> 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.
>
> Johannes Berg wrote:
>> On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
>> 
>>> 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
>> 
>
>
>

-- 
Derek Smithies Ph.D.
IndraNet Technologies Ltd.
ph +64 3 365 6485
Web: http://www.indranet-technologies.com/

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: Adhoc networking, was Re: compat-wireless and minstrel

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

Derek Smithies wrote:
> 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.
>
>
> On Mon, 16 Nov 2009, Adam Wozniak wrote:
>
>> 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.
>>
>> Johannes Berg wrote:
>>> On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
>>>
>>>> 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
>>>
>>
>>
>>
>

Re: Adhoc networking, was Re: compat-wireless and minstrel

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

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



On Mon, 16 Nov 2009, Adam Wozniak wrote:

> 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 ?
>
> Derek Smithies wrote:
>> 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.
>> 
>> 
>> On Mon, 16 Nov 2009, Adam Wozniak wrote:
>> 
>>> 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.
>>> 
>>> Johannes Berg wrote:
>>>> On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
>>>> 
>>>>> 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
>>>> 
>>> 
>>> 
>>> 
>> 
>
>
>

-- 
Derek Smithies Ph.D.
IndraNet Technologies Ltd.
ph +64 3 365 6485
Web: http://www.indranet-technologies.com/

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Adam Wozniak]

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.

Derek Smithies wrote:
> 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:
>
>>> 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.
>
>
>
> On Mon, 16 Nov 2009, Adam Wozniak wrote:
>
>> 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 ?
>>
>> Derek Smithies wrote:
>>> 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.
>>>
>>>
>>> On Mon, 16 Nov 2009, Adam Wozniak wrote:
>>>
>>>> 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.
>>>>
>>>> Johannes Berg wrote:
>>>>> On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
>>>>>
>>>>>> 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
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Felix Fietkau]

Adam Wozniak wrote:
> 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.
In mesh networks here in Berlin and elsewhere, some people are running
with beacons disabled ('ahdemo' mode), so it would be nice if mac80211
could deal with this as well, by testing rates in absence of beacon
information.

- Felix

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Derek Smithies]

On Mon, 16 Nov 2009, Adam Wozniak wrote:

> 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?
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
>>>> a)supported rates
>>>> b)the value of X, the value of x
>>>> c)the channel
>>>> d)the ESSSID
from an existing network.

>
> 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.
  These silly examples illustrate deficiencies in the spec.
We do have to know about them, as we need to know what will happen out 
there.

> This has obvious problems for PID and similar rate algorithms.
it may do - but who uses PID now? My understanding was that Minstrel has 
proven to be the better approach.


Derek.
=============================================================
> Derek Smithies wrote:
>> 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:
>> 
>>>> 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.
>> 
>> 
>> 
>> On Mon, 16 Nov 2009, Adam Wozniak wrote:
>> 
>>> 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 ?
>>> 
>>> Derek Smithies wrote:
>>>> 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.
>>>> 
>>>> 
>>>> On Mon, 16 Nov 2009, Adam Wozniak wrote:
>>>> 
>>>>> 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.
>>>>> 
>>>>> Johannes Berg wrote:
>>>>>> On Mon, 2009-11-16 at 09:25 -0800, Adam Wozniak wrote:
>>>>>> 
>>>>>>> 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
>>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>> 
>>> 
>>> 
>>> 
>> 
>
>
>

-- 
Derek Smithies Ph.D.
IndraNet Technologies Ltd.
ph +64 3 365 6485
Web: http://www.indranet-technologies.com/

"The only thing IE should be used for is to download Fire Fox"

"My favorite language is call STAR. It's extremely concise. It has
  exactly one verb '*', which does exactly what I want at the moment."
     --Larry Wall

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Johannes Berg]

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

Please everyone in this thread, trim your quotes.

On Tue, 2009-11-17 at 13:20 +1300, Derek Smithies wrote:

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

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Adam Wozniak]

Johannes Berg wrote:
> 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
@@ -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
@@ -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));
 

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Adam Wozniak]

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

Adam Wozniak wrote:
> 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
> @@ -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
> @@ -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));

Re: Adhoc networking, was Re: compat-wireless and minstrel

[Johannes Berg]

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

On Mon, 2009-11-23 at 12:21 -0800, Adam Wozniak wrote:
> What's the next step in making sure this patch makes it into the real world?

In that form, it never will. You need to create it against
wireless-testing, give it a good changelog, sign it off, make it not
white-space damaged, etc.:
http://wireless.kernel.org/en/developers/Documentation/SubmittingPatches

Besides, some of it really is a hack.

johannes


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 0/2] mac80211: IBSS rates

[Adam Wozniak]

These patches fix problems with rate selection in Ad-Hoc mode

    [PATCH 1/2] mac80211: supp_rates initialization and rate control 
notification
    [PATCH 2/2] mac80211: minstrel try all rates

[PATCH v2 0/2] mac80211: IBSS rates

[Adam Wozniak]

These patches fix problems with rate selection in Ad-Hoc mode.  Hopefully not whitespace damaged this time.

   [PATCH v2 1/2] mac80211: supp_rates initialization and rate control notification
   [PATCH v2 2/2] mac80211: minstrel try all rates

[PATCH 1/2] mac80211: supp_rates initialization and rate control notification

[Adam Wozniak]

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 <awozniak@irobot.com>
---
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 10d1385..474f66d 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -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 | "
                     "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))

Re: [PATCH 1/2] mac80211: supp_rates initialization and rate control notification

[Johannes Berg]

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

On Mon, 2009-11-23 at 16:57 -0800, Adam Wozniak wrote:
> 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 <awozniak@irobot.com>
> ---
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index 10d1385..474f66d 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -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);

This change is pointless -- if we don't get into this if() we don't care
about the value of supp_rates.
 
> @@ -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;

That's just as pointless, it doesn't matter whether you add the
mandatory rates here or earlier.

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

And that's not really right -- I don't think we should be calling
_init() over and over again. This could be a call to
rate_control_rate_update() with a certain flag instead.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 1/2] mac80211: supp_rates initialization and rate control notification

[Adam Wozniak]

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 <awozniak@irobot.com>
---
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 10d1385..474f66d 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -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 | "
 				    "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))

Re: [PATCH v2 1/2] mac80211: supp_rates initialization and rate control notification

[Johannes Berg]

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

On Tue, 2009-11-24 at 09:05 -0800, Adam Wozniak wrote:
> 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)

All my other comments still stand.

johannes

> Signed-off-by: Adam Wozniak <awozniak@irobot.com>
> ---
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index 10d1385..474f66d 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -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 | "
>  				    "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))
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

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 <awozniak@irobot.com>
---
diff --git a/net/mac80211/rc80211_minstrel.c 
b/net/mac80211/rc80211_minstrel.c
index 6e5d68b..3a8fe30 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -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));
 

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Mon, 2009-11-23 at 16:57 -0800, Adam Wozniak wrote:
> 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%.

Both your patches are completely whitespace damaged.

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.

johannes

> Signed-off-by: Adam Wozniak <awozniak@irobot.com>
> ---
> diff --git a/net/mac80211/rc80211_minstrel.c 
> b/net/mac80211/rc80211_minstrel.c
> index 6e5d68b..3a8fe30 100644
> --- a/net/mac80211/rc80211_minstrel.c
> +++ b/net/mac80211/rc80211_minstrel.c
> @@ -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));
>  
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> Both your patches are completely whitespace damaged.
Can you explain what you mean by that?

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Adam Wozniak wrote:
> Johannes Berg wrote:
>> Both your patches are completely whitespace damaged.
> Can you explain what you mean by that?
Ah, xterm cut&paste converting tabs to space.  Will try that again.

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> 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 concur 2/2 is a hack.  I'm not an 802.11 ad-hoc expert, and I haven't 
been allocated time to pursue the issue any more deeply than this.  It 
is still not clear to me how the list of usable rates is supposed to be 
determined when the IBSS is composed of stations with heterogeneous rate 
capabilities, beaconing is stochastically distributed, and not all nodes 
can hear all beacons, much less what is wrong with the way the current 
code is doing it.

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.

I would certainly be in favor of a better patch that resolves all the 
issues.

--Adam Wozniak

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Tue, 2009-11-24 at 09:17 -0800, Adam Wozniak wrote:

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

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> On Tue, 2009-11-24 at 09:17 -0800, Adam Wozniak wrote:
>   
>> 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.

Unless you want to make rate_control_rate_update() do this:

if (ref->ops->rate_update)
                ref->ops->rate_update(ref->priv, sband, ista, priv_sta, 
changed);
else if (ref->ops->rate_init)
                ref->ops->rate_update(ref->priv, sband, ista, priv_sta);

But that seems equally distasteful.

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Tue, 2009-11-24 at 09:55 -0800, Adam Wozniak wrote:
> Johannes Berg wrote:
> > On Tue, 2009-11-24 at 09:17 -0800, Adam Wozniak wrote:
> >   
> >> 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.

I think I'd rather require that they implement it then, and add a new
flag that says "got new info on supported rates".

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?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> 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.

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Tue, 2009-11-24 at 10:34 -0800, Adam Wozniak wrote:
> Johannes Berg wrote:
> > 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.

Hah, indeed, I thought it was symmetric, guess it's not.

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.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> 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.

--Adam Wozniak

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Tue, 2009-11-24 at 10:43 -0800, Adam Wozniak wrote:
> Johannes Berg wrote:
> > 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.

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

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> On Tue, 2009-11-24 at 10:43 -0800, Adam Wozniak wrote:
>   
>> 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:

./drivers/net/wireless/iwlwifi/iwl-3945-rs.c
./drivers/net/wireless/iwlwifi/iwl-agn-rs.c
./drivers/net/wireless/ath/ath9k/rc.c

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Tue, 2009-11-24 at 11:44 -0800, Adam Wozniak wrote:
> Johannes Berg wrote:
> > On Tue, 2009-11-24 at 10:43 -0800, Adam Wozniak wrote:
> >   
> >> 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?

Yeah ... they should go away.

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

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

Johannes Berg wrote:
> Ok, but can they all deal with calling get_rate and rate_init at the
> same time? I think that could happen now, no?
>   
I understand your concern.  I do not know the code well enough to answer 
that question.

It seems like, if that is a real concern, it should be simple enough to 
guarantee that would never happen with a mutex.  But this is getting out 
of scope for the amount of time I have budgeted to pursue this issue.

[PATCH v2 2/2] mac80211: minstrel try all rates

[Adam Wozniak]

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 <awozniak@irobot.com>
---
diff --git a/net/mac80211/rc80211_minstrel.c
b/net/mac80211/rc80211_minstrel.c
index 6e5d68b..3a8fe30 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -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));
 

Re: [PATCH v2 2/2] mac80211: minstrel try all rates

[Johannes Berg]

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

On Tue, 2009-11-24 at 09:05 -0800, Adam Wozniak wrote:
> 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)

I still stand by this being the wrong thing to do

johannes

> Signed-off-by: Adam Wozniak <awozniak@irobot.com>
> ---
> diff --git a/net/mac80211/rc80211_minstrel.c
> b/net/mac80211/rc80211_minstrel.c
> index 6e5d68b..3a8fe30 100644
> --- a/net/mac80211/rc80211_minstrel.c
> +++ b/net/mac80211/rc80211_minstrel.c
> @@ -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));
>  
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: compat-wireless and minstrel

[Christian Lamparter]

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

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index fbffce9..7c6c170 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -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);
 

Re: compat-wireless and minstrel

[Adam Wozniak]

I'll let that run tonight as well.  Although I think the comment about 
using rate_control_rate_update instead of rate_control_rate_init would 
be important to future developers.  I also notice that mlme.c calls 
rate_control_rate_update.  I'm not sure what that does.  Someone 
familiar with that may want to investigate, it's probably a big deal if 
someone changes bands and all the supported rates change.

Alternately, it may be ok just to stick a stub in like this for now:

static void minstrel_rate_update(void *priv, struct 
ieee80211_supported_band *sband,
               struct ieee80211_sta *sta, void *priv_sta, u32 changed) {
   minstrel_rate_init(priv, sband, sta, priv_sta);
}

( and update mac80211_minstrel )


Thoughts?

--Adam

Christian Lamparter wrote:
> 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 :) )
> ---
>
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index fbffce9..7c6c170 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -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);
>  
>   

Re: compat-wireless and minstrel

[Adam Wozniak]

Eep, I can already tell this isn't going to work out.  The ewma 
probability reported in rc_stats is always 0.0, and the success and 
attempts columns are always zero, as if the minstrel layer is constantly 
being reinitialized.  My previous patch was better.

--Adam

Christian Lamparter wrote:
> 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 :) )
> ---
>
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index fbffce9..7c6c170 100644
> --- a/net/mac80211/ibss.c
> +++ b/net/mac80211/ibss.c
> @@ -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);
>  
>