| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c2d138ba-5b08-4daa-95b3-e1f95f05938d@oss.qualcomm.com>
Date: Tue, 10 Jun 2025 13:16:30 +0530
From: Rameshkumar Sundaram <rameshkumar.sundaram@....qualcomm.com>
To: bjorn.andersson@....qualcomm.com, Jeff Johnson <jjohnson@...nel.org>,
Aditya Kumar Singh <aditya.kumar.singh@....qualcomm.com>,
Mahendran P <quic_mahep@...cinc.com>
Cc: linux-arm-msm@...r.kernel.org,
Jeff Johnson <jeff.johnson@....qualcomm.com>,
linux-wireless@...r.kernel.org, ath12k@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] wifi: ath12k: Avoid CPU busy-wait by handling VDEV_STAT
and BCN_STAT
On 6/10/2025 8:36 AM, Bjorn Andersson via B4 Relay wrote:
> From: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
>
> When the ath12k driver is built without CONFIG_ATH12K_DEBUG, the
> recently refactored stats code can cause any user space application
> (such at NetworkManager) to consume 100% CPU for 3 seconds, every time
> stats are read.
>
> Commit 'b8a0d83fe4c7 ("wifi: ath12k: move firmware stats out of
> debugfs")' moved ath12k_debugfs_fw_stats_request() out of debugfs, by
> merging the additional logic into ath12k_mac_get_fw_stats().
>
> Among the added responsibility of ath12k_mac_get_fw_stats() was the
> busy-wait for `fw_stats_done`.
>
> Signalling of `fw_stats_done` happens when one of the
> WMI_REQUEST_PDEV_STAT, WMI_REQUEST_VDEV_STAT, and WMI_REQUEST_BCN_STAT
> messages are received, but the handling of the latter two commands remained
> in the debugfs code. As `fw_stats_done` isn't signalled, the calling
> processes will spin until the timeout (3 seconds) is reached.
>
> Moving the handling of these two additional responses out of debugfs
> resolves the issue.
>
> Fixes: b8a0d83fe4c7 ("wifi: ath12k: move firmware stats out of debugfs")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
> ---
> drivers/net/wireless/ath/ath12k/debugfs.c | 58 --------------------------
> drivers/net/wireless/ath/ath12k/debugfs.h | 7 ----
> drivers/net/wireless/ath/ath12k/wmi.c | 67 +++++++++++++++++++++++++++----
> 3 files changed, 60 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/debugfs.c b/drivers/net/wireless/ath/ath12k/debugfs.c
> index dd624d73b8b2714e77c9d89b5a52f7b3fcb02951..23da93afaa5c25e806c9859dbbdd796afd23d78a 100644
> --- a/drivers/net/wireless/ath/ath12k/debugfs.c
> +++ b/drivers/net/wireless/ath/ath12k/debugfs.c
> @@ -1251,64 +1251,6 @@ void ath12k_debugfs_soc_destroy(struct ath12k_base *ab)
> */
> }
>
> -void
> -ath12k_debugfs_fw_stats_process(struct ath12k *ar,
> - struct ath12k_fw_stats *stats)
> -{
> - struct ath12k_base *ab = ar->ab;
> - struct ath12k_pdev *pdev;
> - bool is_end;
> - static unsigned int num_vdev, num_bcn;
> - size_t total_vdevs_started = 0;
> - int i;
> -
> - if (stats->stats_id == WMI_REQUEST_VDEV_STAT) {
> - if (list_empty(&stats->vdevs)) {
> - ath12k_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
> - */
> - rcu_read_lock();
> - for (i = 0; i < ab->num_radios; i++) {
> - pdev = rcu_dereference(ab->pdevs_active[i]);
> - if (pdev && pdev->ar)
> - total_vdevs_started += pdev->ar->num_started_vdevs;
> - }
> - rcu_read_unlock();
> -
> - is_end = ((++num_vdev) == total_vdevs_started);
> -
> - list_splice_tail_init(&stats->vdevs,
> - &ar->fw_stats.vdevs);
> -
> - if (is_end) {
> - ar->fw_stats.fw_stats_done = true;
> - num_vdev = 0;
> - }
> - return;
> - }
> - if (stats->stats_id == WMI_REQUEST_BCN_STAT) {
> - if (list_empty(&stats->bcn)) {
> - ath12k_warn(ab, "empty beacon stats");
> - return;
> - }
> - /* Mark end until we reached the count of all started VDEVs
> - * within the PDEV
> - */
> - is_end = ((++num_bcn) == ar->num_started_vdevs);
> -
> - list_splice_tail_init(&stats->bcn,
> - &ar->fw_stats.bcn);
> -
> - if (is_end) {
> - ar->fw_stats.fw_stats_done = true;
> - num_bcn = 0;
> - }
> - }
> -}
> -
> static int ath12k_open_vdev_stats(struct inode *inode, struct file *file)
> {
> struct ath12k *ar = inode->i_private;
> diff --git a/drivers/net/wireless/ath/ath12k/debugfs.h b/drivers/net/wireless/ath/ath12k/debugfs.h
> index ebef7dace3448e4bdf2d6cb155d089267315172c..21641a8a03460c6cc1b34929a353e5605bb834ce 100644
> --- a/drivers/net/wireless/ath/ath12k/debugfs.h
> +++ b/drivers/net/wireless/ath/ath12k/debugfs.h
> @@ -12,8 +12,6 @@ void ath12k_debugfs_soc_create(struct ath12k_base *ab);
> void ath12k_debugfs_soc_destroy(struct ath12k_base *ab);
> void ath12k_debugfs_register(struct ath12k *ar);
> void ath12k_debugfs_unregister(struct ath12k *ar);
> -void ath12k_debugfs_fw_stats_process(struct ath12k *ar,
> - struct ath12k_fw_stats *stats);
> void ath12k_debugfs_op_vif_add(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif);
> void ath12k_debugfs_pdev_create(struct ath12k_base *ab);
> @@ -126,11 +124,6 @@ static inline void ath12k_debugfs_unregister(struct ath12k *ar)
> {
> }
>
> -static inline void ath12k_debugfs_fw_stats_process(struct ath12k *ar,
> - struct ath12k_fw_stats *stats)
> -{
> -}
> -
> static inline bool ath12k_debugfs_is_extd_rx_stats_enabled(struct ath12k *ar)
> {
> return false;
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index 60e2444fe08cefa39ae218d07eb9736d2a0c982b..2d2444417e2b2d9281754d113f2b073034e27739 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -7626,6 +7626,63 @@ static int ath12k_wmi_pull_fw_stats(struct ath12k_base *ab, struct sk_buff *skb,
> &parse);
> }
>
> +static void ath12k_wmi_fw_stats_process(struct ath12k *ar,
> + struct ath12k_fw_stats *stats)
> +{
> + struct ath12k_base *ab = ar->ab;
> + struct ath12k_pdev *pdev;
> + bool is_end;
> + static unsigned int num_vdev, num_bcn;
> + size_t total_vdevs_started = 0;
> + int i;
> +
> + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) {
> + if (list_empty(&stats->vdevs)) {
> + ath12k_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
> + */
> + rcu_read_lock();
> + for (i = 0; i < ab->num_radios; i++) {
> + pdev = rcu_dereference(ab->pdevs_active[i]);
> + if (pdev && pdev->ar)
> + total_vdevs_started += pdev->ar->num_started_vdevs;
> + }
> + rcu_read_unlock();
> +
> + is_end = ((++num_vdev) == total_vdevs_started);
> +
> + list_splice_tail_init(&stats->vdevs,
> + &ar->fw_stats.vdevs);
> +
> + if (is_end) {
> + ar->fw_stats.fw_stats_done = true;
> + num_vdev = 0;
> + }
> + return;
> + }
> + if (stats->stats_id == WMI_REQUEST_BCN_STAT) {
> + if (list_empty(&stats->bcn)) {
> + ath12k_warn(ab, "empty beacon stats");
> + return;
> + }
> + /* Mark end until we reached the count of all started VDEVs
> + * within the PDEV
> + */
> + is_end = ((++num_bcn) == ar->num_started_vdevs);
> +
> + list_splice_tail_init(&stats->bcn,
> + &ar->fw_stats.bcn);
> +
> + if (is_end) {
> + ar->fw_stats.fw_stats_done = true;
> + num_bcn = 0;
> + }
> + }
> +}
> +
> static void ath12k_update_stats_event(struct ath12k_base *ab, struct sk_buff *skb)
> {
> struct ath12k_fw_stats stats = {};
> @@ -7655,19 +7712,15 @@ static void ath12k_update_stats_event(struct ath12k_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
> - * debugfs fw stats. Therefore, processing it separately.
> - */
> + /* Handle WMI_REQUEST_PDEV_STAT status update */
> if (stats.stats_id == WMI_REQUEST_PDEV_STAT) {
> list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs);
> ar->fw_stats.fw_stats_done = true;
> goto complete;
> }
>
> - /* WMI_REQUEST_VDEV_STAT and WMI_REQUEST_BCN_STAT are currently requested only
> - * via debugfs fw stats. Hence, processing these in debugfs context.
> - */
> - ath12k_debugfs_fw_stats_process(ar, &stats);
> + /* Handle WMI_REQUEST_VDEV_STAT and WMI_REQUEST_BCN_STAT updates. */
> + ath12k_wmi_fw_stats_process(ar, &stats);
>
> complete:
> complete(&ar->fw_stats_complete);
>
This look fine to me, Thanks for fixing this.
Apart from this we may also have to free up the stats buffer list
maintained when the stats is requested out of debugfs (like
ath12k_mac_op_get_txpower() and ath12k_mac_op_sta_statistics()) once its
scope of usage is done, else the memory will be held untill next fw
stats request or module unload.
--
--
Ramesh
Powered by blists - more mailing lists