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)
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
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
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.
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
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?
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
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
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/
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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Johannes Berg -
Kalle Valo -
Peter Seiderer -
Toke Høiland-Jørgensen