2012-02-23 17:38:38

by Arend van Spriel

[permalink] [raw]
Subject: [PATCH for 3.3 0/2] brcm80211: fix endless A-MPDU retries

Hi John,

The first patch has already been taken in wireless-next repo. However,
as people see the issue with 3.2 and 3.3 kernels it should go to wireless
repository as well.

Fix endless retries that became more obvious upon implementing the
mac80211 flush callback and more so after adding the WARN_ON_ONCE
by Stanislaw Gruszka (https://bugzilla.kernel.org/show_bug.cgi?id=42576).

These second patch removes error print to limit the mess printed in the
log when an A-MPDU transmission times out on the block-ack.

Arend van Spriel (2):
brcm80211: smac: fix endless retry of A-MPDU transmissions
brcm80211: smac: only print block-ack timeout message at trace level

drivers/net/wireless/brcm80211/brcmsmac/ampdu.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)

--
1.7.5.4




2012-02-23 17:38:38

by Arend van Spriel

[permalink] [raw]
Subject: [PATCH 1/2] brcm80211: smac: fix endless retry of A-MPDU transmissions

The A-MPDU code checked against a retry limit, but it was using
the wrong variable to do so. This patch fixes this to assure
proper retry mechanism.

This problem had a side-effect causing the mac80211 flush callback
to remain waiting forever as well. That side effect has been fixed
by commit by Stanislaw Gruszka:

commit f96b08a7e6f69c0f0a576554df3df5b1b519c479
Date: Tue Jan 17 12:38:50 2012 +0100

brcmsmac: fix tx queue flush infinite loop

Reference:
https://bugzilla.kernel.org/show_bug.cgi?id=42576

Cc: Stanislaw Gruszka <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Alwin Beukers <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
drivers/net/wireless/brcm80211/brcmsmac/ampdu.c | 6 +-----
1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
index 90911ee..9265226 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
@@ -1051,17 +1051,13 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb,
}
/* either retransmit or send bar if ack not recd */
if (!ack_recd) {
- struct ieee80211_tx_rate *txrate =
- tx_info->status.rates;
- if (retry && (txrate[0].count < (int)retry_limit)) {
+ if (retry && (ini->txretry[index] < (int)retry_limit)) {
ini->txretry[index]++;
ini->tx_in_transit--;
/*
* Use high prededence for retransmit to
* give some punch
*/
- /* brcms_c_txq_enq(wlc, scb, p,
- * BRCMS_PRIO_TO_PREC(tid)); */
brcms_c_txq_enq(wlc, scb, p,
BRCMS_PRIO_TO_HI_PREC(tid));
} else {
--
1.7.5.4



2012-02-23 17:38:38

by Arend van Spriel

[permalink] [raw]
Subject: [PATCH 2/2] brcm80211: smac: only print block-ack timeout message at trace level

In regular use block-ack timeouts can happen so it does not make
sense to fill the log with these messages.

Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Alwin Beukers <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
drivers/net/wireless/brcm80211/brcmsmac/ampdu.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
index 9265226..30b5887 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
@@ -1070,9 +1070,9 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb,
IEEE80211_TX_STAT_AMPDU_NO_BACK;
skb_pull(p, D11_PHY_HDR_LEN);
skb_pull(p, D11_TXH_LEN);
- wiphy_err(wiphy, "%s: BA Timeout, seq %d, in_"
- "transit %d\n", "AMPDU status", seq,
- ini->tx_in_transit);
+ BCMMSG(wiphy,
+ "BA Timeout, seq %d, in_transit %d\n",
+ seq, ini->tx_in_transit);
ieee80211_tx_status_irqsafe(wlc->pub->ieee_hw,
p);
}
--
1.7.5.4



2012-05-19 08:49:26

by Arend van Spriel

[permalink] [raw]
Subject: Re: [PATCH 3.2.y, 3.3.y] brcm80211: smac: fix endless retry of A-MPDU transmissions

On 05/18/2012 10:51 PM, Jonathan Nieder wrote:
> Hi Greg,
>
> Greg KH wrote:
>> On Wed, May 16, 2012 at 05:28:25PM -0500, Jonathan Nieder wrote:
>
>>> commit 5e379203c7788b7af01150bfadbc74d2797a2ef4 upstream.
> [...]
>> It doesn't apply to the 3.3-stable tree, so it's a bit hard for me to
>> apply it...
>
> My bad --- looks like it was already applied during the 3.3 cycle as
> v3.3~19^2^2~7 (commit 85091fc0a756). Sorry I missed that before.
>
> Thanks,
> Jonathan
>

Yes. The patch was posted upstream with Cc to stable. As it did not
apply to earlier kernels it was marked for 3.3 stable only. Jonathan
ported the original patch for the earlier kernels.

Gr. AvS


2012-05-17 00:06:03

by Ben Hutchings

[permalink] [raw]
Subject: Re: [PATCH 3.2.y, 3.3.y] brcm80211: smac: fix endless retry of A-MPDU transmissions

On Wed, 2012-05-16 at 17:28 -0500, Jonathan Nieder wrote:
> From: Arend van Spriel <[email protected]>
> Date: Thu, 9 Feb 2012 21:08:58 +0100
>
> commit 5e379203c7788b7af01150bfadbc74d2797a2ef4 upstream.
[...]

Added to the queue for 3.2.y.

Ben.

--
Ben Hutchings
Every program is either trivial or else contains at least one bug


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

2012-05-16 22:28:33

by Jonathan Nieder

[permalink] [raw]
Subject: [PATCH 3.2.y, 3.3.y] brcm80211: smac: fix endless retry of A-MPDU transmissions

From: Arend van Spriel <[email protected]>
Date: Thu, 9 Feb 2012 21:08:58 +0100

commit 5e379203c7788b7af01150bfadbc74d2797a2ef4 upstream.

The A-MPDU code checked against a retry limit, but it was using
the wrong variable to do so. This patch fixes this to assure
proper retry mechanism.

This problem had a side-effect causing the mac80211 flush callback
to remain waiting forever as well. That side effect has been fixed
by commit by Stanislaw Gruszka:

commit f96b08a7e6f69c0f0a576554df3df5b1b519c479
Date: Tue Jan 17 12:38:50 2012 +0100

brcmsmac: fix tx queue flush infinite loop

Reference:
https://bugzilla.kernel.org/show_bug.cgi?id=42576

Cc: Stanislaw Gruszka <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Alwin Beukers <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
Signed-off-by: Jonathan Nieder <[email protected]>
---
Hi Ben and Greg,

Please consider

5e379203c778 brcm80211: smac: fix endless retry of A-MPDU
transmissions

for application to the 3.2.y and 3.3.y stable trees. It is the real
fix to the bug worked around by f96b08a7e6f6 (brcmsmac: fix tx queue
flush infinite loop, 2012-01-17). The remaining symptom after that
workaround was an assertion failure (WARNING at
drivers/net/wireless/brcm80211/brcmsmac/main.c:8241).

Touko Korpela (cc-ed) tested the patch against 3.2.17 + "brcm80211:
smac: pass missing argument to 'brcms_b_mute'" and found it to
work[1]:

> Patch works, warning is gone after couple of hours of use. I let you
> know if situation changes.

Thanks,
Jonathan

[1] http://bugs.debian.org/672891

drivers/net/wireless/brcm80211/brcmsmac/ampdu.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
index 7f27dbdb6b60..051586275f14 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/ampdu.c
@@ -1053,17 +1053,13 @@ brcms_c_ampdu_dotxstatus_complete(struct ampdu_info *ampdu, struct scb *scb,
}
/* either retransmit or send bar if ack not recd */
if (!ack_recd) {
- struct ieee80211_tx_rate *txrate =
- tx_info->status.rates;
- if (retry && (txrate[0].count < (int)retry_limit)) {
+ if (retry && (ini->txretry[index] < (int)retry_limit)) {
ini->txretry[index]++;
ini->tx_in_transit--;
/*
* Use high prededence for retransmit to
* give some punch
*/
- /* brcms_c_txq_enq(wlc, scb, p,
- * BRCMS_PRIO_TO_PREC(tid)); */
brcms_c_txq_enq(wlc, scb, p,
BRCMS_PRIO_TO_HI_PREC(tid));
} else {
--
1.7.10.2


2012-05-18 19:58:14

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 3.2.y, 3.3.y] brcm80211: smac: fix endless retry of A-MPDU transmissions

On Wed, May 16, 2012 at 05:28:25PM -0500, Jonathan Nieder wrote:
> From: Arend van Spriel <[email protected]>
> Date: Thu, 9 Feb 2012 21:08:58 +0100
>
> commit 5e379203c7788b7af01150bfadbc74d2797a2ef4 upstream.
>
> The A-MPDU code checked against a retry limit, but it was using
> the wrong variable to do so. This patch fixes this to assure
> proper retry mechanism.
>
> This problem had a side-effect causing the mac80211 flush callback
> to remain waiting forever as well. That side effect has been fixed
> by commit by Stanislaw Gruszka:
>
> commit f96b08a7e6f69c0f0a576554df3df5b1b519c479
> Date: Tue Jan 17 12:38:50 2012 +0100
>
> brcmsmac: fix tx queue flush infinite loop
>
> Reference:
> https://bugzilla.kernel.org/show_bug.cgi?id=42576
>
> Cc: Stanislaw Gruszka <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Alwin Beukers <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> Signed-off-by: John W. Linville <[email protected]>
> Signed-off-by: Jonathan Nieder <[email protected]>
> ---
> Hi Ben and Greg,
>
> Please consider
>
> 5e379203c778 brcm80211: smac: fix endless retry of A-MPDU
> transmissions
>
> for application to the 3.2.y and 3.3.y stable trees. It is the real
> fix to the bug worked around by f96b08a7e6f6 (brcmsmac: fix tx queue
> flush infinite loop, 2012-01-17). The remaining symptom after that
> workaround was an assertion failure (WARNING at
> drivers/net/wireless/brcm80211/brcmsmac/main.c:8241).
>
> Touko Korpela (cc-ed) tested the patch against 3.2.17 + "brcm80211:
> smac: pass missing argument to 'brcms_b_mute'" and found it to
> work[1]:

It doesn't apply to the 3.3-stable tree, so it's a bit hard for me to
apply it...

thanks,

greg k-h

2012-05-18 20:51:23

by Jonathan Nieder

[permalink] [raw]
Subject: Re: [PATCH 3.2.y, 3.3.y] brcm80211: smac: fix endless retry of A-MPDU transmissions

Hi Greg,

Greg KH wrote:
> On Wed, May 16, 2012 at 05:28:25PM -0500, Jonathan Nieder wrote:

>> commit 5e379203c7788b7af01150bfadbc74d2797a2ef4 upstream.
[...]
> It doesn't apply to the 3.3-stable tree, so it's a bit hard for me to
> apply it...

My bad --- looks like it was already applied during the 3.3 cycle as
v3.3~19^2^2~7 (commit 85091fc0a756). Sorry I missed that before.

Thanks,
Jonathan

2012-05-19 16:53:24

by Jonathan Nieder

[permalink] [raw]
Subject: Re: [PATCH 3.2.y, 3.3.y] brcm80211: smac: fix endless retry of A-MPDU transmissions

Arend van Spriel wrote:
> On 05/18/2012 10:51 PM, Jonathan Nieder wrote:

>> My bad --- looks like it was already applied during the 3.3 cycle as
>> v3.3~19^2^2~7 (commit 85091fc0a756). Sorry I missed that before.
[...]
> Yes. The patch was posted upstream with Cc to stable. As it did not
> apply to earlier kernels it was marked for 3.3 stable only.

Technically it wasn't cc-ed to stable; it went to wireless-next
for 3.4 and then was fast-tracked through the plain wireless tree
for 3.3[1]. But the effect is basically the same.

Thanks for writing it.

Jonathan

[1] http://thread.gmane.org/gmane.linux.kernel.wireless.general/85902
http://thread.gmane.org/gmane.linux.kernel.wireless.general/85249

2012-11-24 23:14:48

by Jonathan Nieder

[permalink] [raw]
Subject: [3.2.y] Re: [PATCH 2/2] brcm80211: smac: only print block-ack timeout message at trace level

Hi Ben,

Please consider

2b0a53d51b5f brcm80211: smac: only print block-ack timeout message
at trace level

for application to the 3.2.y tree.

It just lowers the priority of a distracting repeated message about a
normal condition:

ieee80211 phy0: AMPDU status: BA Timeout, seq 152, in_transit 0

See <http://bugs.debian.org/674430> for context.

The patch was applied upstream during the 3.3-rc7 cycle. Applying it
should make debugging other problems easier. Thoughts of all kinds
welcome, as always.

Regards,
Jonathan

2012-12-02 03:35:54

by Ben Hutchings

[permalink] [raw]
Subject: Re: [3.2.y] Re: [PATCH 2/2] brcm80211: smac: only print block-ack timeout message at trace level

On Sat, 2012-11-24 at 15:14 -0800, Jonathan Nieder wrote:
> Hi Ben,
>
> Please consider
>
> 2b0a53d51b5f brcm80211: smac: only print block-ack timeout message
> at trace level
>
> for application to the 3.2.y tree.
>
> It just lowers the priority of a distracting repeated message about a
> normal condition:
>
> ieee80211 phy0: AMPDU status: BA Timeout, seq 152, in_transit 0
>
> See <http://bugs.debian.org/674430> for context.
>
> The patch was applied upstream during the 3.3-rc7 cycle. Applying it
> should make debugging other problems easier. Thoughts of all kinds
> welcome, as always.

Added to the queue, thanks.

Ben.

--
Ben Hutchings
It is easier to change the specification to fit the program than vice versa.


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