* [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions
@ 2024-11-13 1:56 Baochen Qiang
2024-11-13 1:56 ` [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() Baochen Qiang
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw)
To: ath11k; +Cc: linux-wireless, quic_bqiang
We get report [1] that CPU is running in hot loop while requesting firmware stats,
fix it in patch [1/6]. While at it, fix potential failures due to static variables
in patch [2/6]. patch [3/6] fix potential issues in cases ath11k debugfs is not
enabled. patch [4/6] fix lock symmetry issue. and the last two patches refactor
firmware stats request helpers such that we can remove some redundant code.
[1] https://lore.kernel.org/all/7324ac7a-8b7a-42a5-aa19-de52138ff638@app.fastmail.com
Baochen Qiang (6):
wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request()
wifi: ath11k: don't use static variables in
ath11k_debugfs_fw_stats_process()
wifi: ath11k: move some firmware stats related functions outside of
debugfs
wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event()
wifi: ath11k: move locking outside of ath11k_mac_get_fw_stats()
wifi: ath11k: consistently use ath11k_mac_get_fw_stats()
drivers/net/wireless/ath/ath11k/core.h | 2 +
drivers/net/wireless/ath/ath11k/debugfs.c | 139 ++-------------------
drivers/net/wireless/ath/ath11k/debugfs.h | 10 +-
drivers/net/wireless/ath/ath11k/mac.c | 140 +++++++++++++++-------
drivers/net/wireless/ath/ath11k/mac.h | 4 +-
drivers/net/wireless/ath/ath11k/wmi.c | 45 ++++++-
6 files changed, 150 insertions(+), 190 deletions(-)
base-commit: fc6f018eda7f9054e427f731db1e8b200f22873c
--
2.25.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang @ 2024-11-13 1:56 ` Baochen Qiang 2024-11-13 11:12 ` Kalle Valo 2024-11-13 1:56 ` [PATCH 2/6] wifi: ath11k: don't use static variables in ath11k_debugfs_fw_stats_process() Baochen Qiang ` (4 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, quic_bqiang We get report [1] that CPU is running a hot loop in ath11k_debugfs_fw_stats_request(): 94.60% 0.00% i3status [kernel.kallsyms] [k] do_syscall_64 | --94.60%--do_syscall_64 | --94.55%--__sys_sendmsg ___sys_sendmsg ____sys_sendmsg netlink_sendmsg netlink_unicast genl_rcv netlink_rcv_skb genl_rcv_msg | --94.55%--genl_family_rcv_msg_dumpit __netlink_dump_start netlink_dump genl_dumpit nl80211_dump_station | --94.55%--ieee80211_dump_station sta_set_sinfo | --94.55%--ath11k_mac_op_sta_statistics ath11k_debugfs_get_fw_stats | --94.55%--ath11k_debugfs_fw_stats_request | |--41.73%--_raw_spin_lock_bh | |--22.74%--__local_bh_enable_ip | |--9.22%--_raw_spin_unlock_bh | --6.66%--srso_alias_safe_ret This is because, if for whatever reason ar->fw_stats_done is not set by ath11k_update_stats_event(), ath11k_debugfs_fw_stats_request() won't yield CPU before an up to 3s timeout. Add 100ms sleep to avoid CPU burning. Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices") Reported-by: Yury Vostrikov <mon@unformed.ru> Closes: https://lore.kernel.org/all/7324ac7a-8b7a-42a5-aa19-de52138ff638@app.fastmail.com/ # [1] Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- drivers/net/wireless/ath/ath11k/debugfs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index 57281a135dd7..a5e0f2092da5 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -207,6 +207,9 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, break; } spin_unlock_bh(&ar->data_lock); + + /* 100ms is empirical, change if required */ + msleep(100); } return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() 2024-11-13 1:56 ` [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() Baochen Qiang @ 2024-11-13 11:12 ` Kalle Valo 2024-11-14 5:34 ` Baochen Qiang 0 siblings, 1 reply; 15+ messages in thread From: Kalle Valo @ 2024-11-13 11:12 UTC (permalink / raw) To: Baochen Qiang; +Cc: ath11k, linux-wireless Baochen Qiang <quic_bqiang@quicinc.com> writes: > We get report [1] that CPU is running a hot loop in > ath11k_debugfs_fw_stats_request(): > > 94.60% 0.00% i3status [kernel.kallsyms] [k] do_syscall_64 > | > --94.60%--do_syscall_64 > | > --94.55%--__sys_sendmsg > ___sys_sendmsg > ____sys_sendmsg > netlink_sendmsg > netlink_unicast > genl_rcv > netlink_rcv_skb > genl_rcv_msg > | > --94.55%--genl_family_rcv_msg_dumpit > __netlink_dump_start > netlink_dump > genl_dumpit > nl80211_dump_station > | > --94.55%--ieee80211_dump_station > sta_set_sinfo > | > --94.55%--ath11k_mac_op_sta_statistics > ath11k_debugfs_get_fw_stats > | > --94.55%--ath11k_debugfs_fw_stats_request > | > |--41.73%--_raw_spin_lock_bh > | > |--22.74%--__local_bh_enable_ip > | > |--9.22%--_raw_spin_unlock_bh > | > --6.66%--srso_alias_safe_ret > > This is because, if for whatever reason ar->fw_stats_done is not set by > ath11k_update_stats_event(), ath11k_debugfs_fw_stats_request() won't yield > CPU before an up to 3s timeout. > > Add 100ms sleep to avoid CPU burning. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 > > Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices") > Reported-by: Yury Vostrikov <mon@unformed.ru> > Closes: https://lore.kernel.org/all/7324ac7a-8b7a-42a5-aa19-de52138ff638@app.fastmail.com/ # [1] > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/debugfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c > index 57281a135dd7..a5e0f2092da5 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > @@ -207,6 +207,9 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > break; > } > spin_unlock_bh(&ar->data_lock); > + > + /* 100ms is empirical, change if required */ > + msleep(100); > } > return 0; > } Please don't reinvent the wheel. Why not just use completion instead of this ugly spinning hack? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() 2024-11-13 11:12 ` Kalle Valo @ 2024-11-14 5:34 ` Baochen Qiang 0 siblings, 0 replies; 15+ messages in thread From: Baochen Qiang @ 2024-11-14 5:34 UTC (permalink / raw) To: Kalle Valo; +Cc: ath11k, linux-wireless On 11/13/2024 7:12 PM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >> We get report [1] that CPU is running a hot loop in >> ath11k_debugfs_fw_stats_request(): >> >> 94.60% 0.00% i3status [kernel.kallsyms] [k] do_syscall_64 >> | >> --94.60%--do_syscall_64 >> | >> --94.55%--__sys_sendmsg >> ___sys_sendmsg >> ____sys_sendmsg >> netlink_sendmsg >> netlink_unicast >> genl_rcv >> netlink_rcv_skb >> genl_rcv_msg >> | >> --94.55%--genl_family_rcv_msg_dumpit >> __netlink_dump_start >> netlink_dump >> genl_dumpit >> nl80211_dump_station >> | >> --94.55%--ieee80211_dump_station >> sta_set_sinfo >> | >> --94.55%--ath11k_mac_op_sta_statistics >> ath11k_debugfs_get_fw_stats >> | >> --94.55%--ath11k_debugfs_fw_stats_request >> | >> |--41.73%--_raw_spin_lock_bh >> | >> |--22.74%--__local_bh_enable_ip >> | >> |--9.22%--_raw_spin_unlock_bh >> | >> --6.66%--srso_alias_safe_ret >> >> This is because, if for whatever reason ar->fw_stats_done is not set by >> ath11k_update_stats_event(), ath11k_debugfs_fw_stats_request() won't yield >> CPU before an up to 3s timeout. >> >> Add 100ms sleep to avoid CPU burning. >> >> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 >> >> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices") >> Reported-by: Yury Vostrikov <mon@unformed.ru> >> Closes: https://lore.kernel.org/all/7324ac7a-8b7a-42a5-aa19-de52138ff638@app.fastmail.com/ # [1] >> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> >> --- >> drivers/net/wireless/ath/ath11k/debugfs.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c >> index 57281a135dd7..a5e0f2092da5 100644 >> --- a/drivers/net/wireless/ath/ath11k/debugfs.c >> +++ b/drivers/net/wireless/ath/ath11k/debugfs.c >> @@ -207,6 +207,9 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, >> break; >> } >> spin_unlock_bh(&ar->data_lock); >> + >> + /* 100ms is empirical, change if required */ >> + msleep(100); >> } >> return 0; >> } > > Please don't reinvent the wheel. Why not just use completion instead of > this ugly spinning hack? previously I was thinking that a wait_for_completion() is not allowed since we are in atomic context due to spin_lock. But thinking it more I find that after changing to use completion, the spin_lock is not necessary. so thanks, will do in next version. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/6] wifi: ath11k: don't use static variables in ath11k_debugfs_fw_stats_process() 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang 2024-11-13 1:56 ` [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() Baochen Qiang @ 2024-11-13 1:56 ` Baochen Qiang 2024-11-13 11:15 ` Kalle Valo 2024-11-13 1:56 ` [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs Baochen Qiang ` (3 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, quic_bqiang Currently ath11k_debugfs_fw_stats_process() is using static variables to count firmware stat events. Taking num_vdev as an example, if for whatever reason ( say ar->num_started_vdevs is 0 or firmware bug etc.) the following condition (++num_vdev) == total_vdevs_started is not met, is_end is not set thus num_vdev won't be cleared. Next time when firmware stats is requested again, even if everything is working fine, we will fail due to the condition above will never be satisfied. The same applies to num_bcn as well. Change to use non-static counters so that we have a chance to clear them each time firmware stats is requested. Currently only ath11k_fw_stats_request() and ath11k_debugfs_fw_stats_request() are requesting firmware stats, so clear counters there. Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 Fixes: da3a9d3c1576 ("ath11k: refactor debugfs code into debugfs.c") Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- drivers/net/wireless/ath/ath11k/core.h | 2 ++ drivers/net/wireless/ath/ath11k/debugfs.c | 16 +++++++--------- drivers/net/wireless/ath/ath11k/mac.c | 2 ++ 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index 09c37e19a168..35d2eee2f91f 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -599,6 +599,8 @@ struct ath11k_fw_stats { struct list_head pdevs; struct list_head vdevs; struct list_head bcn; + u32 num_vdev_recvd; + u32 num_bcn_recvd; }; struct ath11k_dbg_htt_stats { diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index a5e0f2092da5..fd0d2a057084 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -99,6 +99,8 @@ static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) ar->fw_stats_done = false; ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); + ar->fw_stats.num_vdev_recvd = 0; + ar->fw_stats.num_bcn_recvd = 0; spin_unlock_bh(&ar->data_lock); } @@ -107,7 +109,6 @@ void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats * struct ath11k_base *ab = ar->ab; struct ath11k_pdev *pdev; bool is_end; - static unsigned int num_vdev, num_bcn; size_t total_vdevs_started = 0; int i; @@ -132,15 +133,14 @@ void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats * total_vdevs_started += ar->num_started_vdevs; } - is_end = ((++num_vdev) == total_vdevs_started); + is_end = ((++ar->fw_stats.num_vdev_recvd) == total_vdevs_started); list_splice_tail_init(&stats->vdevs, &ar->fw_stats.vdevs); - if (is_end) { + if (is_end) ar->fw_stats_done = true; - num_vdev = 0; - } + return; } @@ -152,15 +152,13 @@ void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats * /* Mark end until we reached the count of all started VDEVs * within the PDEV */ - is_end = ((++num_bcn) == ar->num_started_vdevs); + is_end = ((++ar->fw_stats.num_bcn_recvd) == ar->num_started_vdevs); list_splice_tail_init(&stats->bcn, &ar->fw_stats.bcn); - if (is_end) { + if (is_end) ar->fw_stats_done = true; - num_bcn = 0; - } } } diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index e6acbff06749..f147ef960c22 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -9334,6 +9334,8 @@ static int ath11k_fw_stats_request(struct ath11k *ar, spin_lock_bh(&ar->data_lock); ar->fw_stats_done = false; ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); + ar->fw_stats.num_vdev_recvd = 0; + ar->fw_stats.num_bcn_recvd = 0; spin_unlock_bh(&ar->data_lock); reinit_completion(&ar->fw_stats_complete); -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] wifi: ath11k: don't use static variables in ath11k_debugfs_fw_stats_process() 2024-11-13 1:56 ` [PATCH 2/6] wifi: ath11k: don't use static variables in ath11k_debugfs_fw_stats_process() Baochen Qiang @ 2024-11-13 11:15 ` Kalle Valo 0 siblings, 0 replies; 15+ messages in thread From: Kalle Valo @ 2024-11-13 11:15 UTC (permalink / raw) To: Baochen Qiang; +Cc: ath11k, linux-wireless Baochen Qiang <quic_bqiang@quicinc.com> writes: > Currently ath11k_debugfs_fw_stats_process() is using static variables to count > firmware stat events. Taking num_vdev as an example, if for whatever reason ( > say ar->num_started_vdevs is 0 or firmware bug etc.) the following condition > > (++num_vdev) == total_vdevs_started > > is not met, is_end is not set thus num_vdev won't be cleared. Next time when > firmware stats is requested again, even if everything is working fine, we will > fail due to the condition above will never be satisfied. > > The same applies to num_bcn as well. > > Change to use non-static counters so that we have a chance to clear them each > time firmware stats is requested. Currently only ath11k_fw_stats_request() and > ath11k_debugfs_fw_stats_request() are requesting firmware stats, so clear > counters there. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 > > Fixes: da3a9d3c1576 ("ath11k: refactor debugfs code into debugfs.c") > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> Yikes, good catch! Acked-by: Kalle Valo <kvalo@kernel.org> -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang 2024-11-13 1:56 ` [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() Baochen Qiang 2024-11-13 1:56 ` [PATCH 2/6] wifi: ath11k: don't use static variables in ath11k_debugfs_fw_stats_process() Baochen Qiang @ 2024-11-13 1:56 ` Baochen Qiang 2024-11-13 11:14 ` Kalle Valo 2024-11-13 1:56 ` [PATCH 4/6] wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event() Baochen Qiang ` (2 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, quic_bqiang Commit b488c766442f ("ath11k: report rssi of each chain to mac80211 for QCA6390/WCN6855") and commit c3b39553fc77 ("ath11k: add signal report to mac80211 for QCA6390 and WCN6855") call debugfs functions in mac ops. Those functions are no-ops if CONFIG_ATH11K_DEBUGFS is not enabled, thus cause wrong status reported. Move them to mac.c. Besides, since WMI_REQUEST_RSSI_PER_CHAIN_STAT and WMI_REQUEST_VDEV_STAT stats could also be requested via mac ops, process them directly in ath11k_update_stats_event(). Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 Fixes: b488c766442f ("ath11k: report rssi of each chain to mac80211 for QCA6390/WCN6855") Fixes: c3b39553fc77 ("ath11k: add signal report to mac80211 for QCA6390 and WCN6855") Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- drivers/net/wireless/ath/ath11k/debugfs.c | 136 +--------------------- drivers/net/wireless/ath/ath11k/debugfs.h | 10 +- drivers/net/wireless/ath/ath11k/mac.c | 101 +++++++++++++++- drivers/net/wireless/ath/ath11k/mac.h | 4 +- drivers/net/wireless/ath/ath11k/wmi.c | 43 ++++++- 5 files changed, 146 insertions(+), 148 deletions(-) diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index fd0d2a057084..7f016be2881d 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -93,56 +93,14 @@ void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, spin_unlock_bh(&dbr_data->lock); } -static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) -{ - spin_lock_bh(&ar->data_lock); - ar->fw_stats_done = false; - ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); - ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); - ar->fw_stats.num_vdev_recvd = 0; - ar->fw_stats.num_bcn_recvd = 0; - spin_unlock_bh(&ar->data_lock); -} - void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats) { struct ath11k_base *ab = ar->ab; - struct ath11k_pdev *pdev; bool is_end; - size_t total_vdevs_started = 0; - int i; - - /* WMI_REQUEST_PDEV_STAT request has been already processed */ - - if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { - ar->fw_stats_done = true; - return; - } - - if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { - if (list_empty(&stats->vdevs)) { - ath11k_warn(ab, "empty vdev stats"); - return; - } - /* FW sends all the active VDEV stats irrespective of PDEV, - * hence limit until the count of all VDEVs started - */ - for (i = 0; i < ab->num_radios; i++) { - pdev = rcu_dereference(ab->pdevs_active[i]); - if (pdev && pdev->ar) - total_vdevs_started += ar->num_started_vdevs; - } - - is_end = ((++ar->fw_stats.num_vdev_recvd) == total_vdevs_started); - - list_splice_tail_init(&stats->vdevs, - &ar->fw_stats.vdevs); - if (is_end) - ar->fw_stats_done = true; - - return; - } + /* WMI_REQUEST_PDEV_STAT, WMI_REQUEST_RSSI_PER_CHAIN_STAT and + * WMI_REQUEST_VDEV_STAT requests have been already processed. + */ if (stats->stats_id == WMI_REQUEST_BCN_STAT) { if (list_empty(&stats->bcn)) { @@ -162,88 +120,6 @@ void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats * } } -static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, - struct stats_request_params *req_param) -{ - struct ath11k_base *ab = ar->ab; - unsigned long timeout, time_left; - int ret; - - lockdep_assert_held(&ar->conf_mutex); - - /* FW stats can get split when exceeding the stats data buffer limit. - * In that case, since there is no end marking for the back-to-back - * received 'update stats' event, we keep a 3 seconds timeout in case, - * fw_stats_done is not marked yet - */ - timeout = jiffies + msecs_to_jiffies(3 * 1000); - - ath11k_debugfs_fw_stats_reset(ar); - - reinit_completion(&ar->fw_stats_complete); - - ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); - - if (ret) { - ath11k_warn(ab, "could not request fw stats (%d)\n", - ret); - return ret; - } - - time_left = wait_for_completion_timeout(&ar->fw_stats_complete, 1 * HZ); - - if (!time_left) - return -ETIMEDOUT; - - for (;;) { - if (time_after(jiffies, timeout)) - break; - - spin_lock_bh(&ar->data_lock); - if (ar->fw_stats_done) { - spin_unlock_bh(&ar->data_lock); - break; - } - spin_unlock_bh(&ar->data_lock); - - /* 100ms is empirical, change if required */ - msleep(100); - } - return 0; -} - -int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, - u32 vdev_id, u32 stats_id) -{ - struct ath11k_base *ab = ar->ab; - struct stats_request_params req_param; - int ret; - - mutex_lock(&ar->conf_mutex); - - if (ar->state != ATH11K_STATE_ON) { - ret = -ENETDOWN; - goto err_unlock; - } - - req_param.pdev_id = pdev_id; - req_param.vdev_id = vdev_id; - req_param.stats_id = stats_id; - - ret = ath11k_debugfs_fw_stats_request(ar, &req_param); - if (ret) - ath11k_warn(ab, "failed to request fw stats: %d\n", ret); - - ath11k_dbg(ab, ATH11K_DBG_WMI, - "debug get fw stat pdev id %d vdev id %d stats id 0x%x\n", - pdev_id, vdev_id, stats_id); - -err_unlock: - mutex_unlock(&ar->conf_mutex); - - return ret; -} - static int ath11k_open_pdev_stats(struct inode *inode, struct file *file) { struct ath11k *ar = inode->i_private; @@ -269,7 +145,7 @@ static int ath11k_open_pdev_stats(struct inode *inode, struct file *file) req_param.vdev_id = 0; req_param.stats_id = WMI_REQUEST_PDEV_STAT; - ret = ath11k_debugfs_fw_stats_request(ar, &req_param); + ret = ath11k_mac_fw_stats_request(ar, &req_param); if (ret) { ath11k_warn(ab, "failed to request fw pdev stats: %d\n", ret); goto err_free; @@ -340,7 +216,7 @@ static int ath11k_open_vdev_stats(struct inode *inode, struct file *file) req_param.vdev_id = 0; req_param.stats_id = WMI_REQUEST_VDEV_STAT; - ret = ath11k_debugfs_fw_stats_request(ar, &req_param); + ret = ath11k_mac_fw_stats_request(ar, &req_param); if (ret) { ath11k_warn(ar->ab, "failed to request fw vdev stats: %d\n", ret); goto err_free; @@ -416,7 +292,7 @@ static int ath11k_open_bcn_stats(struct inode *inode, struct file *file) continue; req_param.vdev_id = arvif->vdev_id; - ret = ath11k_debugfs_fw_stats_request(ar, &req_param); + ret = ath11k_mac_fw_stats_request(ar, &req_param); if (ret) { ath11k_warn(ar->ab, "failed to request fw bcn stats: %d\n", ret); goto err_free; diff --git a/drivers/net/wireless/ath/ath11k/debugfs.h b/drivers/net/wireless/ath/ath11k/debugfs.h index a39e458637b0..54dc949e4299 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.h +++ b/drivers/net/wireless/ath/ath11k/debugfs.h @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: BSD-3-Clause-Clear */ /* * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved. - * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved. + * Copyright (c) 2021-2022, 2024 Qualcomm Innovation Center, Inc. All rights reserved. */ #ifndef _ATH11K_DEBUGFS_H_ @@ -273,8 +273,6 @@ void ath11k_debugfs_unregister(struct ath11k *ar); void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats); void ath11k_debugfs_fw_stats_init(struct ath11k *ar); -int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, - u32 vdev_id, u32 stats_id); static inline bool ath11k_debugfs_is_pktlog_lite_mode_enabled(struct ath11k *ar) { @@ -381,12 +379,6 @@ static inline int ath11k_debugfs_rx_filter(struct ath11k *ar) return 0; } -static inline int ath11k_debugfs_get_fw_stats(struct ath11k *ar, - u32 pdev_id, u32 vdev_id, u32 stats_id) -{ - return 0; -} - static inline void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, enum wmi_direct_buffer_module id, diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index f147ef960c22..318fd0bb6fe5 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -8939,6 +8939,99 @@ static void ath11k_mac_put_chain_rssi(struct station_info *sinfo, } } +static void ath11k_mac_fw_stats_reset(struct ath11k *ar) +{ + spin_lock_bh(&ar->data_lock); + ar->fw_stats_done = false; + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); + ar->fw_stats.num_vdev_recvd = 0; + ar->fw_stats.num_bcn_recvd = 0; + spin_unlock_bh(&ar->data_lock); +} + +int ath11k_mac_fw_stats_request(struct ath11k *ar, + struct stats_request_params *req_param) +{ + struct ath11k_base *ab = ar->ab; + unsigned long timeout, time_left; + int ret; + + lockdep_assert_held(&ar->conf_mutex); + + /* FW stats can get split when exceeding the stats data buffer limit. + * In that case, since there is no end marking for the back-to-back + * received 'update stats' event, we keep a 3 seconds timeout in case, + * fw_stats_done is not marked yet + */ + timeout = jiffies + msecs_to_jiffies(3 * 1000); + + ath11k_mac_fw_stats_reset(ar); + + reinit_completion(&ar->fw_stats_complete); + + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); + + if (ret) { + ath11k_warn(ab, "could not request fw stats (%d)\n", + ret); + return ret; + } + + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, 1 * HZ); + + if (!time_left) + return -ETIMEDOUT; + + for (;;) { + if (time_after(jiffies, timeout)) + break; + + spin_lock_bh(&ar->data_lock); + if (ar->fw_stats_done) { + spin_unlock_bh(&ar->data_lock); + break; + } + spin_unlock_bh(&ar->data_lock); + + /* 100ms is empirical, change if required */ + msleep(100); + } + return 0; +} + +static int ath11k_mac_get_fw_stats(struct ath11k *ar, u32 pdev_id, + u32 vdev_id, u32 stats_id) +{ + struct ath11k_base *ab = ar->ab; + struct stats_request_params req_param; + int ret; + + mutex_lock(&ar->conf_mutex); + + if (ar->state != ATH11K_STATE_ON) { + ret = -ENETDOWN; + goto err_unlock; + } + + req_param.pdev_id = pdev_id; + req_param.vdev_id = vdev_id; + req_param.stats_id = stats_id; + + ret = ath11k_mac_fw_stats_request(ar, &req_param); + if (ret) + ath11k_warn(ab, "failed to request fw stats: %d\n", ret); + + ath11k_dbg(ab, ATH11K_DBG_WMI, + "debug get fw stat pdev id %d vdev id %d stats id 0x%x\n", + pdev_id, vdev_id, stats_id); + +err_unlock: + mutex_unlock(&ar->conf_mutex); + + return ret; +} + static void ath11k_mac_op_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_sta *sta, @@ -8976,8 +9069,8 @@ static void ath11k_mac_op_sta_statistics(struct ieee80211_hw *hw, if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)) && arsta->arvif->vdev_type == WMI_VDEV_TYPE_STA && ar->ab->hw_params.supports_rssi_stats && - !ath11k_debugfs_get_fw_stats(ar, ar->pdev->pdev_id, 0, - WMI_REQUEST_RSSI_PER_CHAIN_STAT)) { + !ath11k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0, + WMI_REQUEST_RSSI_PER_CHAIN_STAT)) { ath11k_mac_put_chain_rssi(sinfo, arsta, "fw stats", true); } @@ -8985,8 +9078,8 @@ static void ath11k_mac_op_sta_statistics(struct ieee80211_hw *hw, if (!signal && arsta->arvif->vdev_type == WMI_VDEV_TYPE_STA && ar->ab->hw_params.supports_rssi_stats && - !(ath11k_debugfs_get_fw_stats(ar, ar->pdev->pdev_id, 0, - WMI_REQUEST_VDEV_STAT))) + !(ath11k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0, + WMI_REQUEST_VDEV_STAT))) signal = arsta->rssi_beacon; ath11k_dbg(ar->ab, ATH11K_DBG_MAC, diff --git a/drivers/net/wireless/ath/ath11k/mac.h b/drivers/net/wireless/ath/ath11k/mac.h index f5800fbecff8..e86129763954 100644 --- a/drivers/net/wireless/ath/ath11k/mac.h +++ b/drivers/net/wireless/ath/ath11k/mac.h @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: BSD-3-Clause-Clear */ /* * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved. - * Copyright (c) 2021-2023 Qualcomm Innovation Center, Inc. All rights reserved. + * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved. */ #ifndef ATH11K_MAC_H @@ -179,4 +179,6 @@ int ath11k_mac_vif_set_keepalive(struct ath11k_vif *arvif, void ath11k_mac_fill_reg_tpc_info(struct ath11k *ar, struct ieee80211_vif *vif, struct ieee80211_chanctx_conf *ctx); +int ath11k_mac_fw_stats_request(struct ath11k *ar, + struct stats_request_params *req_param); #endif diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c index 87abfa547529..641cc979589a 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.c +++ b/drivers/net/wireless/ath/ath11k/wmi.c @@ -8157,6 +8157,11 @@ static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *skb) { struct ath11k_fw_stats stats = {}; + size_t total_vdevs_started = 0; + struct ath11k_pdev *pdev; + bool is_end; + int i; + struct ath11k *ar; int ret; @@ -8183,7 +8188,8 @@ static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *sk spin_lock_bh(&ar->data_lock); - /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower mac ops or via + /* WMI_REQUEST_PDEV_STAT, WMI_REQUEST_VDEV_STAT and + * WMI_REQUEST_RSSI_PER_CHAIN_STAT can be requested via mac ops or via * debugfs fw stats. Therefore, processing it separately. */ if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { @@ -8192,9 +8198,38 @@ static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *sk goto complete; } - /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and WMI_REQUEST_RSSI_PER_CHAIN_STAT - * are currently requested only via debugfs fw stats. Hence, processing these - * in debugfs context + if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { + ar->fw_stats_done = true; + goto complete; + } + + if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { + if (list_empty(&stats.vdevs)) { + ath11k_warn(ab, "empty vdev stats"); + goto complete; + } + /* FW sends all the active VDEV stats irrespective of PDEV, + * hence limit until the count of all VDEVs started + */ + for (i = 0; i < ab->num_radios; i++) { + pdev = rcu_dereference(ab->pdevs_active[i]); + if (pdev && pdev->ar) + total_vdevs_started += ar->num_started_vdevs; + } + + is_end = ((++ar->fw_stats.num_vdev_recvd) == total_vdevs_started); + + list_splice_tail_init(&stats.vdevs, + &ar->fw_stats.vdevs); + + if (is_end) + ar->fw_stats_done = true; + + goto complete; + } + + /* WMI_REQUEST_BCN_STAT is currently requested only via debugfs fw stats. + * Hence, processing it in debugfs context */ ath11k_debugfs_fw_stats_process(ar, &stats); -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs 2024-11-13 1:56 ` [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs Baochen Qiang @ 2024-11-13 11:14 ` Kalle Valo 2024-11-14 6:05 ` Baochen Qiang 0 siblings, 1 reply; 15+ messages in thread From: Kalle Valo @ 2024-11-13 11:14 UTC (permalink / raw) To: Baochen Qiang; +Cc: ath11k, linux-wireless Baochen Qiang <quic_bqiang@quicinc.com> writes: > Commit b488c766442f ("ath11k: report rssi of each chain to mac80211 for QCA6390/WCN6855") > and commit c3b39553fc77 ("ath11k: add signal report to mac80211 for QCA6390 and WCN6855") > call debugfs functions in mac ops. Those functions are no-ops if CONFIG_ATH11K_DEBUGFS is > not enabled, thus cause wrong status reported. What do you mean exactly with wrong status reported? mac.c is quite large already, making it even bigger is something I would like to avoid. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs 2024-11-13 11:14 ` Kalle Valo @ 2024-11-14 6:05 ` Baochen Qiang 2024-11-14 11:34 ` Kalle Valo 0 siblings, 1 reply; 15+ messages in thread From: Baochen Qiang @ 2024-11-14 6:05 UTC (permalink / raw) To: Kalle Valo; +Cc: ath11k, linux-wireless On 11/13/2024 7:14 PM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >> Commit b488c766442f ("ath11k: report rssi of each chain to mac80211 for QCA6390/WCN6855") >> and commit c3b39553fc77 ("ath11k: add signal report to mac80211 for QCA6390 and WCN6855") >> call debugfs functions in mac ops. Those functions are no-ops if CONFIG_ATH11K_DEBUGFS is >> not enabled, thus cause wrong status reported. > > What do you mean exactly with wrong status reported? ah, thanks for pointing that. actually no wrong values reported, since we are not even reporting anything in that case. will be accurate. > > mac.c is quite large already, making it even bigger is something I would > like to avoid. then can you suggest any other place? > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs 2024-11-14 6:05 ` Baochen Qiang @ 2024-11-14 11:34 ` Kalle Valo 2024-11-15 1:46 ` Baochen Qiang 0 siblings, 1 reply; 15+ messages in thread From: Kalle Valo @ 2024-11-14 11:34 UTC (permalink / raw) To: Baochen Qiang; +Cc: ath11k, linux-wireless Baochen Qiang <quic_bqiang@quicinc.com> writes: > On 11/13/2024 7:14 PM, Kalle Valo wrote: >> Baochen Qiang <quic_bqiang@quicinc.com> writes: >> >>> Commit b488c766442f ("ath11k: report rssi of each chain to mac80211 for QCA6390/WCN6855") >>> and commit c3b39553fc77 ("ath11k: add signal report to mac80211 for QCA6390 and WCN6855") >>> call debugfs functions in mac ops. Those functions are no-ops if CONFIG_ATH11K_DEBUGFS is >>> not enabled, thus cause wrong status reported. >> >> What do you mean exactly with wrong status reported? > > ah, thanks for pointing that. actually no wrong values reported, since > we are not even reporting anything in that case. will be accurate. > >> >> mac.c is quite large already, making it even bigger is something I would >> like to avoid. > > then can you suggest any other place? I first need to understand the issue we are fixing. Is it ath11k does not report rssi and signal via ath11k_mac_op_sta_statistics() when CONFIG_ATH11K_DEBUGFS is disabled? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs 2024-11-14 11:34 ` Kalle Valo @ 2024-11-15 1:46 ` Baochen Qiang 0 siblings, 0 replies; 15+ messages in thread From: Baochen Qiang @ 2024-11-15 1:46 UTC (permalink / raw) To: Kalle Valo; +Cc: ath11k, linux-wireless On 11/14/2024 7:34 PM, Kalle Valo wrote: > Baochen Qiang <quic_bqiang@quicinc.com> writes: > >> On 11/13/2024 7:14 PM, Kalle Valo wrote: >>> Baochen Qiang <quic_bqiang@quicinc.com> writes: >>> >>>> Commit b488c766442f ("ath11k: report rssi of each chain to mac80211 for QCA6390/WCN6855") >>>> and commit c3b39553fc77 ("ath11k: add signal report to mac80211 for QCA6390 and WCN6855") >>>> call debugfs functions in mac ops. Those functions are no-ops if CONFIG_ATH11K_DEBUGFS is >>>> not enabled, thus cause wrong status reported. >>> >>> What do you mean exactly with wrong status reported? >> >> ah, thanks for pointing that. actually no wrong values reported, since >> we are not even reporting anything in that case. will be accurate. >> >>> >>> mac.c is quite large already, making it even bigger is something I would >>> like to avoid. >> >> then can you suggest any other place? > > I first need to understand the issue we are fixing. Is it ath11k does > not report rssi and signal via ath11k_mac_op_sta_statistics() when > CONFIG_ATH11K_DEBUGFS is disabled? Yes, Kalle. If CONFIG_ATH11K_DEBUGFS is disabled, ath11k_debugfs_get_fw_stats() is a no-ops, so no WMI command sent to firmware, thus no WMI event and therefore ath11k does not report them. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/6] wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event() 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang ` (2 preceding siblings ...) 2024-11-13 1:56 ` [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs Baochen Qiang @ 2024-11-13 1:56 ` Baochen Qiang 2024-11-13 11:15 ` Kalle Valo 2024-11-13 1:56 ` [PATCH 5/6] wifi: ath11k: move locking outside of ath11k_mac_get_fw_stats() Baochen Qiang 2024-11-13 1:56 ` [PATCH 6/6] wifi: ath11k: consistently use ath11k_mac_get_fw_stats() Baochen Qiang 5 siblings, 1 reply; 15+ messages in thread From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, quic_bqiang Currently RCU lock and ar->data_lock are acquired in a sequence of rcu_read_lock() spin_lock_bh(&ar->data_lock) but released in a sequence of rcu_read_unlock() spin_unlock_bh(&ar->data_lock) Although there are no apparent issues with this, reorder them to achieve symmetry. Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- drivers/net/wireless/ath/ath11k/wmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c index 641cc979589a..0f7df1fbaed8 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.c +++ b/drivers/net/wireless/ath/ath11k/wmi.c @@ -8235,8 +8235,8 @@ static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *sk complete: complete(&ar->fw_stats_complete); - rcu_read_unlock(); spin_unlock_bh(&ar->data_lock); + rcu_read_unlock(); /* Since the stats's pdev, vdev and beacon list are spliced and reinitialised * at this point, no need to free the individual list. -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/6] wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event() 2024-11-13 1:56 ` [PATCH 4/6] wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event() Baochen Qiang @ 2024-11-13 11:15 ` Kalle Valo 0 siblings, 0 replies; 15+ messages in thread From: Kalle Valo @ 2024-11-13 11:15 UTC (permalink / raw) To: Baochen Qiang; +Cc: ath11k, linux-wireless Baochen Qiang <quic_bqiang@quicinc.com> writes: > Currently RCU lock and ar->data_lock are acquired in a sequence of > > rcu_read_lock() > spin_lock_bh(&ar->data_lock) > > but released in a sequence of > > rcu_read_unlock() > spin_unlock_bh(&ar->data_lock) > > Although there are no apparent issues with this, reorder them to > achieve symmetry. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 > > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> Acked-by: Kalle Valo <kvalo@kernel.org> -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/6] wifi: ath11k: move locking outside of ath11k_mac_get_fw_stats() 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang ` (3 preceding siblings ...) 2024-11-13 1:56 ` [PATCH 4/6] wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event() Baochen Qiang @ 2024-11-13 1:56 ` Baochen Qiang 2024-11-13 1:56 ` [PATCH 6/6] wifi: ath11k: consistently use ath11k_mac_get_fw_stats() Baochen Qiang 5 siblings, 0 replies; 15+ messages in thread From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, quic_bqiang Currently ath11k_mac_get_fw_stats() is acquiring/releasing ar->conf_mutex by itself. In order to reuse this function in a context where that lock is already taken, move lock handling to its callers, then the function itself only has to assert it. There is only one caller now, i.e., ath11k_mac_op_sta_statistics(), so add lock handling there. Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- drivers/net/wireless/ath/ath11k/mac.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index 318fd0bb6fe5..0ff430931bfd 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -9007,12 +9007,10 @@ static int ath11k_mac_get_fw_stats(struct ath11k *ar, u32 pdev_id, struct stats_request_params req_param; int ret; - mutex_lock(&ar->conf_mutex); + lockdep_assert_held(&ar->conf_mutex); - if (ar->state != ATH11K_STATE_ON) { - ret = -ENETDOWN; - goto err_unlock; - } + if (ar->state != ATH11K_STATE_ON) + return -ENETDOWN; req_param.pdev_id = pdev_id; req_param.vdev_id = vdev_id; @@ -9026,9 +9024,6 @@ static int ath11k_mac_get_fw_stats(struct ath11k *ar, u32 pdev_id, "debug get fw stat pdev id %d vdev id %d stats id 0x%x\n", pdev_id, vdev_id, stats_id); -err_unlock: - mutex_unlock(&ar->conf_mutex); - return ret; } @@ -9066,6 +9061,7 @@ static void ath11k_mac_op_sta_statistics(struct ieee80211_hw *hw, ath11k_mac_put_chain_rssi(sinfo, arsta, "ppdu", false); + mutex_lock(&ar->conf_mutex); if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)) && arsta->arvif->vdev_type == WMI_VDEV_TYPE_STA && ar->ab->hw_params.supports_rssi_stats && @@ -9081,6 +9077,7 @@ static void ath11k_mac_op_sta_statistics(struct ieee80211_hw *hw, !(ath11k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0, WMI_REQUEST_VDEV_STAT))) signal = arsta->rssi_beacon; + mutex_unlock(&ar->conf_mutex); ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "sta statistics db2dbm %u rssi comb %d rssi beacon %d\n", -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 6/6] wifi: ath11k: consistently use ath11k_mac_get_fw_stats() 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang ` (4 preceding siblings ...) 2024-11-13 1:56 ` [PATCH 5/6] wifi: ath11k: move locking outside of ath11k_mac_get_fw_stats() Baochen Qiang @ 2024-11-13 1:56 ` Baochen Qiang 5 siblings, 0 replies; 15+ messages in thread From: Baochen Qiang @ 2024-11-13 1:56 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, quic_bqiang Currently to get firmware stats, ath11k_mac_op_get_txpower() calls ath11k_fw_stats_request() and ath11k_mac_op_sta_statistics() calls ath11k_mac_get_fw_stats(). Those two helpers are basically doing the same, except for: 1. ath11k_mac_get_fw_stats() verifies ar->state inside itself. 2. ath11k_mac_get_fw_stats() calls ath11k_mac_fw_stats_request() which then calls ath11k_mac_fw_stats_reset() to free pdev/vdev stats whereas only pdev stats are freed by ath11k_fw_stats_request(). 3. ath11k_mac_get_fw_stats() waits for ar->fw_stats_complete and ar->fw_stats_done, whereas ath11k_fw_stats_request() only waits for ar->fw_stats_complete. Change to call ath11k_mac_get_fw_stats() in ath11k_mac_op_get_txpower(). This is valid because: 1. ath11k_mac_op_get_txpower() also has the same request on ar->state. 2. it is harmless to call ath11k_fw_stats_vdevs_free() since ar->fw_stats.vdevs should be empty and there should be no one expecting it at that time. 3. ath11k_mac_op_get_txpower() only needs pdev stats. For pdev stats, ar->fw_stats_done is set to true whenever ar->fw_stats_complete is set to true in ath11k_update_stats_event(). So additional wait on ar->fw_stats_done does not wast any time. Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.37 Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- drivers/net/wireless/ath/ath11k/mac.c | 44 ++------------------------- 1 file changed, 2 insertions(+), 42 deletions(-) diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index 0ff430931bfd..ab94261f7b0e 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -9412,47 +9412,12 @@ static int ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, return ret; } -static int ath11k_fw_stats_request(struct ath11k *ar, - struct stats_request_params *req_param) -{ - struct ath11k_base *ab = ar->ab; - unsigned long time_left; - int ret; - - lockdep_assert_held(&ar->conf_mutex); - - spin_lock_bh(&ar->data_lock); - ar->fw_stats_done = false; - ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); - ar->fw_stats.num_vdev_recvd = 0; - ar->fw_stats.num_bcn_recvd = 0; - spin_unlock_bh(&ar->data_lock); - - reinit_completion(&ar->fw_stats_complete); - - ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); - if (ret) { - ath11k_warn(ab, "could not request fw stats (%d)\n", - ret); - return ret; - } - - time_left = wait_for_completion_timeout(&ar->fw_stats_complete, - 1 * HZ); - - if (!time_left) - return -ETIMEDOUT; - - return 0; -} - static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, struct ieee80211_vif *vif, int *dbm) { struct ath11k *ar = hw->priv; struct ath11k_base *ab = ar->ab; - struct stats_request_params req_param = {0}; struct ath11k_fw_stats_pdev *pdev; int ret; @@ -9464,9 +9429,6 @@ static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, */ mutex_lock(&ar->conf_mutex); - if (ar->state != ATH11K_STATE_ON) - goto err_fallback; - /* Firmware doesn't provide Tx power during CAC hence no need to fetch * the stats. */ @@ -9475,10 +9437,8 @@ static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, return -EAGAIN; } - req_param.pdev_id = ar->pdev->pdev_id; - req_param.stats_id = WMI_REQUEST_PDEV_STAT; - - ret = ath11k_fw_stats_request(ar, &req_param); + ret = ath11k_mac_get_fw_stats(ar, ar->pdev->pdev_id, 0, + WMI_REQUEST_PDEV_STAT); if (ret) { ath11k_warn(ab, "failed to request fw pdev stats: %d\n", ret); goto err_fallback; -- 2.25.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-11-15 1:46 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-13 1:56 [PATCH 0/6] wifi: ath11k: fixes and refactor to firmware stats related functions Baochen Qiang 2024-11-13 1:56 ` [PATCH 1/6] wifi: ath11k: avoid burning CPU in ath11k_debugfs_fw_stats_request() Baochen Qiang 2024-11-13 11:12 ` Kalle Valo 2024-11-14 5:34 ` Baochen Qiang 2024-11-13 1:56 ` [PATCH 2/6] wifi: ath11k: don't use static variables in ath11k_debugfs_fw_stats_process() Baochen Qiang 2024-11-13 11:15 ` Kalle Valo 2024-11-13 1:56 ` [PATCH 3/6] wifi: ath11k: move some firmware stats related functions outside of debugfs Baochen Qiang 2024-11-13 11:14 ` Kalle Valo 2024-11-14 6:05 ` Baochen Qiang 2024-11-14 11:34 ` Kalle Valo 2024-11-15 1:46 ` Baochen Qiang 2024-11-13 1:56 ` [PATCH 4/6] wifi: ath11k: adjust unlock sequence in ath11k_update_stats_event() Baochen Qiang 2024-11-13 11:15 ` Kalle Valo 2024-11-13 1:56 ` [PATCH 5/6] wifi: ath11k: move locking outside of ath11k_mac_get_fw_stats() Baochen Qiang 2024-11-13 1:56 ` [PATCH 6/6] wifi: ath11k: consistently use ath11k_mac_get_fw_stats() Baochen Qiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).