Improve ath10k flush queue mechanism
cover Improve ath10k flush queue mechanism
It has been reported [0] that a 3-4 seconds (actually up to 5 sec) of radio silence could be observed followed by the error below on ath10k devices:
ath10k_pci 0000:04:00.0: failed to flush transmit queue (skip 0 ar-state 1): 0
This is due to how the TX queues are flushed in ath10k. When a STA is removed, mac80211 need to flush queues [1], but because ath10k does not have a lightweight .flush_sta operation, ieee80211_flush_queues() is called instead effectively blocking the whole queue during the drain causing this radio silence. Also because ath10k_flush() waits for all queued to be emptied, not only the flushed ones it could more easily take up to 5 seconds to finish making the whole situation worst.
The first patch of this series adds a .flush_sta operation to flush only specific STA traffic avoiding the need to stop whole queues and should be enough in itself to fix the reported issue.
The second patch of this series is a proposal to improve ath10k_flush so that it will be less likely to timeout waiting for non related queues to drain.
The abose kernel warning could still be observed (e.g. flushing a dead STA) but should be now harmless.
[0]: https://lore.kernel.org/all/CA+Xfe4FjUmzM5mvPxGbpJsF3SvSdE5_wgxvgFJ0bsdrKODVXCQ@mail.gmail.com/
[1]: commit 0b75a1b1e42e ("wifi: mac80211: flush queues on STA removal")
Remi Pommarel (2):
wifi: ath10k: Implement ieee80211 flush_sta callback
wifi: ath10k: Flush only requested txq in ath10k_flush()
drivers/net/wireless/ath/ath10k/core.h | 4 ++ drivers/net/wireless/ath/ath10k/htt.h | 11 +++- drivers/net/wireless/ath/ath10k/htt_tx.c | 50 +++++++++++++++- drivers/net/wireless/ath/ath10k/mac.c | 76 ++++++++++++++++++++---- drivers/net/wireless/ath/ath10k/txrx.c | 5 +- 5 files changed, 129 insertions(+), 17 deletions(-)
Signature · 1 line
2.40.0
PATCH 1/2 wifi: ath10k: Implement ieee80211 flush_sta callback
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")
--- 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(-)
drivers/net/wireless/ath/ath10k/core.h +4 -0
@@ -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; };#define ATH10K_VDEV_SETUP_TIMEOUT_HZ (5 * HZ)
drivers/net/wireless/ath/ath10k/htt.h +4 -0
@@ -2452,6 +2452,10 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt);void ath10k_htt_tx_mgmt_dec_pending (struct ath10k_htt *htt);int ath10k_htt_tx_mgmt_inc_pending (struct ath10k_htt *htt,bool is_mgmt,bool is_presp); +void ath10k_htt_tx_sta_inc_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta); +void ath10k_htt_tx_sta_dec_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta);int ath10k_htt_tx_alloc_msdu_id (struct ath10k_htt *htt,struct sk_buff *skb);void ath10k_htt_tx_free_msdu_id (struct ath10k_htt *htt,u16 msdu_id);
drivers/net/wireless/ath/ath10k/htt_tx.c +32 -0
@@ -195,6 +195,38 @@ void ath10k_htt_tx_mgmt_dec_pending(struct ath10k_htt *htt) htt->num_pending_mgmt_tx--; } +void ath10k_htt_tx_sta_inc_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta; + +if (!sta) +return ; + + arsta = (struct ath10k_sta *)sta->drv_priv; + +spin_lock_bh (&arsta->sta_tx_lock); + arsta->num_fw_queued++; +spin_unlock_bh (&arsta->sta_tx_lock); +} + +void ath10k_htt_tx_sta_dec_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta; + +if (!sta) +return ; + + arsta = (struct ath10k_sta *)sta->drv_priv; + +spin_lock_bh (&arsta->sta_tx_lock); + arsta->num_fw_queued--; +if (arsta->num_fw_queued ==0 ) +wake_up (&arsta->empty_tx_wq); +spin_unlock_bh (&arsta->sta_tx_lock); +} +int ath10k_htt_tx_alloc_msdu_id (struct ath10k_htt *htt,struct sk_buff *skb) {struct ath10k *ar = htt->ar;
drivers/net/wireless/ath/ath10k/mac.c +42 -1
73 lines not on this page · raw message
drivers/net/wireless/ath/ath10k/txrx.c +3 -0
@@ -91,6 +91,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, skb_cb->airtime_est, 0);rcu_read_unlock (); +if (txq) +ath10k_htt_tx_sta_dec_pending (htt, txq->sta); +if (ar->bus_param.dev_type !=ATH10K_DEV_TYPE_HL )dma_unmap_single (dev, skb_cb->paddr, msdu->len,DMA_TO_DEVICE );
Signature · 1 line
2.40.0
Re: [PATCH 1/2] wifi: ath10k: Implement ieee80211 flush_sta callback
Quoted 21 lines · Remi Pommarel
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 theflush_stacallback,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 (seeATH10K_FLUSH_TIMEOUT_HZ).
Quoted 7 lines
Implement a
flush_stacallback 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 <[email protected]>
checkpatch.pl reports:
WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
Quoted 14 lines
Signed-off-by Remi Pommarel <[email protected]>--- 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(-)drivers/net/wireless/ath/ath10k/core.h +0
-0
drivers/net/wireless/ath/ath10k/core.h struct ath10k_sta { · line 564
Quoted 7 lines
drivers/net/wireless/ath/ath10k/core.h +4
-0@@ -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?
Re: [PATCH 1/2] wifi: ath10k: Implement ieee80211 flush_sta callback
Quoted 26 lines · Jeff Johnson
Quoted 25 lines · Remi Pommarel
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 theflush_stacallback,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 (seeATH10K_FLUSH_TIMEOUT_HZ).Implement a
flush_stacallback in ath10k to wait only for a specific STA pending frames to be drained (without stopping the whole HW queue) to fix that.
Quoted 6 lines
Quoted 3 lines
[0]: commit
0b75a1b1e42e("wifi: mac80211: flush queues on STA removal")Reported-by Cedric Veilleux <[email protected]>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 ?
Quoted 10 lines
Quoted 10 lines
Signed-off-by Remi Pommarel <[email protected]>--- 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(-)
Quoted 14 lines
drivers/net/wireless/ath/ath10k/core.h struct ath10k_sta { · line 564
Quoted 12 lines
drivers/net/wireless/ath/ath10k/core.h +4
-0@@ -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,
Signature · 1 line
Remi
Re: [PATCH 1/2] wifi: ath10k: Implement ieee80211 flush_sta callback
In reply to Remi PommarelQuoted 197 lines · Remi Pommarel
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 theflush_stacallback,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 (seeATH10K_FLUSH_TIMEOUT_HZ).Implement a
flush_stacallback 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 <[email protected]>Signed-off-by Remi Pommarel <[email protected]>--- 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(-)drivers/net/wireless/ath/ath10k/core.h +4
-0@@ -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; };#define ATH10K_VDEV_SETUP_TIMEOUT_HZ (5 * HZ)drivers/net/wireless/ath/ath10k/htt.h +4
-0@@ -2452,6 +2452,10 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt);void ath10k_htt_tx_mgmt_dec_pending (struct ath10k_htt *htt);int ath10k_htt_tx_mgmt_inc_pending (struct ath10k_htt *htt,bool is_mgmt,bool is_presp); +void ath10k_htt_tx_sta_inc_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta); +void ath10k_htt_tx_sta_dec_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta);int ath10k_htt_tx_alloc_msdu_id (struct ath10k_htt *htt,struct sk_buff *skb);void ath10k_htt_tx_free_msdu_id (struct ath10k_htt *htt,u16 msdu_id);drivers/net/wireless/ath/ath10k/htt_tx.c +32
-0@@ -195,6 +195,38 @@ void ath10k_htt_tx_mgmt_dec_pending(struct ath10k_htt *htt) htt->num_pending_mgmt_tx--; } +void ath10k_htt_tx_sta_inc_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta; + +if (!sta) +return ; + + arsta = (struct ath10k_sta *)sta->drv_priv; + +spin_lock_bh (&arsta->sta_tx_lock); + arsta->num_fw_queued++; +spin_unlock_bh (&arsta->sta_tx_lock); +} + +void ath10k_htt_tx_sta_dec_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta; + +if (!sta) +return ; + + arsta = (struct ath10k_sta *)sta->drv_priv; + +spin_lock_bh (&arsta->sta_tx_lock); + arsta->num_fw_queued--; +if (arsta->num_fw_queued ==0 ) +wake_up (&arsta->empty_tx_wq); +spin_unlock_bh (&arsta->sta_tx_lock); +} +int ath10k_htt_tx_alloc_msdu_id (struct ath10k_htt *htt,struct sk_buff *skb) {struct ath10k *ar = htt->ar;drivers/net/wireless/ath/ath10k/mac.c +42
-1@@ -4423,6 +4423,8 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,spin_unlock_bh (&ar->htt.tx_lock); } +ath10k_htt_tx_sta_inc_pending (&ar->htt, sta); + ret =ath10k_mac_tx (ar, vif, txmode, txpath, skb, false);if (unlikely (ret)) {ath10k_warn (ar,"failed to push frame: %d\n" , ret);@@ -4432,6 +4434,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,if (is_mgmt)ath10k_htt_tx_mgmt_dec_pending (htt);spin_unlock_bh (&ar->htt.tx_lock); +ath10k_htt_tx_sta_dec_pending (&ar->htt, sta);return ret; }@@ -7474,7 +7477,8 @@ static int ath10k_sta_state(struct ieee80211_hw *hw, arsta->peer_ps_state =WMI_PEER_PS_STATE_DISABLED ;INIT_WORK (&arsta->update_wk, ath10k_sta_rc_update_wk);INIT_WORK (&arsta->tid_config_wk, ath10k_sta_tid_cfg_wk); - +spin_lock_init (&arsta->sta_tx_lock); +init_waitqueue_head (&arsta->empty_tx_wq);for (i =0 ; i <ARRAY_SIZE (sta->txq); i++)ath10k_mac_txq_init (sta->txq[i]); }@@ -8098,6 +8102,42 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,mutex_unlock (&ar->conf_mutex); } +static void ath10k_flush_sta (struct ieee80211_hw *hw,struct ieee80211_vif *vif, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv; +struct ath10k *ar = hw->priv; +bool skip; +long time_left; + +/* TODO do we need drop implemented here ? */ + +mutex_lock (&ar->conf_mutex); + +if (ar->state ==ATH10K_STATE_WEDGED ) + goto out; + + time_left =wait_event_timeout (arsta->empty_tx_wq, ({ +bool empty; + +spin_lock_bh (&arsta->sta_tx_lock); + empty = (arsta->num_fw_queued ==0 ); +spin_unlock_bh (&arsta->sta_tx_lock); + + skip = (ar->state ==ATH10K_STATE_WEDGED ) || +test_bit (ATH10K_FLAG_CRASH_FLUSH , + &ar->dev_flags); + + (empty || skip); + }),ATH10K_FLUSH_TIMEOUT_HZ ); + +if (time_left ==0 || skip) +ath10k_warn (ar,"failed to flush sta txq (sta %pM skip %i ar-state %i): %ld\n" , + sta->addr, skip, ar->state, time_left); +out: +mutex_unlock (&ar->conf_mutex); +} +/* TODO: Implement this function properly * For now it is needed to reply to Probe Requests in IBSS mode. * Probably we need this information from FW. @@ -9444,6 +9484,7 @@ static const struct ieee80211_ops ath10k_ops = { .set_rts_threshold = ath10k_set_rts_threshold, .set_frag_threshold = ath10k_mac_op_set_frag_threshold, .flush = ath10k_flush, + .flush_sta = ath10k_flush_sta, .tx_last_beacon = ath10k_tx_last_beacon, .set_antenna = ath10k_set_antenna, .get_antenna = ath10k_get_antenna,drivers/net/wireless/ath/ath10k/txrx.c +0
-0
drivers/net/wireless/ath/ath10k/txrx.c int ath10k_txrx_tx_unref(struct ath10k_htt *htt, · line 96
Quoted 6 lines
drivers/net/wireless/ath/ath10k/txrx.c +3
-0@@ -91,6 +91,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, skb_cb->airtime_est, 0);rcu_read_unlock (); +if (txq) +ath10k_htt_tx_sta_dec_pending (htt, txq->sta); +
This should be called within rcu?
Vasanth
Re: [PATCH 1/2] wifi: ath10k: Implement ieee80211 flush_sta callback
Quoted 193 lines · Vasanthakumar Thiagarajan
Quoted 190 lines · Remi Pommarel
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 theflush_stacallback,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 (seeATH10K_FLUSH_TIMEOUT_HZ).Implement a
flush_stacallback 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 <[email protected]>Signed-off-by Remi Pommarel <[email protected]>--- 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(-)drivers/net/wireless/ath/ath10k/core.h +4
-0@@ -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; };#define ATH10K_VDEV_SETUP_TIMEOUT_HZ (5 * HZ)drivers/net/wireless/ath/ath10k/htt.h +4
-0@@ -2452,6 +2452,10 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt);void ath10k_htt_tx_mgmt_dec_pending (struct ath10k_htt *htt);int ath10k_htt_tx_mgmt_inc_pending (struct ath10k_htt *htt,bool is_mgmt,bool is_presp); +void ath10k_htt_tx_sta_inc_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta); +void ath10k_htt_tx_sta_dec_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta);int ath10k_htt_tx_alloc_msdu_id (struct ath10k_htt *htt,struct sk_buff *skb);void ath10k_htt_tx_free_msdu_id (struct ath10k_htt *htt,u16 msdu_id);drivers/net/wireless/ath/ath10k/htt_tx.c +32
-0@@ -195,6 +195,38 @@ void ath10k_htt_tx_mgmt_dec_pending(struct ath10k_htt *htt) htt->num_pending_mgmt_tx--; } +void ath10k_htt_tx_sta_inc_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta; + +if (!sta) +return ; + + arsta = (struct ath10k_sta *)sta->drv_priv; + +spin_lock_bh (&arsta->sta_tx_lock); + arsta->num_fw_queued++; +spin_unlock_bh (&arsta->sta_tx_lock); +} + +void ath10k_htt_tx_sta_dec_pending (struct ath10k_htt *htt, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta; + +if (!sta) +return ; + + arsta = (struct ath10k_sta *)sta->drv_priv; + +spin_lock_bh (&arsta->sta_tx_lock); + arsta->num_fw_queued--; +if (arsta->num_fw_queued ==0 ) +wake_up (&arsta->empty_tx_wq); +spin_unlock_bh (&arsta->sta_tx_lock); +} +int ath10k_htt_tx_alloc_msdu_id (struct ath10k_htt *htt,struct sk_buff *skb) {struct ath10k *ar = htt->ar;drivers/net/wireless/ath/ath10k/mac.c +42
-1@@ -4423,6 +4423,8 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,spin_unlock_bh (&ar->htt.tx_lock); } +ath10k_htt_tx_sta_inc_pending (&ar->htt, sta); + ret =ath10k_mac_tx (ar, vif, txmode, txpath, skb, false);if (unlikely (ret)) {ath10k_warn (ar,"failed to push frame: %d\n" , ret);@@ -4432,6 +4434,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,if (is_mgmt)ath10k_htt_tx_mgmt_dec_pending (htt);spin_unlock_bh (&ar->htt.tx_lock); +ath10k_htt_tx_sta_dec_pending (&ar->htt, sta);return ret; }@@ -7474,7 +7477,8 @@ static int ath10k_sta_state(struct ieee80211_hw *hw, arsta->peer_ps_state =WMI_PEER_PS_STATE_DISABLED ;INIT_WORK (&arsta->update_wk, ath10k_sta_rc_update_wk);INIT_WORK (&arsta->tid_config_wk, ath10k_sta_tid_cfg_wk); - +spin_lock_init (&arsta->sta_tx_lock); +init_waitqueue_head (&arsta->empty_tx_wq);for (i =0 ; i <ARRAY_SIZE (sta->txq); i++)ath10k_mac_txq_init (sta->txq[i]); }@@ -8098,6 +8102,42 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,mutex_unlock (&ar->conf_mutex); } +static void ath10k_flush_sta (struct ieee80211_hw *hw,struct ieee80211_vif *vif, +struct ieee80211_sta *sta) +{ +struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv; +struct ath10k *ar = hw->priv; +bool skip; +long time_left; + +/* TODO do we need drop implemented here ? */ + +mutex_lock (&ar->conf_mutex); + +if (ar->state ==ATH10K_STATE_WEDGED ) + goto out; + + time_left =wait_event_timeout (arsta->empty_tx_wq, ({ +bool empty; + +spin_lock_bh (&arsta->sta_tx_lock); + empty = (arsta->num_fw_queued ==0 ); +spin_unlock_bh (&arsta->sta_tx_lock); + + skip = (ar->state ==ATH10K_STATE_WEDGED ) || +test_bit (ATH10K_FLAG_CRASH_FLUSH , + &ar->dev_flags); + + (empty || skip); + }),ATH10K_FLUSH_TIMEOUT_HZ ); + +if (time_left ==0 || skip) +ath10k_warn (ar,"failed to flush sta txq (sta %pM skip %i ar-state %i): %ld\n" , + sta->addr, skip, ar->state, time_left); +out: +mutex_unlock (&ar->conf_mutex); +} +/* TODO: Implement this function properly * For now it is needed to reply to Probe Requests in IBSS mode. * Probably we need this information from FW. @@ -9444,6 +9484,7 @@ static const struct ieee80211_ops ath10k_ops = { .set_rts_threshold = ath10k_set_rts_threshold, .set_frag_threshold = ath10k_mac_op_set_frag_threshold, .flush = ath10k_flush, + .flush_sta = ath10k_flush_sta, .tx_last_beacon = ath10k_tx_last_beacon, .set_antenna = ath10k_set_antenna, .get_antenna = ath10k_get_antenna,drivers/net/wireless/ath/ath10k/txrx.c +0
-0
Quoted 8 lines
drivers/net/wireless/ath/ath10k/txrx.c int ath10k_txrx_tx_unref(struct ath10k_htt *htt, · line 95
Quoted 6 lines
drivers/net/wireless/ath/ath10k/txrx.c +3
-0@@ -91,6 +91,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, skb_cb->airtime_est, 0);rcu_read_unlock (); +if (txq) +ath10k_htt_tx_sta_dec_pending (htt, txq->sta); +This should be called within rcu?
According to [0] yes. But not sure to understand how that fixes the null pointer dereference here as txq->sta is never set to NULL elsewhere and no rcu_dereference is used in rcu critical section. The only things I can think of is that it delays sta memory release past the rcu section. So yes maybe it is safer (and harmless) to put that within rcu read lock.
Waiting to know if sta pending should be atomic instead of spinlock protected and send v2 accordingly.
Thanks
[0]: commit acb31476adc9f ("ath10k: fix kernel null pointer dereference")
Signature · 4 lines
Remi -- Remi
PATCH 2/2 wifi: ath10k: Flush only requested txq in ath10k_flush()
In reply to Remi PommarelThe ieee80211 flush callback can be called to flush only part of all hw queues. The ath10k's flush callback implementation (i.e. ath10k_flush()) was waiting for all pending frames of all queues to be flushed ignoring the queue parameter. Because only the queues to be flushed are stopped by mac80211, skb can still be queued to other queues meanwhile. Thus ath10k_flush() could fail (and wait 5sec holding ar->conf lock) even if the requested queues are flushed correctly.
A way to reproduce the issue is to use two different APs because each vdev has its own hw queue in ath10k. Connect STA0 to AP0 and STA1 to AP1. Then generate traffic from AP0 to STA0 and kill STA0 without clean disassociation frame (e.g. unplug power cable, reboot -f, ...). Now if we were to flush AP1's queue, ath10k_flush() would fail (and effectively block 5 seconds with ar->conf or even wiphy's lock held) with the following warning:
ath10k_pci 0000:01:00.0: failed to flush transmit queue (skip 0 ar-state 2): 0
Wait only for pending frames of the requested queues to be flushed in ath10k_flush() to avoid that long blocking.
--- drivers/net/wireless/ath/ath10k/htt.h | 7 +++-- drivers/net/wireless/ath/ath10k/htt_tx.c | 18 ++++++++++--- drivers/net/wireless/ath/ath10k/mac.c | 33 +++++++++++++++++------- drivers/net/wireless/ath/ath10k/txrx.c | 2 +- 4 files changed, 44 insertions(+), 16 deletions(-)
drivers/net/wireless/ath/ath10k/htt.h +5 -2
@@ -1870,6 +1870,7 @@ struct ath10k_htt {spinlock_t tx_lock;int max_num_pending_tx;int num_pending_tx; +int pending_per_queue[IEEE80211_MAX_QUEUES ];int num_pending_mgmt_tx;struct idr pending_tx;wait_queue_head_t empty_tx_wq;
@@ -2447,8 +2448,10 @@ void ath10k_htt_tx_txq_update(struct ieee80211_hw *hw,void ath10k_htt_tx_txq_recalc (struct ieee80211_hw *hw,struct ieee80211_txq *txq);void ath10k_htt_tx_txq_sync (struct ath10k *ar); -void ath10k_htt_tx_dec_pending (struct ath10k_htt *htt); -int ath10k_htt_tx_inc_pending (struct ath10k_htt *htt); +void ath10k_htt_tx_dec_pending (struct ath10k_htt *htt, +struct ieee80211_txq *txq); +int ath10k_htt_tx_inc_pending (struct ath10k_htt *htt, +struct ieee80211_txq *txq);void ath10k_htt_tx_mgmt_dec_pending (struct ath10k_htt *htt);int ath10k_htt_tx_mgmt_inc_pending (struct ath10k_htt *htt,bool is_mgmt,bool is_presp);
drivers/net/wireless/ath/ath10k/htt_tx.c +15 -3
@@ -140,19 +140,26 @@ void ath10k_htt_tx_txq_update(struct ieee80211_hw *hw,spin_unlock_bh (&ar->htt.tx_lock); } -void ath10k_htt_tx_dec_pending (struct ath10k_htt *htt) +void ath10k_htt_tx_dec_pending (struct ath10k_htt *htt, +struct ieee80211_txq *txq) { +int num_txq =-1 ; +lockdep_assert_held (&htt->tx_lock); htt->num_pending_tx--;if (htt->num_pending_tx == htt->max_num_pending_tx -1 )ath10k_mac_tx_unlock (htt->ar,ATH10K_TX_PAUSE_Q_FULL ); -if (htt->num_pending_tx ==0 ) +if (txq) + num_txq = --htt->pending_per_queue[txq->vif->hw_queue[txq->ac]]; + +if (htt->num_pending_tx ==0 || num_txq ==0 )wake_up (&htt->empty_tx_wq); } -int ath10k_htt_tx_inc_pending (struct ath10k_htt *htt) +int ath10k_htt_tx_inc_pending (struct ath10k_htt *htt, +struct ieee80211_txq *txq) {lockdep_assert_held (&htt->tx_lock);
@@ -163,6 +170,11 @@ int ath10k_htt_tx_inc_pending(struct ath10k_htt *htt)if (htt->num_pending_tx == htt->max_num_pending_tx)ath10k_mac_tx_lock (htt->ar,ATH10K_TX_PAUSE_Q_FULL ); +if (!txq) +return 0 ; + + htt->pending_per_queue[txq->vif->hw_queue[txq->ac]]++; +return 0 ; }
drivers/net/wireless/ath/ath10k/mac.c +23 -10
@@ -4385,7 +4385,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,u16 airtime;spin_lock_bh (&ar->htt.tx_lock); - ret =ath10k_htt_tx_inc_pending (htt); + ret =ath10k_htt_tx_inc_pending (htt, txq);spin_unlock_bh (&ar->htt.tx_lock);if (ret)
@@ -4394,7 +4394,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw, skb =ieee80211_tx_dequeue_ni (hw, txq);if (!skb) {spin_lock_bh (&ar->htt.tx_lock); -ath10k_htt_tx_dec_pending (htt); +ath10k_htt_tx_dec_pending (htt, txq);spin_unlock_bh (&ar->htt.tx_lock);return -ENOENT ;
@@ -4416,7 +4416,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw, ret =ath10k_htt_tx_mgmt_inc_pending (htt, is_mgmt, is_presp);if (ret) { -ath10k_htt_tx_dec_pending (htt); +ath10k_htt_tx_dec_pending (htt, txq);spin_unlock_bh (&ar->htt.tx_lock);return ret; }
@@ -4430,7 +4430,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,ath10k_warn (ar,"failed to push frame: %d\n" , ret);spin_lock_bh (&ar->htt.tx_lock); -ath10k_htt_tx_dec_pending (htt); +ath10k_htt_tx_dec_pending (htt, txq);if (is_mgmt)ath10k_htt_tx_mgmt_dec_pending (htt);spin_unlock_bh (&ar->htt.tx_lock);
@@ -4693,7 +4693,7 @@ static void ath10k_mac_op_tx(struct ieee80211_hw *hw, is_presp =ieee80211_is_probe_resp (hdr->frame_control); } - ret =ath10k_htt_tx_inc_pending (htt); + ret =ath10k_htt_tx_inc_pending (htt, txq);if (ret) {ath10k_warn (ar,"failed to increase tx pending count: %d, dropping\n" , ret);
@@ -4706,7 +4706,7 @@ static void ath10k_mac_op_tx(struct ieee80211_hw *hw,if (ret) {ath10k_dbg (ar,ATH10K_DBG_MAC ,"failed to increase tx mgmt pending count: %d, dropping\n" , ret); -ath10k_htt_tx_dec_pending (htt); +ath10k_htt_tx_dec_pending (htt, txq);spin_unlock_bh (&ar->htt.tx_lock);ieee80211_free_txskb (ar->hw, skb);return ;
@@ -4719,7 +4719,7 @@ static void ath10k_mac_op_tx(struct ieee80211_hw *hw,ath10k_warn (ar,"failed to transmit frame: %d\n" , ret);if (is_htt) {spin_lock_bh (&ar->htt.tx_lock); -ath10k_htt_tx_dec_pending (htt); +ath10k_htt_tx_dec_pending (htt, txq);if (is_mgmt)ath10k_htt_tx_mgmt_dec_pending (htt);spin_unlock_bh (&ar->htt.tx_lock);
@@ -8046,10 +8046,12 @@ static int ath10k_mac_op_set_frag_threshold(struct ieee80211_hw *hw, u32 value)return -EOPNOTSUPP ; } -void ath10k_mac_wait_tx_complete (struct ath10k *ar) +static void _ath10k_mac_wait_tx_complete (struct ath10k *ar, +unsigned long queues) {bool skip;long time_left; +unsigned int q;/* mac80211 doesn't care if we really xmit queued frames or not * we'll collect those frames either way if we stop/delete vdevs
@@ -8062,7 +8064,11 @@ void ath10k_mac_wait_tx_complete(struct ath10k *ar)bool empty;spin_lock_bh (&ar->htt.tx_lock); - empty = (ar->htt.num_pending_tx ==0 ); +for_each_set_bit (q, &queues, ar->hw->queues) { + empty = (ar->htt.pending_per_queue[q] ==0 ); +if (!empty) +break ; + }spin_unlock_bh (&ar->htt.tx_lock); skip = (ar->state == ATH10K_STATE_WEDGED) ||
@@ -8077,6 +8083,13 @@ void ath10k_mac_wait_tx_complete(struct ath10k *ar) skip, ar->state, time_left); } +void ath10k_mac_wait_tx_complete (struct ath10k *ar) +{ +unsigned int queues =GENMASK (ar->hw->queues -1 ,0 ); + +_ath10k_mac_wait_tx_complete (ar, queues); +} +static void ath10k_flush (struct ieee80211_hw *hw,struct ieee80211_vif *vif,u32 queues,bool drop) {
@@ -8098,7 +8111,7 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, }mutex_lock (&ar->conf_mutex); -ath10k_mac_wait_tx_complete (ar); +_ath10k_mac_wait_tx_complete (ar, queues);mutex_unlock (&ar->conf_mutex); }
drivers/net/wireless/ath/ath10k/txrx.c +1 -1
@@ -82,7 +82,7 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt, flags = skb_cb->flags;ath10k_htt_tx_free_msdu_id (htt, tx_done->msdu_id); -ath10k_htt_tx_dec_pending (htt); +ath10k_htt_tx_dec_pending (htt, txq);spin_unlock_bh (&htt->tx_lock);rcu_read_lock ();
Signature · 1 line
2.40.0
Re: [PATCH 2/2] wifi: ath10k: Flush only requested txq in ath10k_flush()
Quoted 16 lines · Remi Pommarel
The ieee80211 flush callback can be called to flush only part of all hw queues. The ath10k's flush callback implementation (i.e.
ath10k_flush()) was waiting for all pending frames of all queues to be flushed ignoring the queue parameter. Because only the queues to be flushed are stopped by mac80211, skb can still be queued to other queues meanwhile. Thusath10k_flush()could fail (and wait 5sec holdingar->conflock) even if the requested queues are flushed correctly.A way to reproduce the issue is to use two different APs because each vdev has its own hw queue in ath10k. Connect STA0 to AP0 and STA1 to AP1. Then generate traffic from AP0 to STA0 and kill STA0 without clean disassociation frame (e.g. unplug power cable, reboot -f, ...). Now if we were to flush AP1's queue,
ath10k_flush()would fail (and effectively block 5 seconds withar->confor even wiphy's lock held) with the following warning:
Quoted 6 lines
ath10k_pci0000:01:00.0: failed to flush transmit queue (skip 0 ar-state 2): 0Wait only for pending frames of the requested queues to be flushed in
ath10k_flush()to avoid that long blocking.Reported-by Cedric Veilleux <[email protected]>
WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
Quoted 7 lines
Signed-off-by Remi Pommarel <[email protected]>--- drivers/net/wireless/ath/ath10k/htt.h | 7 +++-- drivers/net/wireless/ath/ath10k/htt_tx.c | 18 ++++++++++--- drivers/net/wireless/ath/ath10k/mac.c | 33 +++++++++++++++++------- drivers/net/wireless/ath/ath10k/txrx.c | 2 +- 4 files changed, 44 insertions(+), 16 deletions(-)
Re: [PATCH 2/2] wifi: ath10k: Flush only requested txq in ath10k_flush()
In reply to Remi PommarelQuoted 35 lines · Remi Pommarel
The ieee80211 flush callback can be called to flush only part of all hw queues. The ath10k's flush callback implementation (i.e.
ath10k_flush()) was waiting for all pending frames of all queues to be flushed ignoring the queue parameter. Because only the queues to be flushed are stopped by mac80211, skb can still be queued to other queues meanwhile. Thusath10k_flush()could fail (and wait 5sec holdingar->conflock) even if the requested queues are flushed correctly.A way to reproduce the issue is to use two different APs because each vdev has its own hw queue in ath10k. Connect STA0 to AP0 and STA1 to AP1. Then generate traffic from AP0 to STA0 and kill STA0 without clean disassociation frame (e.g. unplug power cable, reboot -f, ...). Now if we were to flush AP1's queue,
ath10k_flush()would fail (and effectively block 5 seconds withar->confor even wiphy's lock held) with the following warning:
ath10k_pci0000:01:00.0: failed to flush transmit queue (skip 0 ar-state 2): 0Wait only for pending frames of the requested queues to be flushed in
ath10k_flush()to avoid that long blocking.Reported-by Cedric Veilleux <[email protected]>Signed-off-by Remi Pommarel <[email protected]>--- drivers/net/wireless/ath/ath10k/htt.h | 7 +++-- drivers/net/wireless/ath/ath10k/htt_tx.c | 18 ++++++++++--- drivers/net/wireless/ath/ath10k/mac.c | 33 +++++++++++++++++------- drivers/net/wireless/ath/ath10k/txrx.c | 2 +- 4 files changed, 44 insertions(+), 16 deletions(-)drivers/net/wireless/ath/ath10k/htt.h +0
-0
drivers/net/wireless/ath/ath10k/htt.h struct ath10k_htt { · line 1873
Quoted 4 lines
drivers/net/wireless/ath/ath10k/htt.h +1
-0@@ -1870,6 +1870,7 @@ struct ath10k_htt {spinlock_t tx_lock;int max_num_pending_tx;int num_pending_tx; +int pending_per_queue[IEEE80211_MAX_QUEUES ];
Something like num_pending_per_queue[] to align with the existing packet counter looks better?
Vasanth
Quoted 32 lines · Remi Pommarel
It has been reported [0] that a 3-4 seconds (actually up to 5 sec) of radio silence could be observed followed by the error below on ath10k devices:
ath10k_pci0000:04:00.0: failed to flush transmit queue (skip 0 ar-state 1): 0This is due to how the TX queues are flushed in ath10k. When a STA is removed, mac80211 need to flush queues [1], but because ath10k does not have a lightweight .flush_sta operation,
ieee80211_flush_queues()is called instead effectively blocking the whole queue during the drain causing this radio silence. Also becauseath10k_flush()waits for all queued to be emptied, not only the flushed ones it could more easily take up to 5 seconds to finish making the whole situation worst.The first patch of this series adds a .flush_sta operation to flush only specific STA traffic avoiding the need to stop whole queues and should be enough in itself to fix the reported issue.
The second patch of this series is a proposal to improve
ath10k_flushso that it will be less likely to timeout waiting for non related queues to drain.The abose kernel warning could still be observed (e.g. flushing a dead STA) but should be now harmless.
[0]: https://lore.kernel.org/all/CA+Xfe4FjUmzM5mvPxGbpJsF3SvSdE5_wgxvgFJ0bsdrKODVXCQ@mail.gmail.com/
[1]: commit0b75a1b1e42e("wifi: mac80211: flush queues on STA removal")Remi Pommarel (2):
wifi: ath10k: Implement ieee80211flush_stacallback
wifi: ath10k: Flush only requested txq inath10k_flush()
Quoted 7 lines
drivers/net/wireless/ath/ath10k/core.h | 4 ++ drivers/net/wireless/ath/ath10k/htt.h | 11 +++- drivers/net/wireless/ath/ath10k/htt_tx.c | 50 +++++++++++++++- drivers/net/wireless/ath/ath10k/mac.c | 76 ++++++++++++++++++++---- drivers/net/wireless/ath/ath10k/txrx.c | 5 +- 5 files changed, 129 insertions(+), 17 deletions(-)
LGTM wit a few nits.
Hope the Qualcomm ath10k folks review this.
/jeff