[PATCH] ath9k: fix an issue in ieee80211_tx_info count field management

[PATCH] ath9k: fix an issue in ieee80211_tx_info count field management

[Lorenzo Bianconi]

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);

--

Re: [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management

[Felix Fietkau]

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

Re: [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management

[Lorenzo Bianconi]

> 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

Re: [PATCH] ath9k: fix an issue in ieee80211_tx_info count field management

[Luis R. Rodriguez]

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