2008-10-30 10:12:40

by Johannes Berg

[permalink] [raw]
Subject: [PATCH for 2.6.28] mac80211: fix basic rates

This fixes the basic rate bitmap calculation in mac80211.

Signed-off-by: Johannes Berg <[email protected]>
Cc: [email protected] [2.6.25, 2.6.26, 2.6.27]
---
Is this appropriate for stable? It's a trivial and obvious fix, and can
throw off the duration calculation, but it's likely nobody will ever
really notice...

net/mac80211/mlme.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

--- everything.orig/net/mac80211/mlme.c 2008-10-29 18:08:12.000000000 +0100
+++ everything/net/mac80211/mlme.c 2008-10-29 18:08:19.000000000 +0100
@@ -1307,8 +1307,9 @@ static void ieee80211_rx_mgmt_assoc_resp
have_higher_than_11mbit = true;

for (j = 0; j < sband->n_bitrates; j++) {
- if (sband->bitrates[j].bitrate == rate)
- rates |= BIT(j);
+ if (sband->bitrates[j].bitrate != rate)
+ continue;
+ rates |= BIT(j);
if (elems.supp_rates[i] & 0x80)
basic_rates |= BIT(j);
}
@@ -1321,8 +1322,9 @@ static void ieee80211_rx_mgmt_assoc_resp
have_higher_than_11mbit = true;

for (j = 0; j < sband->n_bitrates; j++) {
- if (sband->bitrates[j].bitrate == rate)
- rates |= BIT(j);
+ if (sband->bitrates[j].bitrate != rate)
+ continue;
+ rates |= BIT(j);
if (elems.ext_supp_rates[i] & 0x80)
basic_rates |= BIT(j);
}




2008-10-30 14:25:08

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH for 2.6.28] mac80211: fix basic rates

On Thu, 2008-10-30 at 15:23 +0200, Tomas Winkler wrote:
> On Wed, Oct 29, 2008 at 7:14 PM, Johannes Berg
> <[email protected]> wrote:
> > This fixes the basic rate bitmap calculation in mac80211.
> >
> > Signed-off-by: Johannes Berg <[email protected]>
> > Cc: [email protected] [2.6.25, 2.6.26, 2.6.27]
> > ---
> > Is this appropriate for stable? It's a trivial and obvious fix, and can
> > throw off the duration calculation, but it's likely nobody will ever
> > really notice...
>
> You would should fail association if one of the basic rates is not
> supported by the station

No, this has nothing to do with association, it's just setting the
internal variable about which rates are basic in the BSS.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-10-30 14:52:00

by Tomas Winkler

[permalink] [raw]
Subject: Re: [PATCH for 2.6.28] mac80211: fix basic rates

On Thu, Oct 30, 2008 at 4:41 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2008-10-30 at 16:39 +0200, Tomas Winkler wrote:
>> On Thu, Oct 30, 2008 at 4:24 PM, Johannes Berg
>> <[email protected]> wrote:
>> > On Thu, 2008-10-30 at 15:23 +0200, Tomas Winkler wrote:
>> >> On Wed, Oct 29, 2008 at 7:14 PM, Johannes Berg
>> >> <[email protected]> wrote:
>> >> > This fixes the basic rate bitmap calculation in mac80211.
>> >> >
>> >> > Signed-off-by: Johannes Berg <[email protected]>
>> >> > Cc: [email protected] [2.6.25, 2.6.26, 2.6.27]
>> >> > ---
>> >> > Is this appropriate for stable? It's a trivial and obvious fix, and can
>> >> > throw off the duration calculation, but it's likely nobody will ever
>> >> > really notice...
>> >>
>> >> You would should fail association if one of the basic rates is not
>> >> supported by the station
>> >
>> > No, this has nothing to do with association, it's just setting the
>> > internal variable about which rates are basic in the BSS.
>>
>> I understand that but What your patch is doing it is preventing
>> of setting basic rates for ratest that are not sported by the driver,
>> but according
>> the spec it is not permissible that driver will not support one of the
>> basic rates.
>
> I don't understand. My patch is simply fixing the basic rate bitmap
> calculation. If we don't support all basic rates then the AP shall have
> denied the association, or we probably didn't even ask it.
>
> The current code there is just wrong, it will always set _all_ rates in
> the basic rate bitmap rather than just those that really are basic
> rates.

Oh, I now I got it out of index. What about coding it in more readable way?


for (i = 0; i < elems.supp_rates_len; i++) {
int rate = (elems.supp_rates[i] & 0x7f) * 5;
bool is_basic = !!(elems.supp_rates[i] & 0x80);

if (rate > 110)
have_higher_than_11mbit = true;

for (j = 0; j < sband->n_bitrates; j++) {
if (sband->bitrates[j].bitrate == rate) {
rates |= BIT(j);
if (is_basic)
basic_rates |= BIT(j);
break;
}
}
}

Tomas

2008-10-30 13:24:00

by Tomas Winkler

[permalink] [raw]
Subject: Re: [PATCH for 2.6.28] mac80211: fix basic rates

On Wed, Oct 29, 2008 at 7:14 PM, Johannes Berg
<[email protected]> wrote:
> This fixes the basic rate bitmap calculation in mac80211.
>
> Signed-off-by: Johannes Berg <[email protected]>
> Cc: [email protected] [2.6.25, 2.6.26, 2.6.27]
> ---
> Is this appropriate for stable? It's a trivial and obvious fix, and can
> throw off the duration calculation, but it's likely nobody will ever
> really notice...

You would should fail association if one of the basic rates is not
supported by the station
Tomas

>
> net/mac80211/mlme.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> --- everything.orig/net/mac80211/mlme.c 2008-10-29 18:08:12.000000000 +0100
> +++ everything/net/mac80211/mlme.c 2008-10-29 18:08:19.000000000 +0100
> @@ -1307,8 +1307,9 @@ static void ieee80211_rx_mgmt_assoc_resp
> have_higher_than_11mbit = true;
>
> for (j = 0; j < sband->n_bitrates; j++) {
> - if (sband->bitrates[j].bitrate == rate)
> - rates |= BIT(j);
> + if (sband->bitrates[j].bitrate != rate)
> + continue;
> + rates |= BIT(j);
> if (elems.supp_rates[i] & 0x80)
> basic_rates |= BIT(j);
> }
> @@ -1321,8 +1322,9 @@ static void ieee80211_rx_mgmt_assoc_resp
> have_higher_than_11mbit = true;
>
> for (j = 0; j < sband->n_bitrates; j++) {
> - if (sband->bitrates[j].bitrate == rate)
> - rates |= BIT(j);
> + if (sband->bitrates[j].bitrate != rate)
> + continue;
> + rates |= BIT(j);
> if (elems.ext_supp_rates[i] & 0x80)
> basic_rates |= BIT(j);
> }
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2008-10-30 14:41:46

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH for 2.6.28] mac80211: fix basic rates

On Thu, 2008-10-30 at 16:39 +0200, Tomas Winkler wrote:
> On Thu, Oct 30, 2008 at 4:24 PM, Johannes Berg
> <[email protected]> wrote:
> > On Thu, 2008-10-30 at 15:23 +0200, Tomas Winkler wrote:
> >> On Wed, Oct 29, 2008 at 7:14 PM, Johannes Berg
> >> <[email protected]> wrote:
> >> > This fixes the basic rate bitmap calculation in mac80211.
> >> >
> >> > Signed-off-by: Johannes Berg <[email protected]>
> >> > Cc: [email protected] [2.6.25, 2.6.26, 2.6.27]
> >> > ---
> >> > Is this appropriate for stable? It's a trivial and obvious fix, and can
> >> > throw off the duration calculation, but it's likely nobody will ever
> >> > really notice...
> >>
> >> You would should fail association if one of the basic rates is not
> >> supported by the station
> >
> > No, this has nothing to do with association, it's just setting the
> > internal variable about which rates are basic in the BSS.
>
> I understand that but What your patch is doing it is preventing
> of setting basic rates for ratest that are not sported by the driver,
> but according
> the spec it is not permissible that driver will not support one of the
> basic rates.

I don't understand. My patch is simply fixing the basic rate bitmap
calculation. If we don't support all basic rates then the AP shall have
denied the association, or we probably didn't even ask it.

The current code there is just wrong, it will always set _all_ rates in
the basic rate bitmap rather than just those that really are basic
rates.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-10-30 14:54:56

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH for 2.6.28] mac80211: fix basic rates

On Thu, 2008-10-30 at 16:51 +0200, Tomas Winkler wrote:

> Oh, I now I got it out of index. What about coding it in more readable way?
>
>
> for (i = 0; i < elems.supp_rates_len; i++) {
> int rate = (elems.supp_rates[i] & 0x7f) * 5;
> bool is_basic = !!(elems.supp_rates[i] & 0x80);
>
> if (rate > 110)
> have_higher_than_11mbit = true;
>
> for (j = 0; j < sband->n_bitrates; j++) {
> if (sband->bitrates[j].bitrate == rate) {
> rates |= BIT(j);
> if (is_basic)
> basic_rates |= BIT(j);
> break;

yeah, that works too, and the break is probably a good idea, want to
send a patch to supersede this one?

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-10-30 14:39:06

by Tomas Winkler

[permalink] [raw]
Subject: Re: [PATCH for 2.6.28] mac80211: fix basic rates

On Thu, Oct 30, 2008 at 4:24 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2008-10-30 at 15:23 +0200, Tomas Winkler wrote:
>> On Wed, Oct 29, 2008 at 7:14 PM, Johannes Berg
>> <[email protected]> wrote:
>> > This fixes the basic rate bitmap calculation in mac80211.
>> >
>> > Signed-off-by: Johannes Berg <[email protected]>
>> > Cc: [email protected] [2.6.25, 2.6.26, 2.6.27]
>> > ---
>> > Is this appropriate for stable? It's a trivial and obvious fix, and can
>> > throw off the duration calculation, but it's likely nobody will ever
>> > really notice...
>>
>> You would should fail association if one of the basic rates is not
>> supported by the station
>
> No, this has nothing to do with association, it's just setting the
> internal variable about which rates are basic in the BSS.

I understand that but What your patch is doing it is preventing
of setting basic rates for ratest that are not sported by the driver,
but according
the spec it is not permissible that driver will not support one of the
basic rates.

Tomas