Subject: [PATCH] ath9k: Fix bug in the way "bf_tx_aborted" of struct ath_buf is used

This bug was introduced by the following commit

Author: Vasanthakumar Thiagarajan <[email protected]>
Date: Thu Apr 15 17:38:46 2010 -0400

ath9k: Remove ATH9K_TX_SW_ABORTED and introduce a bool for this purpose

Wrong buffer is checked for bf_tx_aborted field in ath_tx_num_badfrms(),
this may result in a rate scaling with wrong feedback (number
of unacked frames in this case). It is the last one in the chain
of buffers for an aggregate frame that should be checked.

Also it misses the initialization of this field in the buffer,
this may lead to a situation where we stop the sw retransmission
of failed subframes associated to this buffer.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath9k/xmit.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 3db1917..c575552 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1728,6 +1728,8 @@ static int ath_tx_setup_buffer(struct ieee80211_hw *hw, struct ath_buf *bf,
} else
bf->bf_isnullfunc = false;

+ bf->bf_tx_aborted = false;
+
return 0;
}

@@ -1989,7 +1991,7 @@ static int ath_tx_num_badfrms(struct ath_softc *sc, struct ath_buf *bf,
int nbad = 0;
int isaggr = 0;

- if (bf->bf_tx_aborted)
+ if (bf->bf_lastbf->bf_tx_aborted)
return 0;

isaggr = bf_isaggr(bf);
--
1.7.0.4



2010-05-27 03:10:55

by Luis R. Rodriguez

[permalink] [raw]
Subject: Re: [PATCH] ath9k: Fix bug in the way "bf_tx_aborted" of struct ath_buf is used

On Wed, May 26, 2010 at 7:06 PM, Vasanthakumar Thiagarajan
<[email protected]> wrote:
> This bug was introduced by the following commit
>
>        Author: Vasanthakumar Thiagarajan <[email protected]>
>        Date:   Thu Apr 15 17:38:46 2010 -0400
>
>        ath9k: Remove ATH9K_TX_SW_ABORTED and introduce a bool for this purpose
>
> Wrong buffer is checked for bf_tx_aborted field in ath_tx_num_badfrms(),
> this may result in a rate scaling with wrong feedback (number
> of unacked frames in this case). It is the last one in the chain
> of buffers for an aggregate frame that should be checked.
>
> Also it misses the initialization of this field in the buffer,
> this may lead to a situation where we stop the sw retransmission
> of failed subframes associated to this buffer.
>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
> ---

FWIW, for those who want to test this, I've applied this patch into
the compat-wireless linux-next-pending/ directory. I've also kicked
out a new linux-next based compat-wireless tarball with the
pending+crap patches applied and fixed today's new compile issue
introduced by:

Author: Alexey Dobriyan <[email protected]>
Date: Mon May 24 14:33:03 2010 -0700

kernel-wide: replace USHORT_MAX, SHORT_MAX and SHORT_MIN with
USHRT_MAX, SHRT_MAX and SHRT_MIN

- C99 knows about USHRT_MAX/SHRT_MAX/SHRT_MIN, not
USHORT_MAX/SHORT_MAX/SHORT_MIN.

- Make SHRT_MIN of type s16, not int, for consistency.

[[email protected]: fix drivers/dma/timb_dma.c]
[[email protected]: fix security/keys/keyring.c]
Signed-off-by: Alexey Dobriyan <[email protected]>
Acked-by: WANG Cong <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>

This fix required a simple patch on compat.git.

The "pc" compat-wireless tarball for today then is:

http://www.orbit-lab.org/kernel/compat-wireless-2.6/compat-wireless-2010-05-26-pc.tar.bz2
sha1sum: 62937a3bec62cc564731b30295b44994504355cf

I've compile and load tested ath9k against 2.6.31-20-generic

Luis