From: Toke Høiland-Jørgensen <[email protected]>
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: [email protected]
Reported-by: Peter Seiderer <[email protected]>
Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
---
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
Greg KH <[email protected]> writes:
> On Sat, Apr 02, 2022 at 02:27:51PM +0200, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <[email protected]>
>>
>> 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: [email protected]
>> Reported-by: Peter Seiderer <[email protected]>
>> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
>> Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
>> ---
>> 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
On Sat, Apr 02, 2022 at 02:27:51PM +0200, Toke H?iland-J?rgensen wrote:
> From: Toke H?iland-J?rgensen <[email protected]>
>
> 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: [email protected]
> Reported-by: Peter Seiderer <[email protected]>
> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
> Signed-off-by: Toke H?iland-J?rgensen <[email protected]>
> ---
> 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?
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
Hello Toke,
On Sat, 2 Apr 2022 14:27:51 +0200, Toke Høiland-Jørgensen <[email protected]> wrote:
> From: Toke Høiland-Jørgensen <[email protected]>
>
> 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: [email protected]
> Reported-by: Peter Seiderer <[email protected]>
> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
> Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
> ---
> 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/[email protected]/
Peter Seiderer <[email protected]> writes:
> Hello Toke,
>
> On Sat, 2 Apr 2022 14:27:51 +0200, Toke Høiland-Jørgensen <[email protected]> wrote:
>
>> From: Toke Høiland-Jørgensen <[email protected]>
>>
>> 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: [email protected]
>> Reported-by: Peter Seiderer <[email protected]>
>> Fixes: 037250f0a45c ("ath9k: Properly clear TX status area before reporting to mac80211")
>> Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
>> ---
>> 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