On 6/11/2025 1:41 AM, Bjorn Andersson wrote:
> On Tue, Jun 10, 2025 at 01:16:30PM +0530, Rameshkumar Sundaram wrote:
>>
>>
>> On 6/10/2025 8:36 AM, Bjorn Andersson via B4 Relay wrote:
>>> From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxxx>
>>> ---
>>> 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.
>>
>
> I agree with this. In fact it seems to me that the majority of [1]
> should be considered for ath12k as well (and Jeff acknowledged the
> same).
Yeah, there are some other issues in addition to this DEBUGFS one, I jsut posted the fix
for review.
>
> The purpose of this patch was solely to deal with the regression from
> the previous behavior introduced in v6.16-rc1, causing my X Elite laptop
> to idle about 10C warmer. (Afaict neither distros or upstream arm64
> defconfig has ATH12K_DEBUG enabled)
>
> The "also fix X, Y, Z" would at least be separate patches, and could be
> applied either to -rc or v6.17 on top of something like this.
>
> [1] https://lore.kernel.org/ath11k/20250220082448.31039-1-quic_bqiang@xxxxxxxxxxx/
>
> Regards,
> Bjorn
>
>> --
>> --
>> Ramesh
>>
>>
>
