[PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Toke Høiland-Jørgensen]

The ath9k driver was not properly clearing the status area in the
ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
it was manually filling in fields, which meant that fields introduced later
were left as-is.

Conveniently, mac80211 actually provides a helper to zero out the status
area, so use that to make sure we zero everything.

The last commit touching the driver function writing the status information
seems to have actually been fixing an issue that was also caused by the
area being uninitialised; but it only added clearing of a single field
instead of the whole struct. That is now redundant, though, so revert that
commit and use it as a convenient Fixes tag.

Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index d0caf1de2bde..cbcf96ac303e 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2553,6 +2553,8 @@ 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);
+
 	if (txok)
 		tx_info->status.ack_signal = ts->ts_rssi;
 
@@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
 	}
 
 	tx_info->status.rates[tx_rateindex].count = ts->ts_longretry + 1;
-
-	/* we report airtime in ath_tx_count_airtime(), don't report twice */
-	tx_info->status.tx_time = 0;
 }
 
 static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)
-- 
2.35.1

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Bagas Sanjaya]

On 30/03/22 23.44, Toke Høiland-Jørgensen wrote:
> The ath9k driver was not properly clearing the status area in the
> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
> it was manually filling in fields, which meant that fields introduced later
> were left as-is.
> 
> Conveniently, mac80211 actually provides a helper to zero out the status
> area, so use that to make sure we zero everything.
> 
> The last commit touching the driver function writing the status information
> seems to have actually been fixing an issue that was also caused by the
> area being uninitialised; but it only added clearing of a single field
> instead of the whole struct. That is now redundant, though, so revert that
> commit and use it as a convenient Fixes tag.
> 
> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

No regressions and UBSAN warning [1] reported on dmesg.

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

However, there is something missing. I don't see Cc: stable@vger.kernel.org
trailer in this patch. I think it should, because I reported that this issue
first occurred on v5.17, then still appeared on v5.17.1.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Bagas Sanjaya]

On 31/03/22 12.06, Bagas Sanjaya wrote:
> No regressions and UBSAN warning [1] reported on dmesg.
> 
> Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
> 
> However, there is something missing. I don't see Cc: stable@vger.kernel.org
> trailer in this patch. I think it should, because I reported that this issue
> first occurred on v5.17, then still appeared on v5.17.1.
> 

Oops, missing link for [1].

[1]: https://lore.kernel.org/linux-wireless/87bkxn4kpf.fsf@toke.dk/T/#mc2ef5bbdf51fd4ead05b1115cedd58153675365c

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Kalle Valo]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 30/03/22 23.44, Toke Høiland-Jørgensen wrote:
>> The ath9k driver was not properly clearing the status area in the
>> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
>> it was manually filling in fields, which meant that fields introduced later
>> were left as-is.
>>
>> Conveniently, mac80211 actually provides a helper to zero out the status
>> area, so use that to make sure we zero everything.
>>
>> The last commit touching the driver function writing the status information
>> seems to have actually been fixing an issue that was also caused by the
>> area being uninitialised; but it only added clearing of a single field
>> instead of the whole struct. That is now redundant, though, so revert that
>> commit and use it as a convenient Fixes tag.
>>
>> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> No regressions and UBSAN warning [1] reported on dmesg.
>
> Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
>
> However, there is something missing. I don't see Cc: stable@vger.kernel.org
> trailer in this patch. I think it should, because I reported that this issue
> first occurred on v5.17, then still appeared on v5.17.1.

I can add that during commit.

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

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

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>
>> On 30/03/22 23.44, Toke Høiland-Jørgensen wrote:
>>> The ath9k driver was not properly clearing the status area in the
>>> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
>>> it was manually filling in fields, which meant that fields introduced later
>>> were left as-is.
>>>
>>> Conveniently, mac80211 actually provides a helper to zero out the status
>>> area, so use that to make sure we zero everything.
>>>
>>> The last commit touching the driver function writing the status information
>>> seems to have actually been fixing an issue that was also caused by the
>>> area being uninitialised; but it only added clearing of a single field
>>> instead of the whole struct. That is now redundant, though, so revert that
>>> commit and use it as a convenient Fixes tag.
>>>
>>> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
>>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>
>> No regressions and UBSAN warning [1] reported on dmesg.
>>
>> Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
>>
>> However, there is something missing. I don't see Cc: stable@vger.kernel.org
>> trailer in this patch. I think it should, because I reported that this issue
>> first occurred on v5.17, then still appeared on v5.17.1.
>
> I can add that during commit.

Thanks! And sorry about that, I have gotten so used to the netdev policy
of not including an explicit stable Cc that I totally forgot that this
doesn't apply to the wireless tree.

In any case I think the stable tree autoselection bit should pick it up
from the Fixes tag, though...

-Toke

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Kalle Valo]

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

> Kalle Valo <kvalo@kernel.org> writes:
>
>> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>>
>>> On 30/03/22 23.44, Toke Høiland-Jørgensen wrote:
>>>> The ath9k driver was not properly clearing the status area in the
>>>> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
>>>> it was manually filling in fields, which meant that fields introduced later
>>>> were left as-is.
>>>>
>>>> Conveniently, mac80211 actually provides a helper to zero out the status
>>>> area, so use that to make sure we zero everything.
>>>>
>>>> The last commit touching the driver function writing the status information
>>>> seems to have actually been fixing an issue that was also caused by the
>>>> area being uninitialised; but it only added clearing of a single field
>>>> instead of the whole struct. That is now redundant, though, so revert that
>>>> commit and use it as a convenient Fixes tag.
>>>>
>>>> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
>>>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>>
>>> No regressions and UBSAN warning [1] reported on dmesg.
>>>
>>> Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
>>>
>>> However, there is something missing. I don't see Cc: stable@vger.kernel.org
>>> trailer in this patch. I think it should, because I reported that this issue
>>> first occurred on v5.17, then still appeared on v5.17.1.
>>
>> I can add that during commit.
>
> Thanks! And sorry about that, I have gotten so used to the netdev policy
> of not including an explicit stable Cc that I totally forgot that this
> doesn't apply to the wireless tree.

No worries!

> In any case I think the stable tree autoselection bit should pick it up
> from the Fixes tag, though...

Yeah, I agree. I also only add the Fixes tag to my patches, and let the
stable team to choose if they want the commit or not. But if people ask
to add cc stable I'll do that, it's easy enough as I have the edit
command in my patchwork script :)

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

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

Re: [v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Kalle Valo]

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

> The ath9k driver was not properly clearing the status area in the
> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
> it was manually filling in fields, which meant that fields introduced later
> were left as-is.
> 
> Conveniently, mac80211 actually provides a helper to zero out the status
> area, so use that to make sure we zero everything.
> 
> The last commit touching the driver function writing the status information
> seems to have actually been fixing an issue that was also caused by the
> area being uninitialised; but it only added clearing of a single field
> instead of the whole struct. That is now redundant, though, so revert that
> commit and use it as a convenient Fixes tag.
> 
> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

Patch applied to wireless.git, thanks.

037250f0a45c ath9k: Properly clear TX status area before reporting to mac80211

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220330164409.16645-1-toke@toke.dk/

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

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Peter Seiderer]

Hello Toke,

On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> The ath9k driver was not properly clearing the status area in the
> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
> it was manually filling in fields, which meant that fields introduced later
> were left as-is.
> 
> Conveniently, mac80211 actually provides a helper to zero out the status
> area, so use that to make sure we zero everything.
> 
> The last commit touching the driver function writing the status information
> seems to have actually been fixing an issue that was also caused by the
> area being uninitialised; but it only added clearing of a single field
> instead of the whole struct. That is now redundant, though, so revert that
> commit and use it as a convenient Fixes tag.
> 
> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index d0caf1de2bde..cbcf96ac303e 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -2553,6 +2553,8 @@ 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);
> +

As this also clears the status.rates[].count, see include/net/mac80211.h:

1195 static inline void
1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
1197 {
1198         int i;
1199         
1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
1201                      offsetof(struct ieee80211_tx_info, control.rates));
1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
1203                      offsetof(struct ieee80211_tx_info, driver_rates));
1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
1205         /* clear the rate counts */
1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
1207                 info->status.rates[i].count = 0;
1208         memset_after(&info->status, 0, rates);
1209 }

I would have expected some lines added to restore the count (for the
rates with index < tx_rateindex), e.g. as done in
drivers/net/wireless/ath/ath5k/base.c:

1731         ieee80211_tx_info_clear_status(info);
1732 
1733         for (i = 0; i < ts->ts_final_idx; i++) {
1734                 struct ieee80211_tx_rate *r =
1735                         &info->status.rates[i];
1736 
1737                 r->count = tries[i];
1738         }

In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
the tx_rateindex index (which is often zero in case the first suggested rate
succeeds, but in noisy environment is sometimes > 0)...

>  	if (txok)
>  		tx_info->status.ack_signal = ts->ts_rssi;
>  
> @@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>  	}
>  
>  	tx_info->status.rates[)].count = ts->ts_longretry + 1;
> -
> -	/* we report airtime in ath_tx_count_airtime(), don't report twice */
> -	tx_info->status.tx_time = 0;
>  }
>  
>  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)


And from drivers/net/wireless/ath/ath9k/xmit.c:

2592         for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
2593                 tx_info->status.rates[i].count = 0;
2594                 tx_info->status.rates[i].idx = -1;
2595         }

Line 2593 can be deleted as status.rates[].count is already zeroed through the
ieee80211_tx_info_clear_status() call...

And it should be sufficient to do:

	if (tx_rateindex + 1 < hw->max_rates)
		tx_info->status.rates[tx_rateindex + 1].idx = -1;

Regards,
Peter

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Toke Høiland-Jørgensen]

Peter Seiderer <ps.report@gmx.net> writes:

> Hello Toke,
>
> On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
>> The ath9k driver was not properly clearing the status area in the
>> ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
>> it was manually filling in fields, which meant that fields introduced later
>> were left as-is.
>> 
>> Conveniently, mac80211 actually provides a helper to zero out the status
>> area, so use that to make sure we zero everything.
>> 
>> The last commit touching the driver function writing the status information
>> seems to have actually been fixing an issue that was also caused by the
>> area being uninitialised; but it only added clearing of a single field
>> instead of the whole struct. That is now redundant, though, so revert that
>> commit and use it as a convenient Fixes tag.
>> 
>> Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> ---
>>  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> index d0caf1de2bde..cbcf96ac303e 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -2553,6 +2553,8 @@ 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);
>> +
>
> As this also clears the status.rates[].count, see include/net/mac80211.h:
>
> 1195 static inline void
> 1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
> 1197 {
> 1198         int i;
> 1199         
> 1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> 1201                      offsetof(struct ieee80211_tx_info, control.rates));
> 1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> 1203                      offsetof(struct ieee80211_tx_info, driver_rates));
> 1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
> 1205         /* clear the rate counts */
> 1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
> 1207                 info->status.rates[i].count = 0;
> 1208         memset_after(&info->status, 0, rates);
> 1209 }
>
> I would have expected some lines added to restore the count (for the
> rates with index < tx_rateindex), e.g. as done in
> drivers/net/wireless/ath/ath5k/base.c:
>
> 1731         ieee80211_tx_info_clear_status(info);
> 1732 
> 1733         for (i = 0; i < ts->ts_final_idx; i++) {
> 1734                 struct ieee80211_tx_rate *r =
> 1735                         &info->status.rates[i];
> 1736 
> 1737                 r->count = tries[i];
> 1738         }
>
> In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
> the tx_rateindex index (which is often zero in case the first suggested rate
> succeeds, but in noisy environment is sometimes > 0)...

Ah, you're right! I looked at that code, and somehow parsed that as "OK,
it's setting all the rate counts, we're fine". But obviously that's not
what that code is doing, so we'll lose some information now. Bugger :(

I'll send a follow-up, thanks for flagging this!

-Toke

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Peter Seiderer]

Hello Toke,

On Fri, 1 Apr 2022 19:26:57 +0200, Peter Seiderer <ps.report@gmx.net> wrote:

> Hello Toke,
> 
> On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> > The ath9k driver was not properly clearing the status area in the
> > ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
> > it was manually filling in fields, which meant that fields introduced later
> > were left as-is.
> > 
> > Conveniently, mac80211 actually provides a helper to zero out the status
> > area, so use that to make sure we zero everything.
> > 
> > The last commit touching the driver function writing the status information
> > seems to have actually been fixing an issue that was also caused by the
> > area being uninitialised; but it only added clearing of a single field
> > instead of the whole struct. That is now redundant, though, so revert that
> > commit and use it as a convenient Fixes tag.
> > 
> > Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
> > Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> > ---
> >  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> > index d0caf1de2bde..cbcf96ac303e 100644
> > --- a/drivers/net/wireless/ath/ath9k/xmit.c
> > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> > @@ -2553,6 +2553,8 @@ 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);
> > +  
> 
> As this also clears the status.rates[].count, see include/net/mac80211.h:
> 
> 1195 static inline void
> 1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
> 1197 {
> 1198         int i;
> 1199         
> 1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> 1201                      offsetof(struct ieee80211_tx_info, control.rates));
> 1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> 1203                      offsetof(struct ieee80211_tx_info, driver_rates));
> 1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
> 1205         /* clear the rate counts */
> 1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
> 1207                 info->status.rates[i].count = 0;
> 1208         memset_after(&info->status, 0, rates);
> 1209 }
> 
> I would have expected some lines added to restore the count (for the
> rates with index < tx_rateindex), e.g. as done in
> drivers/net/wireless/ath/ath5k/base.c:
> 
> 1731         ieee80211_tx_info_clear_status(info);
> 1732 
> 1733         for (i = 0; i < ts->ts_final_idx; i++) {
> 1734                 struct ieee80211_tx_rate *r =
> 1735                         &info->status.rates[i];
> 1736 
> 1737                 r->count = tries[i];
> 1738         }
> 
> In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
> the tx_rateindex index (which is often zero in case the first suggested rate
> succeeds, but in noisy environment is sometimes > 0)...
> 
> >  	if (txok)
> >  		tx_info->status.ack_signal = ts->ts_rssi;
> >  
> > @@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> >  	}
> >  
> >  	tx_info->status.rates[)].count = ts->ts_longretry + 1;
> > -
> > -	/* we report airtime in ath_tx_count_airtime(), don't report twice */
> > -	tx_info->status.tx_time = 0;
> >  }
> >  
> >  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)  
> 
> 
> And from drivers/net/wireless/ath/ath9k/xmit.c:
> 
> 2592         for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
> 2593                 tx_info->status.rates[i].count = 0;
> 2594                 tx_info->status.rates[i].idx = -1;
> 2595         }
> 
> Line 2593 can be deleted as status.rates[].count is already zeroed through the
> ieee80211_tx_info_clear_status() call...
> 
> And it should be sufficient to do:
> 
> 	if (tx_rateindex + 1 < hw->max_rates)
> 		tx_info->status.rates[tx_rateindex + 1].idx = -1;
> 
> Regards,
> Peter

And one additional problem found with your patch applied (to 5.16.18), the log
get spammed by:

[  445.489139] ------------[ cut here ]------------
[  445.489150] WARNING: CPU: 0 PID: 8630 at drivers/net/wireless/ath/ath9k/xmit.c:174 ath_tx_complete+0x190/0x1a0 [ath9k]
[  445.489178] Modules linked in: ath9k ath9k_common ath9k_hw mac80211 ppp_async ppp_generic slhc xt_u32 xt_TCPMSS xt_tcpmss xt_MASQUERADE iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter iptable_filter ip6table_mangle ip6_tables cls_u32 sch_prio qmi_wwan cdc_wdm usbnet qcserial usb_wwan gpio_pca953x xt_tcpudp xt_mark iptable_mangle ip_tables x_tables dummy tqmx86_wdt watchdog i2c_machxo2 gpio_tqmx86 libarc4 ath tqmx86 mfd_core ftdi_sio cfg80211 usbserial nct7802 regmap_i2c [last unloaded: mac80211]
[  445.489298] CPU: 0 PID: 8630 Comm: modprobe Tainted: G        W         5.16.18+ #2
[  445.489307] Hardware name: TQ-Group TQMxE38M X64/Type2 - Board Product Name, BIOS TQMxE38M.5.4.48.0028.12 01/31/2017
[  445.489311] EIP: ath_tx_complete+0x190/0x1a0 [ath9k]
[  445.489328] Code: ff 75 e8 68 d7 a1 2c f8 89 55 ec e8 d5 4f e3 ff 8b 45 f0 8b 55 ec 83 c4 10 8b 80 58 0a 00 00 e9 a8 fe ff ff 8d b6 00 00 00 00 <0f> 0b c7 42 68 00 00 00 00 e9 1d ff ff ff 66 90 55 89 e5 57 56 89
[  445.489336] EAX: 00000000 EBX: c2e67900 ECX: c23cc040 EDX: c235a980
[  445.489342] ESI: c235a888 EDI: c2359860 EBP: c1089e18 ESP: c1089dfc
[  445.489348] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010297
[  445.489355] CR0: 80050033 CR2: b7f81000 CR3: 02ec7000 CR4: 001006d0
[  445.489362] Call Trace:
[  445.489366]  <SOFTIRQ>
[  445.489372]  ath_tx_complete_buf+0x100/0x130 [ath9k]
[  445.489390]  ath_tx_process_buffer+0x187/0xb40 [ath9k]
[  445.489407]  ? resched_curr+0x1d/0xc0
[  445.489419]  ? check_preempt_wakeup+0x115/0x250
[  445.489429]  ? task_fork_fair+0xc0/0x170
[  445.489436]  ? put_prev_task_fair+0x40/0x40
[  445.489444]  ? check_preempt_curr+0x62/0x70
[  445.489452]  ? __local_bh_enable_ip+0x33/0x80
[  445.489460]  ? _raw_spin_unlock_bh+0x13/0x20
[  445.489468]  ? ath_txq_unlock_complete+0x5a/0x100 [ath9k]
[  445.489485]  ? _raw_spin_unlock_irqrestore+0x16/0x30
[  445.489492]  ? try_to_wake_up+0x7c/0x550
[  445.489499]  ath_tx_tasklet+0x1de/0x2e0 [ath9k]
[  445.489519]  ath9k_tasklet+0x22f/0x330 [ath9k]
[  445.489535]  tasklet_action_common.constprop.0+0x89/0xb0
[  445.489544]  tasklet_action+0x21/0x30
[  445.489551]  __do_softirq+0xc5/0x28b
[  445.489559]  ? __entry_text_end+0x4/0x4
[  445.489567]  call_on_stack+0x40/0x50
[  445.489576]  </SOFTIRQ>
[  445.489580]  ? irq_exit_rcu+0x92/0x100
[  445.489587]  ? common_interrupt+0x27/0x40
[  445.489595]  ? asm_common_interrupt+0x102/0x140
[  445.489605] ---[ end trace 5c176b666255bca1 ]---
[  445.607206] ------------[ cut here ]------------


Could be fixed by the following additional change:

--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -2511,9 +2511,12 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
        struct ieee80211_hw *hw = sc->hw;
        struct ath_hw *ah = sc->sc_ah;
        u8 i, tx_rateindex;
+       bool is_valid_ack_signal = tx_info->status.is_valid_ack_signal;

        ieee80211_tx_info_clear_status(tx_info);

+       tx_info->status.is_valid_ack_signal = is_valid_ack_signal;
+
        if (txok)

Regards,
Peter

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Toke Høiland-Jørgensen]

Peter Seiderer <ps.report@gmx.net> writes:

> Hello Toke,
>
> On Fri, 1 Apr 2022 19:26:57 +0200, Peter Seiderer <ps.report@gmx.net> wrote:
>
>> Hello Toke,
>> 
>> On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> > The ath9k driver was not properly clearing the status area in the
>> > ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
>> > it was manually filling in fields, which meant that fields introduced later
>> > were left as-is.
>> > 
>> > Conveniently, mac80211 actually provides a helper to zero out the status
>> > area, so use that to make sure we zero everything.
>> > 
>> > The last commit touching the driver function writing the status information
>> > seems to have actually been fixing an issue that was also caused by the
>> > area being uninitialised; but it only added clearing of a single field
>> > instead of the whole struct. That is now redundant, though, so revert that
>> > commit and use it as a convenient Fixes tag.
>> > 
>> > Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
>> > Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> > ---
>> >  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
>> >  1 file changed, 2 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> > index d0caf1de2bde..cbcf96ac303e 100644
>> > --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> > @@ -2553,6 +2553,8 @@ 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);
>> > +  
>> 
>> As this also clears the status.rates[].count, see include/net/mac80211.h:
>> 
>> 1195 static inline void
>> 1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
>> 1197 {
>> 1198         int i;
>> 1199         
>> 1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>> 1201                      offsetof(struct ieee80211_tx_info, control.rates));
>> 1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>> 1203                      offsetof(struct ieee80211_tx_info, driver_rates));
>> 1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
>> 1205         /* clear the rate counts */
>> 1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
>> 1207                 info->status.rates[i].count = 0;
>> 1208         memset_after(&info->status, 0, rates);
>> 1209 }
>> 
>> I would have expected some lines added to restore the count (for the
>> rates with index < tx_rateindex), e.g. as done in
>> drivers/net/wireless/ath/ath5k/base.c:
>> 
>> 1731         ieee80211_tx_info_clear_status(info);
>> 1732 
>> 1733         for (i = 0; i < ts->ts_final_idx; i++) {
>> 1734                 struct ieee80211_tx_rate *r =
>> 1735                         &info->status.rates[i];
>> 1736 
>> 1737                 r->count = tries[i];
>> 1738         }
>> 
>> In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
>> the tx_rateindex index (which is often zero in case the first suggested rate
>> succeeds, but in noisy environment is sometimes > 0)...
>> 
>> >  	if (txok)
>> >  		tx_info->status.ack_signal = ts->ts_rssi;
>> >  
>> > @@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>> >  	}
>> >  
>> >  	tx_info->status.rates[)].count = ts->ts_longretry + 1;
>> > -
>> > -	/* we report airtime in ath_tx_count_airtime(), don't report twice */
>> > -	tx_info->status.tx_time = 0;
>> >  }
>> >  
>> >  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)  
>> 
>> 
>> And from drivers/net/wireless/ath/ath9k/xmit.c:
>> 
>> 2592         for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
>> 2593                 tx_info->status.rates[i].count = 0;
>> 2594                 tx_info->status.rates[i].idx = -1;
>> 2595         }
>> 
>> Line 2593 can be deleted as status.rates[].count is already zeroed through the
>> ieee80211_tx_info_clear_status() call...
>> 
>> And it should be sufficient to do:
>> 
>> 	if (tx_rateindex + 1 < hw->max_rates)
>> 		tx_info->status.rates[tx_rateindex + 1].idx = -1;
>> 
>> Regards,
>> Peter
>
> And one additional problem found with your patch applied (to 5.16.18), the log
> get spammed by:
>
> [  445.489139] ------------[ cut here ]------------
> [  445.489150] WARNING: CPU: 0 PID: 8630 at drivers/net/wireless/ath/ath9k/xmit.c:174 ath_tx_complete+0x190/0x1a0 [ath9k]
> [  445.489178] Modules linked in: ath9k ath9k_common ath9k_hw mac80211 ppp_async ppp_generic slhc xt_u32 xt_TCPMSS xt_tcpmss xt_MASQUERADE iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter iptable_filter ip6table_mangle ip6_tables cls_u32 sch_prio qmi_wwan cdc_wdm usbnet qcserial usb_wwan gpio_pca953x xt_tcpudp xt_mark iptable_mangle ip_tables x_tables dummy tqmx86_wdt watchdog i2c_machxo2 gpio_tqmx86 libarc4 ath tqmx86 mfd_core ftdi_sio cfg80211 usbserial nct7802 regmap_i2c [last unloaded: mac80211]
> [  445.489298] CPU: 0 PID: 8630 Comm: modprobe Tainted: G        W         5.16.18+ #2
> [  445.489307] Hardware name: TQ-Group TQMxE38M X64/Type2 - Board Product Name, BIOS TQMxE38M.5.4.48.0028.12 01/31/2017
> [  445.489311] EIP: ath_tx_complete+0x190/0x1a0 [ath9k]
> [  445.489328] Code: ff 75 e8 68 d7 a1 2c f8 89 55 ec e8 d5 4f e3 ff 8b 45 f0 8b 55 ec 83 c4 10 8b 80 58 0a 00 00 e9 a8 fe ff ff 8d b6 00 00 00 00 <0f> 0b c7 42 68 00 00 00 00 e9 1d ff ff ff 66 90 55 89 e5 57 56 89
> [  445.489336] EAX: 00000000 EBX: c2e67900 ECX: c23cc040 EDX: c235a980
> [  445.489342] ESI: c235a888 EDI: c2359860 EBP: c1089e18 ESP: c1089dfc
> [  445.489348] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010297
> [  445.489355] CR0: 80050033 CR2: b7f81000 CR3: 02ec7000 CR4: 001006d0
> [  445.489362] Call Trace:
> [  445.489366]  <SOFTIRQ>
> [  445.489372]  ath_tx_complete_buf+0x100/0x130 [ath9k]
> [  445.489390]  ath_tx_process_buffer+0x187/0xb40 [ath9k]
> [  445.489407]  ? resched_curr+0x1d/0xc0
> [  445.489419]  ? check_preempt_wakeup+0x115/0x250
> [  445.489429]  ? task_fork_fair+0xc0/0x170
> [  445.489436]  ? put_prev_task_fair+0x40/0x40
> [  445.489444]  ? check_preempt_curr+0x62/0x70
> [  445.489452]  ? __local_bh_enable_ip+0x33/0x80
> [  445.489460]  ? _raw_spin_unlock_bh+0x13/0x20
> [  445.489468]  ? ath_txq_unlock_complete+0x5a/0x100 [ath9k]
> [  445.489485]  ? _raw_spin_unlock_irqrestore+0x16/0x30
> [  445.489492]  ? try_to_wake_up+0x7c/0x550
> [  445.489499]  ath_tx_tasklet+0x1de/0x2e0 [ath9k]
> [  445.489519]  ath9k_tasklet+0x22f/0x330 [ath9k]
> [  445.489535]  tasklet_action_common.constprop.0+0x89/0xb0
> [  445.489544]  tasklet_action+0x21/0x30
> [  445.489551]  __do_softirq+0xc5/0x28b
> [  445.489559]  ? __entry_text_end+0x4/0x4
> [  445.489567]  call_on_stack+0x40/0x50
> [  445.489576]  </SOFTIRQ>
> [  445.489580]  ? irq_exit_rcu+0x92/0x100
> [  445.489587]  ? common_interrupt+0x27/0x40
> [  445.489595]  ? asm_common_interrupt+0x102/0x140
> [  445.489605] ---[ end trace 5c176b666255bca1 ]---
> [  445.607206] ------------[ cut here ]------------
>
>
> Could be fixed by the following additional change:
>
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -2511,9 +2511,12 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>         struct ieee80211_hw *hw = sc->hw;
>         struct ath_hw *ah = sc->sc_ah;
>         u8 i, tx_rateindex;
> +       bool is_valid_ack_signal = tx_info->status.is_valid_ack_signal;
>
>         ieee80211_tx_info_clear_status(tx_info);
>
> +       tx_info->status.is_valid_ack_signal = is_valid_ack_signal;
> +
>         if (txok)

That doesn't seem right, zeroing is_valid_ack_signal was the whole point
of the first patch... Something seems off in that backtrace; could you
try running it through ./scripts/decode_stacktrace.sh please?

-Toke

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Peter Seiderer]

Hello Toke,

On Sat, 02 Apr 2022 17:11:54 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> Peter Seiderer <ps.report@gmx.net> writes:
> 
> > Hello Toke,
> >
> > On Fri, 1 Apr 2022 19:26:57 +0200, Peter Seiderer <ps.report@gmx.net> wrote:
> >  
> >> Hello Toke,
> >> 
> >> On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> >>   
> >> > The ath9k driver was not properly clearing the status area in the
> >> > ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
> >> > it was manually filling in fields, which meant that fields introduced later
> >> > were left as-is.
> >> > 
> >> > Conveniently, mac80211 actually provides a helper to zero out the status
> >> > area, so use that to make sure we zero everything.
> >> > 
> >> > The last commit touching the driver function writing the status information
> >> > seems to have actually been fixing an issue that was also caused by the
> >> > area being uninitialised; but it only added clearing of a single field
> >> > instead of the whole struct. That is now redundant, though, so revert that
> >> > commit and use it as a convenient Fixes tag.
> >> > 
> >> > Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
> >> > Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> >> > ---
> >> >  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
> >> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >> > 
> >> > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> >> > index d0caf1de2bde..cbcf96ac303e 100644
> >> > --- a/drivers/net/wireless/ath/ath9k/xmit.c
> >> > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> >> > @@ -2553,6 +2553,8 @@ 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);
> >> > +    
> >> 
> >> As this also clears the status.rates[].count, see include/net/mac80211.h:
> >> 
> >> 1195 static inline void
> >> 1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
> >> 1197 {
> >> 1198         int i;
> >> 1199         
> >> 1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> >> 1201                      offsetof(struct ieee80211_tx_info, control.rates));
> >> 1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> >> 1203                      offsetof(struct ieee80211_tx_info, driver_rates));
> >> 1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
> >> 1205         /* clear the rate counts */
> >> 1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
> >> 1207                 info->status.rates[i].count = 0;
> >> 1208         memset_after(&info->status, 0, rates);
> >> 1209 }
> >> 
> >> I would have expected some lines added to restore the count (for the
> >> rates with index < tx_rateindex), e.g. as done in
> >> drivers/net/wireless/ath/ath5k/base.c:
> >> 
> >> 1731         ieee80211_tx_info_clear_status(info);
> >> 1732 
> >> 1733         for (i = 0; i < ts->ts_final_idx; i++) {
> >> 1734                 struct ieee80211_tx_rate *r =
> >> 1735                         &info->status.rates[i];
> >> 1736 
> >> 1737                 r->count = tries[i];
> >> 1738         }
> >> 
> >> In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
> >> the tx_rateindex index (which is often zero in case the first suggested rate
> >> succeeds, but in noisy environment is sometimes > 0)...
> >>   
> >> >  	if (txok)
> >> >  		tx_info->status.ack_signal = ts->ts_rssi;
> >> >  
> >> > @@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> >> >  	}
> >> >  
> >> >  	tx_info->status.rates[)].count = ts->ts_longretry + 1;
> >> > -
> >> > -	/* we report airtime in ath_tx_count_airtime(), don't report twice */
> >> > -	tx_info->status.tx_time = 0;
> >> >  }
> >> >  
> >> >  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)    
> >> 
> >> 
> >> And from drivers/net/wireless/ath/ath9k/xmit.c:
> >> 
> >> 2592         for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
> >> 2593                 tx_info->status.rates[i].count = 0;
> >> 2594                 tx_info->status.rates[i].idx = -1;
> >> 2595         }
> >> 
> >> Line 2593 can be deleted as status.rates[].count is already zeroed through the
> >> ieee80211_tx_info_clear_status() call...
> >> 
> >> And it should be sufficient to do:
> >> 
> >> 	if (tx_rateindex + 1 < hw->max_rates)
> >> 		tx_info->status.rates[tx_rateindex + 1].idx = -1;
> >> 
> >> Regards,
> >> Peter  
> >
> > And one additional problem found with your patch applied (to 5.16.18), the log
> > get spammed by:
> >
> > [  445.489139] ------------[ cut here ]------------
> > [  445.489150] WARNING: CPU: 0 PID: 8630 at drivers/net/wireless/ath/ath9k/xmit.c:174 ath_tx_complete+0x190/0x1a0 [ath9k]
> > [  445.489178] Modules linked in: ath9k ath9k_common ath9k_hw mac80211 ppp_async ppp_generic slhc xt_u32 xt_TCPMSS xt_tcpmss xt_MASQUERADE iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter iptable_filter ip6table_mangle ip6_tables cls_u32 sch_prio qmi_wwan cdc_wdm usbnet qcserial usb_wwan gpio_pca953x xt_tcpudp xt_mark iptable_mangle ip_tables x_tables dummy tqmx86_wdt watchdog i2c_machxo2 gpio_tqmx86 libarc4 ath tqmx86 mfd_core ftdi_sio cfg80211 usbserial nct7802 regmap_i2c [last unloaded: mac80211]
> > [  445.489298] CPU: 0 PID: 8630 Comm: modprobe Tainted: G        W         5.16.18+ #2
> > [  445.489307] Hardware name: TQ-Group TQMxE38M X64/Type2 - Board Product Name, BIOS TQMxE38M.5.4.48.0028.12 01/31/2017
> > [  445.489311] EIP: ath_tx_complete+0x190/0x1a0 [ath9k]
> > [  445.489328] Code: ff 75 e8 68 d7 a1 2c f8 89 55 ec e8 d5 4f e3 ff 8b 45 f0 8b 55 ec 83 c4 10 8b 80 58 0a 00 00 e9 a8 fe ff ff 8d b6 00 00 00 00 <0f> 0b c7 42 68 00 00 00 00 e9 1d ff ff ff 66 90 55 89 e5 57 56 89
> > [  445.489336] EAX: 00000000 EBX: c2e67900 ECX: c23cc040 EDX: c235a980
> > [  445.489342] ESI: c235a888 EDI: c2359860 EBP: c1089e18 ESP: c1089dfc
> > [  445.489348] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010297
> > [  445.489355] CR0: 80050033 CR2: b7f81000 CR3: 02ec7000 CR4: 001006d0
> > [  445.489362] Call Trace:
> > [  445.489366]  <SOFTIRQ>
> > [  445.489372]  ath_tx_complete_buf+0x100/0x130 [ath9k]
> > [  445.489390]  ath_tx_process_buffer+0x187/0xb40 [ath9k]
> > [  445.489407]  ? resched_curr+0x1d/0xc0
> > [  445.489419]  ? check_preempt_wakeup+0x115/0x250
> > [  445.489429]  ? task_fork_fair+0xc0/0x170
> > [  445.489436]  ? put_prev_task_fair+0x40/0x40
> > [  445.489444]  ? check_preempt_curr+0x62/0x70
> > [  445.489452]  ? __local_bh_enable_ip+0x33/0x80
> > [  445.489460]  ? _raw_spin_unlock_bh+0x13/0x20
> > [  445.489468]  ? ath_txq_unlock_complete+0x5a/0x100 [ath9k]
> > [  445.489485]  ? _raw_spin_unlock_irqrestore+0x16/0x30
> > [  445.489492]  ? try_to_wake_up+0x7c/0x550
> > [  445.489499]  ath_tx_tasklet+0x1de/0x2e0 [ath9k]
> > [  445.489519]  ath9k_tasklet+0x22f/0x330 [ath9k]
> > [  445.489535]  tasklet_action_common.constprop.0+0x89/0xb0
> > [  445.489544]  tasklet_action+0x21/0x30
> > [  445.489551]  __do_softirq+0xc5/0x28b
> > [  445.489559]  ? __entry_text_end+0x4/0x4
> > [  445.489567]  call_on_stack+0x40/0x50
> > [  445.489576]  </SOFTIRQ>
> > [  445.489580]  ? irq_exit_rcu+0x92/0x100
> > [  445.489587]  ? common_interrupt+0x27/0x40
> > [  445.489595]  ? asm_common_interrupt+0x102/0x140
> > [  445.489605] ---[ end trace 5c176b666255bca1 ]---
> > [  445.607206] ------------[ cut here ]------------
> >
> >
> > Could be fixed by the following additional change:
> >
> > --- a/drivers/net/wireless/ath/ath9k/xmit.c
> > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> > @@ -2511,9 +2511,12 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> >         struct ieee80211_hw *hw = sc->hw;
> >         struct ath_hw *ah = sc->sc_ah;
> >         u8 i, tx_rateindex;
> > +       bool is_valid_ack_signal = tx_info->status.is_valid_ack_signal;
> >
> >         ieee80211_tx_info_clear_status(tx_info);
> >
> > +       tx_info->status.is_valid_ack_signal = is_valid_ack_signal;
> > +
> >         if (txok)  
> 
> That doesn't seem right, zeroing is_valid_ack_signal was the whole point
> of the first patch... Something seems off in that backtrace; could you
> try running it through ./scripts/decode_stacktrace.sh please?

Will do (and re-run the test with an fresh compiled kernel as CONFIG_DEBUG_KERNEL
was not set - but need to wait until next week as I need physical access to
the test system for kernel update)...

Regards,
Peter

> 
> -Toke

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Peter Seiderer]

Hello Toke,

On Sat, 2 Apr 2022 18:19:10 +0200, Peter Seiderer <ps.report@gmx.net> wrote:

> Hello Toke,
> 
> On Sat, 02 Apr 2022 17:11:54 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> > Peter Seiderer <ps.report@gmx.net> writes:
> >   
> > > Hello Toke,
> > >
> > > On Fri, 1 Apr 2022 19:26:57 +0200, Peter Seiderer <ps.report@gmx.net> wrote:
> > >    
> > >> Hello Toke,
> > >> 
> > >> On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> > >>     
> > >> > The ath9k driver was not properly clearing the status area in the
> > >> > ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
> > >> > it was manually filling in fields, which meant that fields introduced later
> > >> > were left as-is.
> > >> > 
> > >> > Conveniently, mac80211 actually provides a helper to zero out the status
> > >> > area, so use that to make sure we zero everything.
> > >> > 
> > >> > The last commit touching the driver function writing the status information
> > >> > seems to have actually been fixing an issue that was also caused by the
> > >> > area being uninitialised; but it only added clearing of a single field
> > >> > instead of the whole struct. That is now redundant, though, so revert that
> > >> > commit and use it as a convenient Fixes tag.
> > >> > 
> > >> > Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
> > >> > Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> > >> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> > >> > ---
> > >> >  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
> > >> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >> > 
> > >> > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> > >> > index d0caf1de2bde..cbcf96ac303e 100644
> > >> > --- a/drivers/net/wireless/ath/ath9k/xmit.c
> > >> > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> > >> > @@ -2553,6 +2553,8 @@ 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);
> > >> > +      
> > >> 
> > >> As this also clears the status.rates[].count, see include/net/mac80211.h:
> > >> 
> > >> 1195 static inline void
> > >> 1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
> > >> 1197 {
> > >> 1198         int i;
> > >> 1199         
> > >> 1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> > >> 1201                      offsetof(struct ieee80211_tx_info, control.rates));
> > >> 1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
> > >> 1203                      offsetof(struct ieee80211_tx_info, driver_rates));
> > >> 1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
> > >> 1205         /* clear the rate counts */
> > >> 1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
> > >> 1207                 info->status.rates[i].count = 0;
> > >> 1208         memset_after(&info->status, 0, rates);
> > >> 1209 }
> > >> 
> > >> I would have expected some lines added to restore the count (for the
> > >> rates with index < tx_rateindex), e.g. as done in
> > >> drivers/net/wireless/ath/ath5k/base.c:
> > >> 
> > >> 1731         ieee80211_tx_info_clear_status(info);
> > >> 1732 
> > >> 1733         for (i = 0; i < ts->ts_final_idx; i++) {
> > >> 1734                 struct ieee80211_tx_rate *r =
> > >> 1735                         &info->status.rates[i];
> > >> 1736 
> > >> 1737                 r->count = tries[i];
> > >> 1738         }
> > >> 
> > >> In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
> > >> the tx_rateindex index (which is often zero in case the first suggested rate
> > >> succeeds, but in noisy environment is sometimes > 0)...
> > >>     
> > >> >  	if (txok)
> > >> >  		tx_info->status.ack_signal = ts->ts_rssi;
> > >> >  
> > >> > @@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> > >> >  	}
> > >> >  
> > >> >  	tx_info->status.rates[)].count = ts->ts_longretry + 1;
> > >> > -
> > >> > -	/* we report airtime in ath_tx_count_airtime(), don't report twice */
> > >> > -	tx_info->status.tx_time = 0;
> > >> >  }
> > >> >  
> > >> >  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)      
> > >> 
> > >> 
> > >> And from drivers/net/wireless/ath/ath9k/xmit.c:
> > >> 
> > >> 2592         for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
> > >> 2593                 tx_info->status.rates[i].count = 0;
> > >> 2594                 tx_info->status.rates[i].idx = -1;
> > >> 2595         }
> > >> 
> > >> Line 2593 can be deleted as status.rates[].count is already zeroed through the
> > >> ieee80211_tx_info_clear_status() call...
> > >> 
> > >> And it should be sufficient to do:
> > >> 
> > >> 	if (tx_rateindex + 1 < hw->max_rates)
> > >> 		tx_info->status.rates[tx_rateindex + 1].idx = -1;
> > >> 
> > >> Regards,
> > >> Peter    
> > >
> > > And one additional problem found with your patch applied (to 5.16.18), the log
> > > get spammed by:
> > >
> > > [  445.489139] ------------[ cut here ]------------
> > > [  445.489150] WARNING: CPU: 0 PID: 8630 at drivers/net/wireless/ath/ath9k/xmit.c:174 ath_tx_complete+0x190/0x1a0 [ath9k]
> > > [  445.489178] Modules linked in: ath9k ath9k_common ath9k_hw mac80211 ppp_async ppp_generic slhc xt_u32 xt_TCPMSS xt_tcpmss xt_MASQUERADE iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter iptable_filter ip6table_mangle ip6_tables cls_u32 sch_prio qmi_wwan cdc_wdm usbnet qcserial usb_wwan gpio_pca953x xt_tcpudp xt_mark iptable_mangle ip_tables x_tables dummy tqmx86_wdt watchdog i2c_machxo2 gpio_tqmx86 libarc4 ath tqmx86 mfd_core ftdi_sio cfg80211 usbserial nct7802 regmap_i2c [last unloaded: mac80211]
> > > [  445.489298] CPU: 0 PID: 8630 Comm: modprobe Tainted: G        W         5.16.18+ #2
> > > [  445.489307] Hardware name: TQ-Group TQMxE38M X64/Type2 - Board Product Name, BIOS TQMxE38M.5.4.48.0028.12 01/31/2017
> > > [  445.489311] EIP: ath_tx_complete+0x190/0x1a0 [ath9k]
> > > [  445.489328] Code: ff 75 e8 68 d7 a1 2c f8 89 55 ec e8 d5 4f e3 ff 8b 45 f0 8b 55 ec 83 c4 10 8b 80 58 0a 00 00 e9 a8 fe ff ff 8d b6 00 00 00 00 <0f> 0b c7 42 68 00 00 00 00 e9 1d ff ff ff 66 90 55 89 e5 57 56 89
> > > [  445.489336] EAX: 00000000 EBX: c2e67900 ECX: c23cc040 EDX: c235a980
> > > [  445.489342] ESI: c235a888 EDI: c2359860 EBP: c1089e18 ESP: c1089dfc
> > > [  445.489348] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010297
> > > [  445.489355] CR0: 80050033 CR2: b7f81000 CR3: 02ec7000 CR4: 001006d0
> > > [  445.489362] Call Trace:
> > > [  445.489366]  <SOFTIRQ>
> > > [  445.489372]  ath_tx_complete_buf+0x100/0x130 [ath9k]
> > > [  445.489390]  ath_tx_process_buffer+0x187/0xb40 [ath9k]
> > > [  445.489407]  ? resched_curr+0x1d/0xc0
> > > [  445.489419]  ? check_preempt_wakeup+0x115/0x250
> > > [  445.489429]  ? task_fork_fair+0xc0/0x170
> > > [  445.489436]  ? put_prev_task_fair+0x40/0x40
> > > [  445.489444]  ? check_preempt_curr+0x62/0x70
> > > [  445.489452]  ? __local_bh_enable_ip+0x33/0x80
> > > [  445.489460]  ? _raw_spin_unlock_bh+0x13/0x20
> > > [  445.489468]  ? ath_txq_unlock_complete+0x5a/0x100 [ath9k]
> > > [  445.489485]  ? _raw_spin_unlock_irqrestore+0x16/0x30
> > > [  445.489492]  ? try_to_wake_up+0x7c/0x550
> > > [  445.489499]  ath_tx_tasklet+0x1de/0x2e0 [ath9k]
> > > [  445.489519]  ath9k_tasklet+0x22f/0x330 [ath9k]
> > > [  445.489535]  tasklet_action_common.constprop.0+0x89/0xb0
> > > [  445.489544]  tasklet_action+0x21/0x30
> > > [  445.489551]  __do_softirq+0xc5/0x28b
> > > [  445.489559]  ? __entry_text_end+0x4/0x4
> > > [  445.489567]  call_on_stack+0x40/0x50
> > > [  445.489576]  </SOFTIRQ>
> > > [  445.489580]  ? irq_exit_rcu+0x92/0x100
> > > [  445.489587]  ? common_interrupt+0x27/0x40
> > > [  445.489595]  ? asm_common_interrupt+0x102/0x140
> > > [  445.489605] ---[ end trace 5c176b666255bca1 ]---
> > > [  445.607206] ------------[ cut here ]------------
> > >
> > >
> > > Could be fixed by the following additional change:
> > >
> > > --- a/drivers/net/wireless/ath/ath9k/xmit.c
> > > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> > > @@ -2511,9 +2511,12 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
> > >         struct ieee80211_hw *hw = sc->hw;
> > >         struct ath_hw *ah = sc->sc_ah;
> > >         u8 i, tx_rateindex;
> > > +       bool is_valid_ack_signal = tx_info->status.is_valid_ack_signal;
> > >
> > >         ieee80211_tx_info_clear_status(tx_info);
> > >
> > > +       tx_info->status.is_valid_ack_signal = is_valid_ack_signal;
> > > +
> > >         if (txok)    
> > 
> > That doesn't seem right, zeroing is_valid_ack_signal was the whole point
> > of the first patch... Something seems off in that backtrace; could you
> > try running it through ./scripts/decode_stacktrace.sh please?  
> 
> Will do (and re-run the test with an fresh compiled kernel as CONFIG_DEBUG_KERNEL
> was not set - but need to wait until next week as I need physical access to
> the test system for kernel update)...

Same with an fresh build (and with stacktrace run through decode_stacktrace.sh), but
did some more investigations, added an debug print of tx_info->status.is_valid_ack_signal
in ath_tx_rc_status() and an corresponding one in ath_txq_skb_done() printing out
fi->txq, with the additional

	tx_info->status.is_valid_ack_signal = is_valid_ack_signal;

line added showed the following output

[  839.543175] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
[  839.543195] XXX - ath_txq_skb_done() txq: 2
[  839.617898] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
[  839.617961] XXX - ath_txq_skb_done() txq: 2
[  840.024845] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
[  840.024863] XXX - ath_txq_skb_done() txq: 2
[...]

and without the following:

[  924.614745] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
[  924.614762] XXX - ath_txq_skb_done() txq: 0
[  924.614805] ------------[ cut here ]------------
[  924.614809] WARNING: CPU: 0 PID: 0 at drivers/net/wireless/ath/ath9k/xmit.c:176 ath_txq_skb_done.constprop.0+0x28/0x38 [ath9k]
[...]


Seems ieee80211_tx_info_clear_status() clears/destroys the dual use of the
struct ieee80211_tx_info.status vs. ieee80211_tx_info.rate_driver_data...

See drivers/net/wireless/ath/ath9k/xmit.c

 140 static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
 141 {
 142         struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
 143         BUILD_BUG_ON(sizeof(struct ath_frame_info) >
 144                      sizeof(tx_info->rate_driver_data));
 145         return (struct ath_frame_info *) &tx_info->rate_driver_data[0];
 146 }


 205 static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
 206                              struct sk_buff *skb)
 207 {
 208         struct ath_frame_info *fi = get_frame_info(skb);
 209         int q = fi->txq;
 210 
 211         if (q < 0)
 212                 return;
 213 
 214         txq = sc->tx.txq_map[q];
 215         if (WARN_ON(--txq->pending_frames < 0))
 216                 txq->pending_frames = 0;
 217 
 218 }

Seems it only worked by chance (?) before...

Regards,
Peter


> 
> Regards,
> Peter
> 
> > 
> > -Toke  
> 

Re: [PATCH v5.18] ath9k: Properly clear TX status area before reporting to mac80211

[Toke Høiland-Jørgensen]

Peter Seiderer <ps.report@gmx.net> writes:

> Hello Toke,
>
> On Sat, 2 Apr 2022 18:19:10 +0200, Peter Seiderer <ps.report@gmx.net> wrote:
>
>> Hello Toke,
>> 
>> On Sat, 02 Apr 2022 17:11:54 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> > Peter Seiderer <ps.report@gmx.net> writes:
>> >   
>> > > Hello Toke,
>> > >
>> > > On Fri, 1 Apr 2022 19:26:57 +0200, Peter Seiderer <ps.report@gmx.net> wrote:
>> > >    
>> > >> Hello Toke,
>> > >> 
>> > >> On Wed, 30 Mar 2022 18:44:09 +0200, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> > >>     
>> > >> > The ath9k driver was not properly clearing the status area in the
>> > >> > ieee80211_tx_info struct before reporting TX status to mac80211. Instead,
>> > >> > it was manually filling in fields, which meant that fields introduced later
>> > >> > were left as-is.
>> > >> > 
>> > >> > Conveniently, mac80211 actually provides a helper to zero out the status
>> > >> > area, so use that to make sure we zero everything.
>> > >> > 
>> > >> > The last commit touching the driver function writing the status information
>> > >> > seems to have actually been fixing an issue that was also caused by the
>> > >> > area being uninitialised; but it only added clearing of a single field
>> > >> > instead of the whole struct. That is now redundant, though, so revert that
>> > >> > commit and use it as a convenient Fixes tag.
>> > >> > 
>> > >> > Fixes: cc591d77aba1 ("ath9k: Make sure to zero status.tx_time before reporting TX status")
>> > >> > Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> > >> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> > >> > ---
>> > >> >  drivers/net/wireless/ath/ath9k/xmit.c | 5 ++---
>> > >> >  1 file changed, 2 insertions(+), 3 deletions(-)
>> > >> > 
>> > >> > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> > >> > index d0caf1de2bde..cbcf96ac303e 100644
>> > >> > --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> > >> > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> > >> > @@ -2553,6 +2553,8 @@ 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);
>> > >> > +      
>> > >> 
>> > >> As this also clears the status.rates[].count, see include/net/mac80211.h:
>> > >> 
>> > >> 1195 static inline void
>> > >> 1196 ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
>> > >> 1197 {
>> > >> 1198         int i;
>> > >> 1199         
>> > >> 1200         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>> > >> 1201                      offsetof(struct ieee80211_tx_info, control.rates));
>> > >> 1202         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>> > >> 1203                      offsetof(struct ieee80211_tx_info, driver_rates));
>> > >> 1204         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8)     ;       
>> > >> 1205         /* clear the rate counts */
>> > >> 1206         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
>> > >> 1207                 info->status.rates[i].count = 0;
>> > >> 1208         memset_after(&info->status, 0, rates);
>> > >> 1209 }
>> > >> 
>> > >> I would have expected some lines added to restore the count (for the
>> > >> rates with index < tx_rateindex), e.g. as done in
>> > >> drivers/net/wireless/ath/ath5k/base.c:
>> > >> 
>> > >> 1731         ieee80211_tx_info_clear_status(info);
>> > >> 1732 
>> > >> 1733         for (i = 0; i < ts->ts_final_idx; i++) {
>> > >> 1734                 struct ieee80211_tx_rate *r =
>> > >> 1735                         &info->status.rates[i];
>> > >> 1736 
>> > >> 1737                 r->count = tries[i];
>> > >> 1738         }
>> > >> 
>> > >> In drivers/net/wireless/ath/ath9k/xmit.c this is only done/changed for
>> > >> the tx_rateindex index (which is often zero in case the first suggested rate
>> > >> succeeds, but in noisy environment is sometimes > 0)...
>> > >>     
>> > >> >  	if (txok)
>> > >> >  		tx_info->status.ack_signal = ts->ts_rssi;
>> > >> >  
>> > >> > @@ -2595,9 +2597,6 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>> > >> >  	}
>> > >> >  
>> > >> >  	tx_info->status.rates[)].count = ts->ts_longretry + 1;
>> > >> > -
>> > >> > -	/* we report airtime in ath_tx_count_airtime(), don't report twice */
>> > >> > -	tx_info->status.tx_time = 0;
>> > >> >  }
>> > >> >  
>> > >> >  static void ath_tx_processq(struct ath_softc *sc, struct ath_txq *txq)      
>> > >> 
>> > >> 
>> > >> And from drivers/net/wireless/ath/ath9k/xmit.c:
>> > >> 
>> > >> 2592         for (i = tx_rateindex + 1; i < hw->max_rates; i++) {
>> > >> 2593                 tx_info->status.rates[i].count = 0;
>> > >> 2594                 tx_info->status.rates[i].idx = -1;
>> > >> 2595         }
>> > >> 
>> > >> Line 2593 can be deleted as status.rates[].count is already zeroed through the
>> > >> ieee80211_tx_info_clear_status() call...
>> > >> 
>> > >> And it should be sufficient to do:
>> > >> 
>> > >> 	if (tx_rateindex + 1 < hw->max_rates)
>> > >> 		tx_info->status.rates[tx_rateindex + 1].idx = -1;
>> > >> 
>> > >> Regards,
>> > >> Peter    
>> > >
>> > > And one additional problem found with your patch applied (to 5.16.18), the log
>> > > get spammed by:
>> > >
>> > > [  445.489139] ------------[ cut here ]------------
>> > > [  445.489150] WARNING: CPU: 0 PID: 8630 at drivers/net/wireless/ath/ath9k/xmit.c:174 ath_tx_complete+0x190/0x1a0 [ath9k]
>> > > [  445.489178] Modules linked in: ath9k ath9k_common ath9k_hw mac80211 ppp_async ppp_generic slhc xt_u32 xt_TCPMSS xt_tcpmss xt_MASQUERADE iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter iptable_filter ip6table_mangle ip6_tables cls_u32 sch_prio qmi_wwan cdc_wdm usbnet qcserial usb_wwan gpio_pca953x xt_tcpudp xt_mark iptable_mangle ip_tables x_tables dummy tqmx86_wdt watchdog i2c_machxo2 gpio_tqmx86 libarc4 ath tqmx86 mfd_core ftdi_sio cfg80211 usbserial nct7802 regmap_i2c [last unloaded: mac80211]
>> > > [  445.489298] CPU: 0 PID: 8630 Comm: modprobe Tainted: G        W         5.16.18+ #2
>> > > [  445.489307] Hardware name: TQ-Group TQMxE38M X64/Type2 - Board Product Name, BIOS TQMxE38M.5.4.48.0028.12 01/31/2017
>> > > [  445.489311] EIP: ath_tx_complete+0x190/0x1a0 [ath9k]
>> > > [  445.489328] Code: ff 75 e8 68 d7 a1 2c f8 89 55 ec e8 d5 4f e3 ff 8b 45 f0 8b 55 ec 83 c4 10 8b 80 58 0a 00 00 e9 a8 fe ff ff 8d b6 00 00 00 00 <0f> 0b c7 42 68 00 00 00 00 e9 1d ff ff ff 66 90 55 89 e5 57 56 89
>> > > [  445.489336] EAX: 00000000 EBX: c2e67900 ECX: c23cc040 EDX: c235a980
>> > > [  445.489342] ESI: c235a888 EDI: c2359860 EBP: c1089e18 ESP: c1089dfc
>> > > [  445.489348] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 EFLAGS: 00010297
>> > > [  445.489355] CR0: 80050033 CR2: b7f81000 CR3: 02ec7000 CR4: 001006d0
>> > > [  445.489362] Call Trace:
>> > > [  445.489366]  <SOFTIRQ>
>> > > [  445.489372]  ath_tx_complete_buf+0x100/0x130 [ath9k]
>> > > [  445.489390]  ath_tx_process_buffer+0x187/0xb40 [ath9k]
>> > > [  445.489407]  ? resched_curr+0x1d/0xc0
>> > > [  445.489419]  ? check_preempt_wakeup+0x115/0x250
>> > > [  445.489429]  ? task_fork_fair+0xc0/0x170
>> > > [  445.489436]  ? put_prev_task_fair+0x40/0x40
>> > > [  445.489444]  ? check_preempt_curr+0x62/0x70
>> > > [  445.489452]  ? __local_bh_enable_ip+0x33/0x80
>> > > [  445.489460]  ? _raw_spin_unlock_bh+0x13/0x20
>> > > [  445.489468]  ? ath_txq_unlock_complete+0x5a/0x100 [ath9k]
>> > > [  445.489485]  ? _raw_spin_unlock_irqrestore+0x16/0x30
>> > > [  445.489492]  ? try_to_wake_up+0x7c/0x550
>> > > [  445.489499]  ath_tx_tasklet+0x1de/0x2e0 [ath9k]
>> > > [  445.489519]  ath9k_tasklet+0x22f/0x330 [ath9k]
>> > > [  445.489535]  tasklet_action_common.constprop.0+0x89/0xb0
>> > > [  445.489544]  tasklet_action+0x21/0x30
>> > > [  445.489551]  __do_softirq+0xc5/0x28b
>> > > [  445.489559]  ? __entry_text_end+0x4/0x4
>> > > [  445.489567]  call_on_stack+0x40/0x50
>> > > [  445.489576]  </SOFTIRQ>
>> > > [  445.489580]  ? irq_exit_rcu+0x92/0x100
>> > > [  445.489587]  ? common_interrupt+0x27/0x40
>> > > [  445.489595]  ? asm_common_interrupt+0x102/0x140
>> > > [  445.489605] ---[ end trace 5c176b666255bca1 ]---
>> > > [  445.607206] ------------[ cut here ]------------
>> > >
>> > >
>> > > Could be fixed by the following additional change:
>> > >
>> > > --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> > > +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> > > @@ -2511,9 +2511,12 @@ static void ath_tx_rc_status(struct ath_softc *sc, struct ath_buf *bf,
>> > >         struct ieee80211_hw *hw = sc->hw;
>> > >         struct ath_hw *ah = sc->sc_ah;
>> > >         u8 i, tx_rateindex;
>> > > +       bool is_valid_ack_signal = tx_info->status.is_valid_ack_signal;
>> > >
>> > >         ieee80211_tx_info_clear_status(tx_info);
>> > >
>> > > +       tx_info->status.is_valid_ack_signal = is_valid_ack_signal;
>> > > +
>> > >         if (txok)    
>> > 
>> > That doesn't seem right, zeroing is_valid_ack_signal was the whole point
>> > of the first patch... Something seems off in that backtrace; could you
>> > try running it through ./scripts/decode_stacktrace.sh please?  
>> 
>> Will do (and re-run the test with an fresh compiled kernel as CONFIG_DEBUG_KERNEL
>> was not set - but need to wait until next week as I need physical access to
>> the test system for kernel update)...
>
> Same with an fresh build (and with stacktrace run through decode_stacktrace.sh), but
> did some more investigations, added an debug print of tx_info->status.is_valid_ack_signal
> in ath_tx_rc_status() and an corresponding one in ath_txq_skb_done() printing out
> fi->txq, with the additional
>
> 	tx_info->status.is_valid_ack_signal = is_valid_ack_signal;
>
> line added showed the following output
>
> [  839.543175] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
> [  839.543195] XXX - ath_txq_skb_done() txq: 2
> [  839.617898] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
> [  839.617961] XXX - ath_txq_skb_done() txq: 2
> [  840.024845] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
> [  840.024863] XXX - ath_txq_skb_done() txq: 2
> [...]
>
> and without the following:
>
> [  924.614745] XXX - ath_tx_rc_status() - is_valid_ack_signal: 2
> [  924.614762] XXX - ath_txq_skb_done() txq: 0
> [  924.614805] ------------[ cut here ]------------
> [  924.614809] WARNING: CPU: 0 PID: 0 at drivers/net/wireless/ath/ath9k/xmit.c:176 ath_txq_skb_done.constprop.0+0x28/0x38 [ath9k]
> [...]
>
>
> Seems ieee80211_tx_info_clear_status() clears/destroys the dual use of the
> struct ieee80211_tx_info.status vs. ieee80211_tx_info.rate_driver_data...
>
> See drivers/net/wireless/ath/ath9k/xmit.c
>
>  140 static struct ath_frame_info *get_frame_info(struct sk_buff *skb)
>  141 {
>  142         struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
>  143         BUILD_BUG_ON(sizeof(struct ath_frame_info) >
>  144                      sizeof(tx_info->rate_driver_data));
>  145         return (struct ath_frame_info *) &tx_info->rate_driver_data[0];
>  146 }
>
>
>  205 static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
>  206                              struct sk_buff *skb)
>  207 {
>  208         struct ath_frame_info *fi = get_frame_info(skb);
>  209         int q = fi->txq;
>  210 
>  211         if (q < 0)
>  212                 return;
>  213 
>  214         txq = sc->tx.txq_map[q];
>  215         if (WARN_ON(--txq->pending_frames < 0))
>  216                 txq->pending_frames = 0;
>  217 
>  218 }
>
> Seems it only worked by chance (?) before...

Yikes, yeah, that's no good! Also, this comment above
ieee80211_tx_info_clear_status() is completely misleading:

 * NOTE: You can only use this function if you do NOT use
 *	 info->driver_data! Use info->rate_driver_data
 *	 instead if you need only the less space that allows.

(I read that, and figured that the current use was OK, but nope, guess
not!)

The ath_frame_info struct is small enough to fit into the
status_driver_data field, though, so guess we could just move it there.
However, ieee80211_tx_info_clear_status() also clears that AFAICT, so
we'll need to save & restore it? That seems a bit odd, is this
intentional Johannes?

-Toke