Hello Toke,
On Mon, 4 Apr 2022 20:11: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 and
> the driver-private part of struct ieee80211_tx_info, so using it breaks
> quite a few other things. So back out of using it, and instead define a
> ath-internal helper that only clears the area between the
> status_driver_data and the rates info. Combined with moving the
> ath_frame_info struct to status_driver_data, this avoids clearing anything
> we shouldn't be, and so we can keep the existing code for handling the rate
> information.
>
> While fixing this I also noticed that the setting of
> tx_info->status.rates[tx_rateindex].count on hardware underrun errors was
> always immediately overridden by the normal setting of the same fields, so
> rearrange the code so that the underrun detection actually takes effect.
>
> The new helper could be generalised to a 'memset_between()' helper, but
> leave it as a driver-internal helper for now since this needs to go to
> stable.
>
> 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 | 30 ++++++++++++++++++---------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index cbcf96ac303e..db83cc4ba810 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -141,8 +141,8 @@ static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
> {
> struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
> BUILD_BUG_ON(sizeof(struct ath_frame_info) >
> - sizeof(tx_info->rate_driver_data));
> - return (struct ath_frame_info *) &tx_info->rate_driver_data[0];
> + sizeof(tx_info->status.status_driver_data));
> + return (struct ath_frame_info *) &tx_info->status.status_driver_data[0];
> }
Would be too easy if all locations would use get_frame_info()..., at least one location
in drivers/net/wireless/ath/ath9k/main.c uses direct access:
841 txinfo = IEEE80211_SKB_CB(bf->bf_mpdu);
842 fi = (struct ath_frame_info *)&txinfo->rate_driver_data[0];
843 if (fi->keyix == keyix)
844 return true;
Regards,
Peter
>
> static void ath_send_bar(struct ath_atx_tid *tid, u16 seqno)
> @@ -2542,6 +2542,16 @@ static void ath_tx_complete_buf(struct ath_softc *sc, struct ath_buf *bf,
> spin_unlock_irqrestore(&sc->tx.txbuflock, flags);
> }
>
> +static void ath_clear_tx_status(struct ieee80211_tx_info *tx_info)
> +{
> + void *ptr = &tx_info->status;
> +
> + memset(ptr + sizeof(tx_info->status.rates), 0,
> + sizeof(tx_info->status) -
> + sizeof(tx_info->status.rates) -
> + sizeof(tx_info->status.status_driver_data));
> +}
> +
> static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> struct ath_tx_status *ts, int nframes, int nbad,
> int txok)
> @@ -2553,7 +2563,7 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> struct ath_hw *ah = sc->sc_ah;
> u8 i, tx_rateindex;
>
> - ieee80211_tx_info_clear_status(tx_info);
> + ath_clear_tx_status(tx_info);
>
> if (txok)
> tx_info->status.ack_signal = ts->ts_rssi;
> @@ -2569,6 +2579,13 @@ 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;
>
> + 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].count = 0;
> + tx_info->status.rates[i].idx = -1;
> + }
> +
> if ((ts->ts_status & ATH9K_TXERR_FILT) == 0 &&
> (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) == 0) {
> /*
> @@ -2590,13 +2607,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> tx_info->status.rates[tx_rateindex].count =
> 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)