Regression in b43 with BCM4311/2

Regression in b43 with BCM4311/2

[Larry Finger]

Johannes,

I installed my BCM4311/2 card to see if I could reproduce the problems being
reported in the list. It had been some time since I last used this model, and
was quite surprised to discover that the maximum transmit rate was only 1 Mb/s.
It had been much higher in the past. By booting old distributions/kernels, I
found that 2.6.27 could transmit at rates nearly 20 Mb/s, thus the problem
occurred in mainline during the 2.6.28 merge. Bisection found the problem to be
with the following commit:

commit c7ab5ef9bcd281135c21b4732c9be779585181be
Author: Johannes Berg <johannes@sipsolutions.net>
Date:   Wed Oct 29 20:02:12 2008 +0100

    b43: implement short slot and basic rate handling

    This implements proper short slot handling and adds code to
    program the hardware for the correct response rates derived
    from the basic rate set for the current BSS.

    Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

It seemed unlikely that the short slot handling was the cause, thus I have
concentrated on the basic rate handling. I found that the returns from
ieee80211_get_response_rate() were not what I expected. I'm not sure the routine
is correct; however, the value for "brates" at entry to b43_update_basic_rates()
is 0xF, thus only the CCK rates are enabled, but sband->n_bitrates is 12, which
includes all the 802.11g rates.

Is b43 missing something needed to set the basic_rates member of struct
ieee80211_bxx_conf to a more reasonable value when b43_op_bss_info_changed() is
entered? I think it should be 0xFFF, not 0xF. I have tested with the larger
value and found that this change did not improve transmit rates.

Thanks,

Larry

Re: Regression in b43 with BCM4311/2

[Johannes Berg]

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

Larry,

> commit c7ab5ef9bcd281135c21b4732c9be779585181be
> Author: Johannes Berg <johannes@sipsolutions.net>
> Date:   Wed Oct 29 20:02:12 2008 +0100
> 
>     b43: implement short slot and basic rate handling

> It seemed unlikely that the short slot handling was the cause, thus I have
> concentrated on the basic rate handling. I found that the returns from
> ieee80211_get_response_rate() were not what I expected. I'm not sure the routine
> is correct; however, the value for "brates" at entry to b43_update_basic_rates()
> is 0xF, thus only the CCK rates are enabled, but sband->n_bitrates is 12, which
> includes all the 802.11g rates.

It doesn't really depend on your supported rates, if your AP says that
only CCK rates are basic rates (you can see that in iw scan output, they
are marked with a *) then they need to be used for response frames, as
per 802.11-2007 9.6 paragraph 7:

        To allow the transmitting STA to calculate the contents of the
        Duration/ID field, a STA responding to a received frame shall
        transmit its Control Response frame (either CTS or ACK), other
        than the BlockAck control frame, at the highest rate in the
        BSSBasicRateSet parameter that is less than or equal to the rate
        of the immediately previous frame in the frame exchange sequence
        (as defined in 9.12) and that is of the same modulation class
        (see 9.6.1) as the received frame. If no rate contained in the
        BSSBasicRateSet parameter meets these conditions, then the
        control frame sent in response to a received frame shall be
        transmitted at the highest mandatory rate of the PHY that is
        less than or equal to the rate of the received frame, and that
        is of the same modulation class as the received frame. In
        addition, the Control Response frame shall be sent using the
        same PHY options as the received frame, unless they conflict
        with the requirement to use the BSSBasicRateSet parameter.
        

Then again, if I read that correctly, we should be checking the
modulation classes, will have to take a closer look at 9.6.1.

> Is b43 missing something needed to set the basic_rates member of struct
> ieee80211_bxx_conf to a more reasonable value when b43_op_bss_info_changed() is
> entered? I think it should be 0xFFF, not 0xF. I have tested with the larger
> value and found that this change did not improve transmit rates.

So you're saying it doesn't help anyway?

johannes

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

Re: Regression in b43 with BCM4311/2

[Larry Finger]

On 01/30/2010 03:30 PM, Johannes Berg wrote:
> Larry,
> 
>> commit c7ab5ef9bcd281135c21b4732c9be779585181be
>> Author: Johannes Berg <johannes@sipsolutions.net>
>> Date:   Wed Oct 29 20:02:12 2008 +0100
>>
>>     b43: implement short slot and basic rate handling
> 
>> It seemed unlikely that the short slot handling was the cause, thus I have
>> concentrated on the basic rate handling. I found that the returns from
>> ieee80211_get_response_rate() were not what I expected. I'm not sure the routine
>> is correct; however, the value for "brates" at entry to b43_update_basic_rates()
>> is 0xF, thus only the CCK rates are enabled, but sband->n_bitrates is 12, which
>> includes all the 802.11g rates.
> 
> It doesn't really depend on your supported rates, if your AP says that
> only CCK rates are basic rates (you can see that in iw scan output, they
> are marked with a *) then they need to be used for response frames, as
> per 802.11-2007 9.6 paragraph 7:
> 
>         To allow the transmitting STA to calculate the contents of the
>         Duration/ID field, a STA responding to a received frame shall
>         transmit its Control Response frame (either CTS or ACK), other
>         than the BlockAck control frame, at the highest rate in the
>         BSSBasicRateSet parameter that is less than or equal to the rate
>         of the immediately previous frame in the frame exchange sequence
>         (as defined in 9.12) and that is of the same modulation class
>         (see 9.6.1) as the received frame. If no rate contained in the
>         BSSBasicRateSet parameter meets these conditions, then the
>         control frame sent in response to a received frame shall be
>         transmitted at the highest mandatory rate of the PHY that is
>         less than or equal to the rate of the received frame, and that
>         is of the same modulation class as the received frame. In
>         addition, the Control Response frame shall be sent using the
>         same PHY options as the received frame, unless they conflict
>         with the requirement to use the BSSBasicRateSet parameter.
>         
> 
> Then again, if I read that correctly, we should be checking the
> modulation classes, will have to take a closer look at 9.6.1.

My AP returns the following in the scan output:

Bit Rates:1 Mb/s; 2 Mb/s; 5.5 Mb/s; 11 Mb/s; 18 Mb/s
          24 Mb/s; 36 Mb/s; 54 Mb/s
Bit Rates:6 Mb/s; 9 Mb/s; 12 Mb/s; 48 Mb/s

I don't see any * markings.

>> Is b43 missing something needed to set the basic_rates member of struct
>> ieee80211_bxx_conf to a more reasonable value when b43_op_bss_info_changed() is
>> entered? I think it should be 0xFFF, not 0xF. I have tested with the larger
>> value and found that this change did not improve transmit rates.
> 
> So you're saying it doesn't help anyway?

Yes, it does not help. Since my previous message, I have also tested the short
slot logic. Inverting the test made no difference.

Larry

Re: Regression in b43 with BCM4311/2

[Johannes Berg]

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

On Sat, 2010-01-30 at 15:55 -0600, Larry Finger wrote:
> On 01/30/2010 03:30 PM, Johannes Berg wrote:

> My AP returns the following in the scan output:
> 
> Bit Rates:1 Mb/s; 2 Mb/s; 5.5 Mb/s; 11 Mb/s; 18 Mb/s
>           24 Mb/s; 36 Mb/s; 54 Mb/s
> Bit Rates:6 Mb/s; 9 Mb/s; 12 Mb/s; 48 Mb/s
> 
> I don't see any * markings.

Check with iw, instead of iwlist, I think?

> >> Is b43 missing something needed to set the basic_rates member of struct
> >> ieee80211_bxx_conf to a more reasonable value when b43_op_bss_info_changed() is
> >> entered? I think it should be 0xFFF, not 0xF. I have tested with the larger
> >> value and found that this change did not improve transmit rates.
> > 
> > So you're saying it doesn't help anyway?
> 
> Yes, it does not help. Since my previous message, I have also tested the short
> slot logic. Inverting the test made no difference.

Strange. can the patch be reverted as a whole and then it's ok again?

johannes

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

Re: Regression in b43 with BCM4311/2

[Larry Finger]

On 01/30/2010 03:58 PM, Johannes Berg wrote:
> On Sat, 2010-01-30 at 15:55 -0600, Larry Finger wrote:
>> On 01/30/2010 03:30 PM, Johannes Berg wrote:
> 
>> My AP returns the following in the scan output:
>>
>> Bit Rates:1 Mb/s; 2 Mb/s; 5.5 Mb/s; 11 Mb/s; 18 Mb/s
>>           24 Mb/s; 36 Mb/s; 54 Mb/s
>> Bit Rates:6 Mb/s; 9 Mb/s; 12 Mb/s; 48 Mb/s
>>
>> I don't see any * markings.
> 
> Check with iw, instead of iwlist, I think?

        Supported rates: 1.0* 2.0* 5.5* 11.0* 18.0 24.0 36.0 54.0
        Extended supported rates: 6.0 9.0 12.0 48.0

OK, only the CCK rates are the basic ones.

> Strange. can the patch be reverted as a whole and then it's ok again?

No the patch cannot be reverted, but I have disabled both parts of it separately
without helping the situation. My bisection must have given a false result.

When I use my 4315 device with an LP PHY, it is OK. The problem has to be in the
G PHY-only code, which your patch didn't touch. I'll have to go back through the
patches in the 2.6.28 merge.

I'll let you know what I find.

Larry

Re: Regression in b43 with BCM4311/2

[Larry Finger]

Johannes,

I finally sorted this out. The patch in question (c7ab5ef) is the bad one, but
not the basic response rate part.

Going back to the code that worked, I discovered that the routines
b43_short_slot_timing_enable/disable() were never called. Once the new code
touched the shared memory location 0x0010, the transmit performance dropped way
off. If I let the new code adjust the Interframe Slot time in MMIO space, but
not the shared memory location, the transmit rate in my BCM4311/2 went from a
maximum of 18 to 22 Mb/s. This problem did not affect the 0x4315 device as the
memory was changed only for G PHYs. I think that should be for 2GHz, and have
made that adjustment. I have not yet tested that with the LP PHY. Once I do, I
will post a patch for comment and testing.

Thanks for helping me understand the basic response rates.

Larry

Re: Regression in b43 with BCM4311/2

[Johannes Berg]

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

Larry,

> I finally sorted this out. The patch in question (c7ab5ef) is the bad one, but
> not the basic response rate part.
> 
> Going back to the code that worked, I discovered that the routines
> b43_short_slot_timing_enable/disable() were never called. Once the new code
> touched the shared memory location 0x0010, the transmit performance dropped way
> off. If I let the new code adjust the Interframe Slot time in MMIO space, but
> not the shared memory location, the transmit rate in my BCM4311/2 went from a
> maximum of 18 to 22 Mb/s. This problem did not affect the 0x4315 device as the
> memory was changed only for G PHYs. I think that should be for 2GHz, and have
> made that adjustment. I have not yet tested that with the LP PHY. Once I do, I
> will post a patch for comment and testing.

Cool, thanks for looking into it and the detailed analysis!

> Thanks for helping me understand the basic response rates.

Any time.

johannes

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