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 <[email protected]>
---
--- 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);
--
> 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
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
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 <[email protected]>
> ---
> --- 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