Re: [PATCH v5.18] ath9k: Save rate counts before clearing tx status area

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Peter Seiderer <ps.report@gmx.net> writes:

> Hello Toke,
>
> On Sat,  2 Apr 2022 14:27:51 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> The ieee80211_tx_info_clear_status() helper also clears the rate counts, so
>> we should restore them after clearing. However, we can get rid of the
>> existing clearing of the counts of invalid rates. Rearrange the code a bit
>> so the order fits the indexes, and so the setting of the count to
>> hw->max_rate_tries on underrun is not immediately overridden.
>> 
>> Cc: stable@vger.kernel.org
>> Reported-by: Peter Seiderer <ps.report@gmx.net>
>> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>  drivers/net/wireless/ath/ath9k/xmit.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> index cbcf96ac303e..ac7efecff29c 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -2551,16 +2551,19 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>>  	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
>>  	struct ieee80211_hw *hw = sc->hw;
>>  	struct ath_hw *ah = sc->sc_ah;
>> -	u8 i, tx_rateindex;
>> +	u8 i, tx_rateindex, tries[IEEE80211_TX_MAX_RATES];
>> +
>> +	tx_rateindex = ts->ts_rateindex;
>> +	WARN_ON(tx_rateindex >= hw->max_rates);
>> +
>> +	for (i = 0; i < tx_rateindex; i++)
>> +		tries[i] = tx_info->status.rates[i].count;
>>  
>>  	ieee80211_tx_info_clear_status(tx_info);
>>  
>>  	if (txok)
>>  		tx_info->status.ack_signal = ts->ts_rssi;
>>  
>> -	tx_rateindex = ts->ts_rateindex;
>> -	WARN_ON(tx_rateindex >= hw->max_rates);
>> -
>>  	if (tx_info->flags & IEEE80211_TX_CTL_AMPDU) {
>>  		tx_info->flags |= IEEE80211_TX_STAT_AMPDU;
>>  
>> @@ -2569,6 +2572,14 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>>  	tx_info->status.ampdu_len = nframes;
>>  	tx_info->status.ampdu_ack_len = nframes - nbad;
>>  
>> +	for (i = 0; i < tx_rateindex; i++)
>> +		tx_info->status.rates[i].count = tries[i];
>> +
>> +	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
>> +
>> +	for (i = tx_rateindex + 1; i < hw->max_rates; i++)
>> +		tx_info->status.rates[i].idx = -1;
>> +
>>  	if ((ts->ts_status & ATH9K_TXERR_FILT) == 0 &&
>>  	    (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0) {
>>  		/*
>> @@ -2591,12 +2602,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>>  				hw->max_rate_tries;
>>  	}
>
> The full lines above read:
>
> 2597                 if (unlikely(ts->ts_flags & (ATH9K_TX_DATA_UNDERRUN |
> 2598                                              ATH9K_TX_DELIM_UNDERRUN)) &&
> 2599                     ieee80211_is_data(hdr->frame_control) && 
> 2600                     ah->tx_trig_level >= sc->sc_ah->config.max_txtrig_level     )
> 2601                         tx_info->status.rates[tx_rateindex].count =
> 2602                                 hw->max_rate_tries;
> 2603         }
>
> So this patch fixes by drive-by a overwrite of
> tx_info->status.rates[tx_rateindex].count...

Yeah, that was intentional; the setting of
tx_info->status.rates[tx_rateindex].count you quoted never had any
effect, which I'm assuming is not deliberate :)

>>  
>> -	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
>> -		tx_info->status.rates[i].count = 0;
>> -		tx_info->status.rates[i].idx = -1;
>> -	}
>> -
>> -	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
>>  }
>>  
>>  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)
>
> Otherwise looks good ;-), would like to give a Reviewed-by/Tested-by but still
> affected by the underlying ieee80211_tx_info status vs. rate_driver_data overwrite
> as mentioned in the other thread (see [1])...

No worries, I'll respin with a fix for that as well (as soon as I figure
out the right way to fix it), so just wait until v2 and give that a spin
instead :)

-Toke