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

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

[Arend van Spriel]

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

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

[Arend van Spriel]

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 <sgruszka@redhat.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Alwin Beukers <alwin@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 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

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

[Arend van Spriel]

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 <pieterpg@broadcom.com>
Reviewed-by: Alwin Beukers <alwin@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 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

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

[Jonathan Nieder]

From: Arend van Spriel <arend@broadcom.com>
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 <sgruszka@redhat.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Alwin Beukers <alwin@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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

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

[Ben Hutchings]

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

On Wed, 2012-05-16 at 17:28 -0500, Jonathan Nieder wrote:
> From: Arend van Spriel <arend@broadcom.com>
> 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

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

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

[Greg KH]

On Wed, May 16, 2012 at 05:28:25PM -0500, Jonathan Nieder wrote:
> From: Arend van Spriel <arend@broadcom.com>
> 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 <sgruszka@redhat.com>
> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Reviewed-by: Alwin Beukers <alwin@broadcom.com>
> Signed-off-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> 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

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

[Jonathan Nieder]

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

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

[Arend van Spriel]

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

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

[Jonathan Nieder]

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

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

[Jonathan Nieder]

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

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

[Ben Hutchings]

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

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.

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