Re: [PATCH 1/2] wifi: ath10k: Implement ieee80211 flush_sta callback

[Jeff Johnson <quic_jjohnson@quicinc.com>]

On 10/12/2024 7:13 AM, Remi Pommarel wrote:
> When a STA reassociates, mac80211's _sta_info_move_state() waits for all
> pending frame to be flushed before removing the key (so that no frame
> get sent unencrypted after key removable [0]). When a driver does not
> implement the flush_sta callback, ieee80211_flush_queues() is called
> instead which effectively stops the whole queue until it is completely
> drained.
> 
> The ath10k driver configure all STAs of one vdev to share the same
> queue. So when flushing one STA this is the whole vdev queue that is
> blocked until completely drained causing Tx to other STA to also stall
> this whole time.
> 
> One easy way to reproduce the issue is to connect two STAs (STA0 and
> STA1) to an ath10k AP. While Generating a bunch of traffic from AP to
> STA0 (e.g. fping -l -p 20 <STA0-IP>) disconnect STA0 from AP without
> clean disassociation (e.g. remove power, reboot -f). Then as soon as
> STA0 is effectively disconnected from AP (either after inactivity
> timeout or forced with iw dev AP station del STA0), its queues get
> flushed using ieee80211_flush_queues(). This causes STA1 to suffer a
> connectivity stall for about 5 seconds (see ATH10K_FLUSH_TIMEOUT_HZ).
> 
> Implement a flush_sta callback in ath10k to wait only for a specific
> STA pending frames to be drained (without stopping the whole HW queue)
> to fix that.
> 
> [0]: commit 0b75a1b1e42e ("wifi: mac80211: flush queues on STA removal")
> 
> Reported-by: Cedric Veilleux <veilleux.cedric@gmail.com>

checkpatch.pl reports:
WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report

> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  drivers/net/wireless/ath/ath10k/core.h   |  4 +++
>  drivers/net/wireless/ath/ath10k/htt.h    |  4 +++
>  drivers/net/wireless/ath/ath10k/htt_tx.c | 32 ++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/mac.c    | 43 +++++++++++++++++++++++-
>  drivers/net/wireless/ath/ath10k/txrx.c   |  3 ++
>  5 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 446dca74f06a..4709e4887efc 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -558,6 +558,10 @@ struct ath10k_sta {
>  	u8 rate_ctrl[ATH10K_TID_MAX];
>  	u32 rate_code[ATH10K_TID_MAX];
>  	int rtscts[ATH10K_TID_MAX];
> +	/* protects num_fw_queued */
> +	spinlock_t sta_tx_lock;
> +	wait_queue_head_t empty_tx_wq;
> +	unsigned int num_fw_queued;

is there a reason to prefer a spinlocked value instead of using an atomic without additional locking?