Re: [PATCH ath-next v6] wifi: ath12k: avoid dynamic alloc when parsing wmi tb

[Rameshkumar Sundaram <rameshkumar.sundaram@oss.qualcomm.com>]

On 4/3/2026 7:58 PM, Nicolas Escande wrote:
> On each WMI message received from the hardware, we alloc a temporary array
> of WMI_TAG_MAX entries of type void *. This array is then populated with
> pointers of parsed structs depending on the WMI type, and then freed. This
> alloc can fail when memory pressure in the system is high enough.
> 
> Given the fact that it is scheduled in softirq with the system_bh_wq, we
> should not be able to parse more than one WMI message per CPU at any time.
> 
> So instead lets move to a per cpu allocated array, that is reused across
> calls. This memory is allocated as needed and refcounted to exist only
> as long as one struct ath12k_base lives.
> 
> ath12k_wmi_tlv_parse_alloc() and ath12k_wmi_tlv_parse() are merged
> together as it no longer allocs mem but returns the existing per-cpu one.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00218-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
> ---
> changes from v4:
>    - moved to a single instance, refcounted per cpu memory alloc
>    
> changes from v3:
>    - simplified ath12k_core_init() with a single statement
>    - move perpcu.h include directly to wmi.c
> 
> changes from v2:
>    - removed now superfluous return in ath12k_wmi_event_teardown_complete()
>    - moved ath12k_wmi_tb declaration to wmi.c & added two functions to
>      alloc / free it
>    - removed useless error message on memory allocation failure
> 
> changes from v1:
>    - rebased on ath-next 27401c9b1432
>    - changed wording according to Jeff's comment
>    - moved alloc/cleanup to new module_init/exit functions in the
>      ath12k module as per Baochen's comment
> 
> changes from RFC:
>    - rebased on ath-next 8e0ab5b9adb7
>    - converted missing call sites ath12k_wmi_obss_color_collision_event()
>      & ath12k_wmi_pdev_temperature_event()
>    - changed alloc order & cleanup path in ath12k_core_alloc() as it seems
>      it confused people
>    - used sizeof(*tb) in ath12k_wmi_tlv_parse()
> ---
>   drivers/net/wireless/ath/ath12k/core.c |   6 +
>   drivers/net/wireless/ath/ath12k/wmi.c  | 214 +++++++++----------------
>   drivers/net/wireless/ath/ath12k/wmi.h  |   3 +
>   3 files changed, 84 insertions(+), 139 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index c31c47fb5a73..6f0f4bfbf699 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -2256,6 +2256,7 @@ void ath12k_core_deinit(struct ath12k_base *ab)
>   void ath12k_core_free(struct ath12k_base *ab)
>   {
>   	timer_delete_sync(&ab->rx_replenish_retry);
> +	ath12k_wmi_free();
>   	destroy_workqueue(ab->workqueue_aux);
>   	destroy_workqueue(ab->workqueue);
>   	kfree(ab);
> @@ -2280,6 +2281,9 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size,
>   	if (!ab->workqueue_aux)
>   		goto err_free_wq;
>   
> +	if (ath12k_wmi_alloc() < 0)
> +		goto err_free_wq_aux;
> +
>   	mutex_init(&ab->core_lock);
>   	spin_lock_init(&ab->base_lock);
>   	init_completion(&ab->reset_complete);
> @@ -2314,6 +2318,8 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size,
>   
>   	return ab;
>   
> +err_free_wq_aux:
> +	destroy_workqueue(ab->workqueue_aux);
>   err_free_wq:
>   	destroy_workqueue(ab->workqueue);
>   err_sc_free:
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index 65a05a9520ff..2641a76e9543 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -15,6 +15,8 @@
>   #include <linux/time.h>
>   #include <linux/of.h>
>   #include <linux/cleanup.h>
> +#include <linux/percpu.h>
> +#include <linux/refcount.h>
>   #include "core.h"
>   #include "debugfs.h"
>   #include "debug.h"
> @@ -134,6 +136,10 @@ struct wmi_pdev_set_obss_bitmap_arg {
>   	const char *label;
>   };
>   
> +static DEFINE_MUTEX(ath12k_wmi_mutex);
> +static refcount_t ath12k_wmi_refcount = REFCOUNT_INIT(0);
> +static void __percpu *ath12k_wmi_tb = NULL;

Checkpatch complains:
drivers/net/wireless/ath/ath12k/wmi.c:141: do not initialise statics to NULL

> +
>   static const struct ath12k_wmi_tlv_policy ath12k_wmi_tlv_policies[] = {
>   	[WMI_TAG_ARRAY_BYTE] = { .min_len = 0 },
>   	[WMI_TAG_ARRAY_UINT32] = { .min_len = 0 },
> @@ -289,29 +295,19 @@ static int ath12k_wmi_tlv_iter_parse(struct ath12k_base *ab, u16 tag, u16 len,
>   	return 0;
>   }
>   

{...}

>   #ifdef CONFIG_ATH12K_DEBUGFS
> @@ -11239,3 +11150,28 @@ int ath12k_wmi_send_mlo_link_set_active_cmd(struct ath12k_base *ab,
>   	dev_kfree_skb(skb);
>   	return ret;
>   }
> +
> +int ath12k_wmi_alloc(void)
> +{
> +	guard(mutex)(&ath12k_wmi_mutex);
> +
> +	if (!ath12k_wmi_tb) {
> +		ath12k_wmi_tb = __alloc_percpu(WMI_TAG_MAX * sizeof(void *),
> +					       __alignof__(void *));
> +		if (!ath12k_wmi_tb)
> +			return -ENOMEM;
> +	}
> +
> +	refcount_inc(&ath12k_wmi_refcount);

With ath12k_wmi_refcount initialized to zero, this will trigger addition 
on 0 - use-after-free warning in refcount.

We may have to do refcount_set(&ath12k_wmi_refcount, 1); on tb alloc.


> +	return 0;
> +}
> +
> +void ath12k_wmi_free(void)
> +{
> +	guard(mutex)(&ath12k_wmi_mutex);
> +
> +	if (refcount_dec_and_test(&ath12k_wmi_refcount)) {
> +		free_percpu(ath12k_wmi_tb);
> +		ath12k_wmi_tb = NULL;
> +	}
> +}
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h
> index 5ba9b7d3a888..4a34b2ca99ea 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.h
> +++ b/drivers/net/wireless/ath/ath12k/wmi.h
> @@ -6576,4 +6576,7 @@ int ath12k_wmi_send_vdev_set_tpc_power(struct ath12k *ar,
>   				       struct ath12k_reg_tpc_power_info *param);
>   int ath12k_wmi_send_mlo_link_set_active_cmd(struct ath12k_base *ab,
>   					    struct wmi_mlo_link_set_active_arg *param);
> +int ath12k_wmi_alloc(void);
> +void ath12k_wmi_free(void);
> +
>   #endif


--
Ramesh