Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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 ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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.
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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 signature.asc Description: This is a digitally signed message part ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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
signature.asc
Description: This is a digitally signed message part
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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 & 0x0001))
+ 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 & 0x);
stat.phy_stat = ((v1 & 0x00FF) >> 16);
tmp = (v0 & 0x);
+ 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
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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 signature.asc Description: This is a digitally signed message part ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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. ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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. ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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 ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: [PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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. ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
[PATCH] b43legacy: Fix failure in rate-adjustment mechanism
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 <[EMAIL PROTECTED]>
Cc: Stable <[EMAIL PROTECTED]> [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 & 0x0001))
+ 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);
}
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
