Peter Seiderer <ps.report@xxxxxxx> writes:
> Hello Toke,
>
> On Sat, 2 Apr 2022 14:27:51 +0200, Toke Høiland-Jørgensen <toke@xxxxxxx> wrote:
>
>> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
>> Reported-by: Peter Seiderer <ps.report@xxxxxxx>
>> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
>> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>> ---
>> 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
