On Thu, Oct 17, 2024 at 02:19:51PM -0700, Jeff Johnson wrote:
> 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
It has been reported on mailing list should I put the thread link here ?
>
> > 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?
No reason except to mimic what is done for num_pending. Can move that to
atomic if needed be.
Thanks,
--
Remi
