[PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Larry Finger]

A coding error present since b43legacy was incorporated into the
kernel has prevented the driver from using the rate-setting mechanism
of mac80211. The driver has been forced to remain at a 1 Mb/s rate.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Stable <stable@kernel.org>		[2.6.26], [2.6.25]
---

John,

Although this is not strictly a regression, it is a bug. I hope
it can be sent to 2.6.27.

Thanks,

Larry
---

Index: wireless-testing/drivers/net/wireless/b43legacy/xmit.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43legacy/xmit.c
+++ wireless-testing/drivers/net/wireless/b43legacy/xmit.c
@@ -629,7 +629,7 @@ void b43legacy_handle_hwtxstatus(struct
 	status.pm_indicated = !!(tmp & 0x80);
 	status.intermediate = !!(tmp & 0x40);
 	status.for_ampdu = !!(tmp & 0x20);
-	status.acked = !!(tmp & 0x02);
+	status.acked = tmp & 0x01;
 
 	b43legacy_handle_txstatus(dev, &status);
 }
Index: wireless-testing/drivers/net/wireless/b43legacy/main.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43legacy/main.c
+++ wireless-testing/drivers/net/wireless/b43legacy/main.c
@@ -744,7 +744,7 @@ static void handle_irq_transmit_status(s
 
 	while (1) {
 		v0 = b43legacy_read32(dev, B43legacy_MMIO_XMITSTAT_0);
-		if (!(v0 & 0x00000001))
+		if (!v0)
 			break;
 		v1 = b43legacy_read32(dev, B43legacy_MMIO_XMITSTAT_1);
 
@@ -758,7 +758,7 @@ static void handle_irq_transmit_status(s
 		stat.pm_indicated = !!(tmp & 0x0080);
 		stat.intermediate = !!(tmp & 0x0040);
 		stat.for_ampdu = !!(tmp & 0x0020);
-		stat.acked = !!(tmp & 0x0002);
+		stat.acked = tmp & 0x0001;
 
 		b43legacy_handle_txstatus(dev, &stat);
 	}

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Michael Buesch]

On Saturday 06 September 2008 20:34:02 Larry Finger wrote:
> A coding error present since b43legacy was incorporated into the
> kernel has prevented the driver from using the rate-setting mechanism
> of mac80211. The driver has been forced to remain at a 1 Mb/s rate.

Does version3 firmware have a different bitlayout for the status?

> Although this is not strictly a regression, it is a bug. I hope
> it can be sent to 2.6.27.

The new rules don't allow us to fix bugs after the merge window.
Only regressions.

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[John W. Linville]

On Sat, Sep 06, 2008 at 08:41:05PM +0200, Michael Buesch wrote:
> On Saturday 06 September 2008 20:34:02 Larry Finger wrote:
> > A coding error present since b43legacy was incorporated into the
> > kernel has prevented the driver from using the rate-setting mechanism
> > of mac80211. The driver has been forced to remain at a 1 Mb/s rate.
> 
> Does version3 firmware have a different bitlayout for the status?
> 
> > Although this is not strictly a regression, it is a bug. I hope
> > it can be sent to 2.6.27.
> 
> The new rules don't allow us to fix bugs after the merge window.
> Only regressions.

I imagine the powers that be would claim it isn't a new rule, but it
is a new enforcement policy...

John

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Larry Finger]

Michael Buesch wrote:
> On Saturday 06 September 2008 20:34:02 Larry Finger wrote:
>> A coding error present since b43legacy was incorporated into the
>> kernel has prevented the driver from using the rate-setting mechanism
>> of mac80211. The driver has been forced to remain at a 1 Mb/s rate.
> 
> Does version3 firmware have a different bitlayout for the status?

It seems so. I found this because I was not getting any acks back to 
net/mac80211/rc80211_pid_algo.c. I then reviewed the V3 specs, found 
that bit 0, not bit 1, contained the ack. Test prints confirmed that 
result. With this patch, both my BCM4306/2 and BCM4303 reach the 
maximum rate. With the current code, 54 Mb/s is not as fast as 36 
Mb/s, but at least the algorithm is working.

Larry

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Michael Buesch]

On Saturday 06 September 2008 20:52:53 Larry Finger wrote:
> Michael Buesch wrote:
> > On Saturday 06 September 2008 20:34:02 Larry Finger wrote:
> >> A coding error present since b43legacy was incorporated into the
> >> kernel has prevented the driver from using the rate-setting mechanism
> >> of mac80211. The driver has been forced to remain at a 1 Mb/s rate.
> > 
> > Does version3 firmware have a different bitlayout for the status?
> 
> It seems so. I found this because I was not getting any acks back to 
> net/mac80211/rc80211_pid_algo.c. I then reviewed the V3 specs, found 
> that bit 0, not bit 1, contained the ack. Test prints confirmed that 
> result. With this patch, both my BCM4306/2 and BCM4303 reach the 
> maximum rate. With the current code, 54 Mb/s is not as fast as 36 
> Mb/s, but at least the algorithm is working.

Yeah ok. I just asked, because it seems the _whole_ flags bitfield
is rightshifted by one (so the other flags are wrong, too. See the
intermediate flag)

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Johannes Berg]

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

On Sat, 2008-09-06 at 20:55 +0200, Michael Buesch wrote:
> On Saturday 06 September 2008 20:52:53 Larry Finger wrote:
> > Michael Buesch wrote:
> > > On Saturday 06 September 2008 20:34:02 Larry Finger wrote:
> > >> A coding error present since b43legacy was incorporated into the
> > >> kernel has prevented the driver from using the rate-setting mechanism
> > >> of mac80211. The driver has been forced to remain at a 1 Mb/s rate.
> > > 
> > > Does version3 firmware have a different bitlayout for the status?
> > 
> > It seems so. I found this because I was not getting any acks back to 
> > net/mac80211/rc80211_pid_algo.c. I then reviewed the V3 specs, found 
> > that bit 0, not bit 1, contained the ack. Test prints confirmed that 
> > result. With this patch, both my BCM4306/2 and BCM4303 reach the 
> > maximum rate. With the current code, 54 Mb/s is not as fast as 36 
> > Mb/s, but at least the algorithm is working.
> 
> Yeah ok. I just asked, because it seems the _whole_ flags bitfield
> is rightshifted by one (so the other flags are wrong, too. See the
> intermediate flag)

It is, this isn't really a difference between the two but a result of
you shifting it up/down due to the tx status via dma queue vs. tx status
via registers thing.

johannes

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

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Michael Buesch]

On Saturday 06 September 2008 20:57:50 Johannes Berg wrote:
> On Sat, 2008-09-06 at 20:55 +0200, Michael Buesch wrote:
> > On Saturday 06 September 2008 20:52:53 Larry Finger wrote:
> > > Michael Buesch wrote:
> > > > On Saturday 06 September 2008 20:34:02 Larry Finger wrote:
> > > >> A coding error present since b43legacy was incorporated into the
> > > >> kernel has prevented the driver from using the rate-setting mechanism
> > > >> of mac80211. The driver has been forced to remain at a 1 Mb/s rate.
> > > > 
> > > > Does version3 firmware have a different bitlayout for the status?
> > > 
> > > It seems so. I found this because I was not getting any acks back to 
> > > net/mac80211/rc80211_pid_algo.c. I then reviewed the V3 specs, found 
> > > that bit 0, not bit 1, contained the ack. Test prints confirmed that 
> > > result. With this patch, both my BCM4306/2 and BCM4303 reach the 
> > > maximum rate. With the current code, 54 Mb/s is not as fast as 36 
> > > Mb/s, but at least the algorithm is working.
> > 
> > Yeah ok. I just asked, because it seems the _whole_ flags bitfield
> > is rightshifted by one (so the other flags are wrong, too. See the
> > intermediate flag)
> 
> It is, this isn't really a difference between the two but a result of
> you shifting it up/down due to the tx status via dma queue vs. tx status
> via registers thing.

Yeah, that's the point. larry's patch modified both the register and dmaqueue
mechanism. I think the register mechanism might be correct as-is (Or is it even
dead code and it's not used by any legacy device?)

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Larry Finger]

Michael Buesch wrote:
> On Saturday 06 September 2008 20:57:50 Johannes Berg wrote:
>> It is, this isn't really a difference between the two but a result of
>> you shifting it up/down due to the tx status via dma queue vs. tx status
>> via registers thing.
> 
> Yeah, that's the point. larry's patch modified both the register and dmaqueue
> mechanism. I think the register mechanism might be correct as-is (Or is it even
> dead code and it's not used by any legacy device?)
> 

I modified the patch to add printouts in both paths as shown below:

Index: linux-2.6/drivers/net/wireless/b43legacy/xmit.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43legacy/xmit.c
+++ linux-2.6/drivers/net/wireless/b43legacy/xmit.c
@@ -631,7 +631,8 @@ void b43legacy_handle_hwtxstatus(struct
         status.pm_indicated = !!(tmp & 0x80);
         status.intermediate = !!(tmp & 0x40);
         status.for_ampdu = !!(tmp & 0x20);
-       status.acked = !!(tmp & 0x02);
+       status.acked = tmp & 0x01;
+       printk(KERN_INFO "b43legacy: In b43legacy_handle_hwtxstatus, 
hw->flags = 0x%X\n", hw->flags);

         b43legacy_handle_txstatus(dev, &status);
  }
Index: linux-2.6/drivers/net/wireless/b43legacy/main.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/b43legacy/main.c
+++ linux-2.6/drivers/net/wireless/b43legacy/main.c
@@ -744,7 +744,7 @@ static void handle_irq_transmit_status(s

         while (1) {
                 v0 = b43legacy_read32(dev, B43legacy_MMIO_XMITSTAT_0);
-               if (!(v0 & 0x00000001))
+               if (!v0)
                         break;
                 v1 = b43legacy_read32(dev, B43legacy_MMIO_XMITSTAT_1);

@@ -752,13 +752,14 @@ static void handle_irq_transmit_status(s
                 stat.seq = (v1 & 0x0000FFFF);
                 stat.phy_stat = ((v1 & 0x00FF0000) >> 16);
                 tmp = (v0 & 0x0000FFFF);
+               printk(KERN_INFO "b43legacy: In 
handle_irq_transmit_status, tmp 0x%X\n", tmp);
                 stat.frame_count = ((tmp & 0xF000) >> 12);
                 stat.rts_count = ((tmp & 0x0F00) >> 8);
                 stat.supp_reason = ((tmp & 0x001C) >> 2);
                 stat.pm_indicated = !!(tmp & 0x0080);
                 stat.intermediate = !!(tmp & 0x0040);
                 stat.for_ampdu = !!(tmp & 0x0020);
-               stat.acked = !!(tmp & 0x0002);
+               stat.acked = tmp & 0x0001;

                 b43legacy_handle_txstatus(dev, &stat);
         }


What I see are lots of

b43legacy: In b43legacy_handle_hwtxstatus, hw->flags = 0x1

and this is the only one that ever triggered. ATM, I'm not sure why 
handle_irq_transmit_status() is not called.

Larry

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Johannes Berg]

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

On Sat, 2008-09-06 at 14:36 -0500, Larry Finger wrote:

> What I see are lots of
> 
> b43legacy: In b43legacy_handle_hwtxstatus, hw->flags = 0x1
> 
> and this is the only one that ever triggered. ATM, I'm not sure why 
> handle_irq_transmit_status() is not called.

The mechanism depends on the card revision, but according to
drivers/net/wireless/b43legacy/dma.c it's always via the dma/pio
mechanism for legacy cards:

        if (dev->dev->id.revision < 5) {
                ring = b43legacy_setup_dmaring(dev, 3, 0, type);
                if (!ring)
                        goto err_destroy_rx0;
                dma->rx_ring3 = ring;
        }

johannes

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

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Larry Finger]

Johannes Berg wrote:
> 
> The mechanism depends on the card revision, but according to
> drivers/net/wireless/b43legacy/dma.c it's always via the dma/pio
> mechanism for legacy cards:
> 
>         if (dev->dev->id.revision < 5) {
>                 ring = b43legacy_setup_dmaring(dev, 3, 0, type);
>                 if (!ring)
>                         goto err_destroy_rx0;
>                 dma->rx_ring3 = ring;
>         }

In the V3 specs, I found

"Transmit Status

When this interrupt is set, the retrieve the TransmitStatus. Note that 
on cores with revision < 5, the last DMA controller or PIO queue can 
also also get the DMA recieve done interrupt, which also triggers the 
TransmitStatus retrieval process. The driver should be prepared to 
deal with both interrupts at any time, on any revision. In AP mode, 
this interrupt also initiates the sending of powersave responses."

The implication is that the interrupt will only be generated if we use 
the last (i.e. #5) DMA controller. As we are only using #3, no 
interrupts and handle_irq_status() is dead code.

Larry

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Johannes Berg]

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

On Sat, 2008-09-06 at 14:59 -0500, Larry Finger wrote:

> In the V3 specs, I found
> 
> "Transmit Status
> 
> When this interrupt is set, the retrieve the TransmitStatus. Note that 
> on cores with revision < 5, the last DMA controller or PIO queue can 
> also also get the DMA recieve done interrupt, which also triggers the 
> TransmitStatus retrieval process. The driver should be prepared to 
> deal with both interrupts at any time, on any revision. In AP mode, 
> this interrupt also initiates the sending of powersave responses."

Yeah, this isn't entirely correct, when the core revision is < 5 then
the register-based TX status doesn't actually exist and the firmware
always uses the FIFO-based mechanism.

> The implication is that the interrupt will only be generated if we use 
> the last (i.e. #5) DMA controller. As we are only using #3, no 
> interrupts and handle_irq_status() is dead code.

Right, only core revisions 2 and 4 are supported here.

johannes

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

Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism

[Michael Buesch]

On Saturday 06 September 2008 21:59:55 Larry Finger wrote:
> Johannes Berg wrote:
> > 
> > The mechanism depends on the card revision, but according to
> > drivers/net/wireless/b43legacy/dma.c it's always via the dma/pio
> > mechanism for legacy cards:
> > 
> >         if (dev->dev->id.revision < 5) {
> >                 ring = b43legacy_setup_dmaring(dev, 3, 0, type);
> >                 if (!ring)
> >                         goto err_destroy_rx0;
> >                 dma->rx_ring3 = ring;
> >         }
> 
> In the V3 specs, I found
> 
> "Transmit Status
> 
> When this interrupt is set, the retrieve the TransmitStatus. Note that 
> on cores with revision < 5, the last DMA controller or PIO queue can 
> also also get the DMA recieve done interrupt, which also triggers the 
> TransmitStatus retrieval process. The driver should be prepared to 
> deal with both interrupts at any time, on any revision. In AP mode, 
> this interrupt also initiates the sending of powersave responses."
> 
> The implication is that the interrupt will only be generated if we use 
> the last (i.e. #5) DMA controller. As we are only using #3, no 
> interrupts and handle_irq_status() is dead code.

Ok. I'm pretty sure that the current code is correct for the register
mechanism, _however_ it is dead code and will never be called for
b43legacy. So I'd suggest you just do something like this (manually-hacked patch):

Index: wireless-testing/drivers/net/wireless/b43legacy/xmit.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43legacy/xmit.c
+++ wireless-testing/drivers/net/wireless/b43legacy/xmit.c
@@ -629,7 +629,7 @@ void b43legacy_handle_hwtxstatus(struct
.......
+ 	tmp <<= 1;
.......
        status.pm_indicated = !!(tmp & 0x80);
        status.intermediate = !!(tmp & 0x40);
        status.for_ampdu = !!(tmp & 0x20);
        status.acked = !!(tmp & 0x02);
 
        b43legacy_handle_txstatus(dev, &status);
 }


Just leave handle_irq_transmit_status as is.


-- 
Greetings Michael.