* [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management
@ 2012-04-05 21:39 Lorenzo Bianconi
2012-04-06 16:19 ` Felix Fietkau
2012-04-12 17:44 ` Luis R. Rodriguez
0 siblings, 2 replies; 4+ messages in thread
From: Lorenzo Bianconi @ 2012-04-05 21:39 UTC (permalink / raw)
To: linux-wireless; +Cc: John Linville, Luis R. Rodriguez
Hi all,
I noticed a possible issue in the status count field management of the
ieee80211_tx_info data structure. In particular, when the AGGR
processing is employed,
ath_tx_complete_aggr() sets status.rates[].count just for the first
frame and not for others belonging to the same burst, leading to wrong
statistic data in the mac80211 debug file system.
Regards
Lorenzo
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
---
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -58,7 +58,7 @@
struct list_head *head, bool internal);
static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
struct ath_tx_status *ts, int nframes, int nbad,
- int txok);
+ int txok, bool update_info);
static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
int seqno);
static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
@@ -542,9 +542,12 @@
if (rc_update && (acked_cnt == 1 || txfail_cnt == 1)) {
memcpy(tx_info->control.rates, rates, sizeof(rates));
- ath_tx_rc_status(sc, bf, ts, nframes, nbad, txok);
+ ath_tx_rc_status(sc, bf, ts, nframes, nbad,
+ txok, true);
rc_update = false;
- }
+ } else
+ ath_tx_rc_status(sc, bf, ts, nframes, nbad,
+ txok, false);
ath_tx_complete_buf(sc, bf, txq, &bf_head, ts,
!txfail);
@@ -2068,7 +2071,7 @@
static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
struct ath_tx_status *ts, int nframes, int nbad,
- int txok)
+ int txok, bool update_info)
{
struct sk_buff *skb = bf->bf_mpdu;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
@@ -2083,16 +2086,16 @@
tx_rateindex = ts->ts_rateindex;
WARN_ON(tx_rateindex >= hw->max_rates);
- if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
+ if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) && update_info) {
tx_info->flags |= IEEE80211_TX_STAT_AMPDU;
BUG_ON(nbad > nframes);
+ tx_info->status.ampdu_len = nframes;
+ tx_info->status.ampdu_ack_len = nframes - nbad;
}
- tx_info->status.ampdu_len = nframes;
- tx_info->status.ampdu_ack_len = nframes - nbad;
if ((ts->ts_status & ATH9K_TXERR_FILT) == 0 &&
- (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0) {
+ (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0 && update_info) {
/*
* If an underrun error is seen assume it as an excessive
* retry only if max frame trigger level has been reached
@@ -2134,7 +2137,7 @@
txq->axq_ampdu_depth--;
if (!bf_isampdu(bf)) {
- ath_tx_rc_status(sc, bf, ts, 1, txok ? 0 : 1, txok);
+ ath_tx_rc_status(sc, bf, ts, 1, txok ? 0 : 1, txok, true);
ath_tx_complete_buf(sc, bf, txq, bf_head, ts, txok);
} else
ath_tx_complete_aggr(sc, txq, bf, bf_head, ts, txok, true);
--
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management
2012-04-05 21:39 [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management Lorenzo Bianconi
@ 2012-04-06 16:19 ` Felix Fietkau
2012-04-06 18:32 ` Lorenzo Bianconi
2012-04-12 17:44 ` Luis R. Rodriguez
1 sibling, 1 reply; 4+ messages in thread
From: Felix Fietkau @ 2012-04-06 16:19 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-wireless, John Linville, Luis R. Rodriguez
On 2012-04-05 11:39 PM, Lorenzo Bianconi wrote:
> Hi all,
>
> I noticed a possible issue in the status count field management of the
> ieee80211_tx_info data structure. In particular, when the AGGR
> processing is employed,
> ath_tx_complete_aggr() sets status.rates[].count just for the first
> frame and not for others belonging to the same burst, leading to wrong
> statistic data in the mac80211 debug file system.
The driver behavior is OK and mac80211 needs to be changed. The tx
status code that counts retries needs to check if AMPDU CTL and STATUS
flags match and stop counting for packets where they don't match.
- Felix
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management
2012-04-06 16:19 ` Felix Fietkau
@ 2012-04-06 18:32 ` Lorenzo Bianconi
0 siblings, 0 replies; 4+ messages in thread
From: Lorenzo Bianconi @ 2012-04-06 18:32 UTC (permalink / raw)
To: Felix Fietkau; +Cc: linux-wireless, John Linville, Luis R. Rodriguez
> On 2012-04-05 11:39 PM, Lorenzo Bianconi wrote:
>> Hi all,
>>
>> I noticed a possible issue in the status count field management of the
>> ieee80211_tx_info data structure. In particular, when the AGGR
>> processing is employed,
>> ath_tx_complete_aggr() sets status.rates[].count just for the first
>> frame and not for others belonging to the same burst, leading to wrong
>> statistic data in the mac80211 debug file system.
> The driver behavior is OK and mac80211 needs to be changed. The tx
> status code that counts retries needs to check if AMPDU CTL and STATUS
> flags match and stop counting for packets where they don't match.
>
> - Felix
Ack. I will send another patch in order to fix the issue in mac80211.
Regards
Lorenzo
--
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management
2012-04-05 21:39 [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management Lorenzo Bianconi
2012-04-06 16:19 ` Felix Fietkau
@ 2012-04-12 17:44 ` Luis R. Rodriguez
1 sibling, 0 replies; 4+ messages in thread
From: Luis R. Rodriguez @ 2012-04-12 17:44 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-wireless, John Linville
On Thu, Apr 05, 2012 at 11:39:57PM +0200, Lorenzo Bianconi wrote:
> Hi all,
>
> I noticed a possible issue in the status count field management of the
> ieee80211_tx_info data structure. In particular, when the AGGR
> processing is employed,
> ath_tx_complete_aggr() sets status.rates[].count just for the first
> frame and not for others belonging to the same burst, leading to wrong
> statistic data in the mac80211 debug file system.
Thanks for the patch! Unfortunatey this patch is not well formed.
Please review exisitng patches via git log. Also please include in the
commit log the effects of not applying the patch Vs actually applying
since you are indicating this is fix. This is important for considering
patches to be either propagated or not to the stable series of the kernel.
In this case this does not seem like a stable fix at all since it seems
you are indicating this is only a debugfs fix.
Patch review below.
> Regards
>
> Lorenzo
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
> ---
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -58,7 +58,7 @@
> struct list_head *head, bool internal);
> static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> struct ath_tx_status *ts, int nframes, int nbad,
> - int txok);
> + int txok, bool update_info);
> static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
> int seqno);
> static struct ath_buf *ath_tx_setup_buffer(struct ath_softc *sc,
> @@ -542,9 +542,12 @@
>
> if (rc_update && (acked_cnt == 1 || txfail_cnt == 1)) {
> memcpy(tx_info->control.rates, rates, sizeof(rates));
> - ath_tx_rc_status(sc, bf, ts, nframes, nbad, txok);
> + ath_tx_rc_status(sc, bf, ts, nframes, nbad,
> + txok, true);
> rc_update = false;
> - }
> + } else
> + ath_tx_rc_status(sc, bf, ts, nframes, nbad,
> + txok, false);
>
> ath_tx_complete_buf(sc, bf, txq, &bf_head, ts,
> !txfail);
> @@ -2068,7 +2071,7 @@
>
> static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> struct ath_tx_status *ts, int nframes, int nbad,
> - int txok)
> + int txok, bool update_info)
> {
> struct sk_buff *skb = bf->bf_mpdu;
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> @@ -2083,16 +2086,16 @@
> tx_rateindex = ts->ts_rateindex;
> WARN_ON(tx_rateindex >= hw->max_rates);
>
> - if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
> + if ((tx_info->flags & IEEE80211_TX_CTL_AMPDU) && update_info) {
> tx_info->flags |= IEEE80211_TX_STAT_AMPDU;
You are changing the behaviour here to only add the
tx_info->flags |= IEEE80211_TX_STAT_AMPDU now only when
update_info is set. What effects does this have from
previous behaviour ?
>
> BUG_ON(nbad > nframes);
> + tx_info->status.ampdu_len = nframes;
> + tx_info->status.ampdu_ack_len = nframes - nbad;
Same here.
> }
> - tx_info->status.ampdu_len = nframes;
> - tx_info->status.ampdu_ack_len = nframes - nbad;
>
> if ((ts->ts_status & ATH9K_TXERR_FILT) == 0 &&
> - (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0) {
> + (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0 && update_info) {
and here
> /*
> * If an underrun error is seen assume it as an excessive
> * retry only if max frame trigger level has been reached
> @@ -2134,7 +2137,7 @@
> txq->axq_ampdu_depth--;
>
> if (!bf_isampdu(bf)) {
> - ath_tx_rc_status(sc, bf, ts, 1, txok ? 0 : 1, txok);
> + ath_tx_rc_status(sc, bf, ts, 1, txok ? 0 : 1, txok, true);
> ath_tx_complete_buf(sc, bf, txq, bf_head, ts, txok);
> } else
> ath_tx_complete_aggr(sc, txq, bf, bf_head, ts, txok, true);
>
> --
Luis
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-04-12 17:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-05 21:39 [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management Lorenzo Bianconi
2012-04-06 16:19 ` Felix Fietkau
2012-04-06 18:32 ` Lorenzo Bianconi
2012-04-12 17:44 ` Luis R. Rodriguez
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.