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

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

[Toke Høiland-Jørgensen]

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

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

[Johannes Berg]

On Sat, 2022-04-02 at 14:27 +0200, Toke Høiland-Jørgensen wrote:
> 
> @@ -2591,12 +2602,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>  				hw->max_rate_tries;
>  	}
>  
> -	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {

might want to drop that blank line too :)

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

since there's nothing else.

johannes

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

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Sat, 2022-04-02 at 14:27 +0200, Toke Høiland-Jørgensen wrote:
>> 
>> @@ -2591,12 +2602,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>>  				hw->max_rate_tries;
>>  	}
>>  
>> -	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
>
> might want to drop that blank line too :)
>
>> -		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;
>>  }
>
> since there's nothing else.

Hmm, fair point; Kalle, I don't suppose I could trouble you for a fixup
when committing? :)

-Toke

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

[Kalle Valo]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Johannes Berg <johannes@sipsolutions.net> writes:
>
>> On Sat, 2022-04-02 at 14:27 +0200, Toke Høiland-Jørgensen wrote:
>>> 
>>> @@ -2591,12 +2602,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>>>  				hw->max_rate_tries;
>>>  	}
>>>  
>>> -	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
>>
>> might want to drop that blank line too :)
>>
>>> -		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;
>>>  }
>>
>> since there's nothing else.
>
> Hmm, fair point; Kalle, I don't suppose I could trouble you for a fixup
> when committing? :)

Sorry, editing the diff is more difficult for me. It would be easier if
you could submit v2.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>
>>> On Sat, 2022-04-02 at 14:27 +0200, Toke Høiland-Jørgensen wrote:
>>>> 
>>>> @@ -2591,12 +2602,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>>>>  				hw->max_rate_tries;
>>>>  	}
>>>>  
>>>> -	for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
>>>
>>> might want to drop that blank line too :)
>>>
>>>> -		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;
>>>>  }
>>>
>>> since there's nothing else.
>>
>> Hmm, fair point; Kalle, I don't suppose I could trouble you for a fixup
>> when committing? :)
>
> Sorry, editing the diff is more difficult for me. It would be easier if
> you could submit v2.

Alright, no worries, can do. Seems we may need more changes anyway, so
will wait for the results of Peter's tests before submitting v2...

-Toke

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

[Greg KH]

On Sat, Apr 02, 2022 at 02:27:51PM +0200, Toke Høiland-Jørgensen 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(-)

What is the git commit id of this change in Linus's tree?

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

[Toke Høiland-Jørgensen]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Sat, Apr 02, 2022 at 02:27:51PM +0200, Toke Høiland-Jørgensen 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(-)
>
> What is the git commit id of this change in Linus's tree?

You mean the commit referred to in the Fixes: tag, right? That's not in
Linus' tree yet, it's a follow-up to a commit that was merged into the
wireless tree yesterday and marked for stable, so the two commits should
be added to stable together once they do hit Linus' tree.

I forgot to add the stable Cc when sending out the previous patch, so
Kalle added it when committing; so I guess you haven't seen that one? :)

-Toke

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

[Greg KH]

On Sat, Apr 02, 2022 at 03:14:53PM +0200, Toke Høiland-Jørgensen wrote:
> Greg KH <gregkh@linuxfoundation.org> writes:
> 
> > On Sat, Apr 02, 2022 at 02:27:51PM +0200, Toke Høiland-Jørgensen 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(-)
> >
> > What is the git commit id of this change in Linus's tree?
> 
> You mean the commit referred to in the Fixes: tag, right? That's not in
> Linus' tree yet, it's a follow-up to a commit that was merged into the
> wireless tree yesterday and marked for stable, so the two commits should
> be added to stable together once they do hit Linus' tree.
> 
> I forgot to add the stable Cc when sending out the previous patch, so
> Kalle added it when committing; so I guess you haven't seen that one? :)

Ah, sorry, I thought this was a request to add a specific patch to the
stable tree.  Nevermind, sorry for the noise.

greg k-h

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

[Peter Seiderer]

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...

>  
> -	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])...

Regards,
Peter


[1] https://lore.kernel.org/linux-wireless/20220404130453.5ec6e045@gmx.net/

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

[Toke Høiland-Jørgensen]

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