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@xxxxxxxxx>
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@xxxxxxxxxxxx>
> ---
> 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?
