Re: [RFC PATCH 1/1] wifi: ath12k: avoid dynamic alloc when parsing wmi tb

Rameshkumar Sundaram
2026-03-03T09:53:37Z

On 2/26/2026 10:25 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, stored in the struct
> ath12k_base, that is reused accros calls. The ath12k_wmi_tlv_parse_alloc()
> is also renamed into / merged with ath12k_wmi_tlv_parse() as it no longer
> allocs memory but returns the existing per-cpu one.
>
> Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
> ---
> drivers/net/wireless/ath/ath12k/core.c | 7 +
> drivers/net/wireless/ath/ath12k/core.h | 2 +
> drivers/net/wireless/ath/ath12k/wmi.c | 170 ++++++-------------------
> 3 files changed, 49 insertions(+), 130 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
> index 9d6c50a94e64..961c9df69aa1 100644
> --- a/drivers/net/wireless/ath/ath12k/core.c
> +++ b/drivers/net/wireless/ath/ath12k/core.c
> @@ -2237,6 +2237,7 @@ void ath12k_core_free(struct ath12k_base *ab)
> timer_delete_sync(&ab->rx_replenish_retry);
> destroy_workqueue(ab->workqueue_aux);
> destroy_workqueue(ab->workqueue);
> + free_percpu(ab->wmi_tb);
> kfree(ab);
> }
>
> @@ -2249,6 +2250,11 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size,
> if (!ab)
> return NULL;
>
> + ab->wmi_tb = __alloc_percpu(WMI_TAG_MAX * sizeof(void *),
> + __alignof__(void *));
> + if (!ab->wmi_tb)
> + goto err_sc_free;
> +

we could move this allocation after workqueue_aux alloc and avoid
free_percpu(ab->wmi_tb); in the label

> init_completion(&ab->driver_recovery);
>
> ab->workqueue = create_singlethread_workqueue("ath12k_wq");
> @@ -2296,6 +2302,7 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size,
> err_free_wq:
> destroy_workqueue(ab->workqueue);
> err_sc_free:
> + free_percpu(ab->wmi_tb);
> kfree(ab);
> return NULL;
> }
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 990934ec92fc..378573a100e8 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -19,6 +19,7 @@
> #include <linux/average.h>
> #include <linux/of.h>
> #include <linux/rhashtable.h>
> +#include <linux/percpu.h>
> #include "qmi.h"
> #include "htc.h"
> #include "wmi.h"
> @@ -934,6 +935,7 @@ struct ath12k_base {
> struct device *dev;
> struct ath12k_qmi qmi;
> struct ath12k_wmi_base wmi_ab;
> + void __percpu *wmi_tb;
> struct completion fw_ready;
> u8 device_id;
> int num_radios;
> diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c
> index 404f031a3c87..9ccc10c07c21 100644
> --- a/drivers/net/wireless/ath/ath12k/wmi.c
> +++ b/drivers/net/wireless/ath/ath12k/wmi.c
> @@ -289,29 +289,19 @@ static int ath12k_wmi_tlv_iter_parse(struct ath12k_base *ab, u16 tag, u16 len,
> return 0;
> }
>
> -static int ath12k_wmi_tlv_parse(struct ath12k_base *ar, const void **tb,
> - const void *ptr, size_t len)
> -{
> - return ath12k_wmi_tlv_iter(ar, ptr, len, ath12k_wmi_tlv_iter_parse,
> - (void *)tb);
> -}
> -
> static const void **
> -ath12k_wmi_tlv_parse_alloc(struct ath12k_base *ab,
> - struct sk_buff *skb, gfp_t gfp)
> +ath12k_wmi_tlv_parse(struct ath12k_base *ab, struct sk_buff *skb)
> {
> const void **tb;
> int ret;
>
> - tb = kcalloc(WMI_TAG_MAX, sizeof(*tb), gfp);
> - if (!tb)
> - return ERR_PTR(-ENOMEM);
> + tb = this_cpu_ptr(ab->wmi_tb);
> + memset(tb, 0, WMI_TAG_MAX * sizeof(void *));

prefer sizeof(*tb) over sizeof(void *)

>
> - ret = ath12k_wmi_tlv_parse(ab, tb, skb->data, skb->len);
> - if (ret) {
> - kfree(tb);
> + ret = ath12k_wmi_tlv_iter(ab, skb->data, skb->len,
> + ath12k_wmi_tlv_iter_parse, (void *)tb);
> + if (ret)
> return ERR_PTR(ret);
> - }
>
> return tb;
> }
> @@ -5714,7 +5704,7 @@ static int ath12k_pull_vdev_start_resp_tlv(struct ath12k_base *ab, struct sk_buf
> const struct wmi_vdev_start_resp_event *ev;
> int ret;
>
> - tb = ath12k_wmi_tlv_parse_alloc(ab, skb, GFP_ATOMIC);
> + tb = ath12k_wmi_tlv_parse(ab, skb);
> if (IS_ERR(tb)) {
> ret = PTR_ERR(tb);
> ath12k_warn(ab, "failed to parse tlv: %d\n", ret);
> @@ -5724,13 +5714,11 @@ static int ath12k_pull_vdev_start_resp_tlv(struct ath12k_base *ab, struct sk_buf
> ev = tb[WMI_TAG_VDEV_START_RESPONSE_EVENT];
> if (!ev) {
> ath12k_warn(ab, "failed to fetch vdev start resp ev");
> - kfree(tb);
> return -EPROTO;
> }
>
> *vdev_rsp = *ev;
>
> - kfree(tb);
> return 0;
> }
>
> @@ -5809,7 +5797,7 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
>
> ath12k_dbg(ab, ATH12K_DBG_WMI, "processing regulatory ext channel list\n");
>
> - tb = ath12k_wmi_tlv_parse_alloc(ab, skb, GFP_ATOMIC);
> + tb = ath12k_wmi_tlv_parse(ab, skb);
> if (IS_ERR(tb)) {
> ret = PTR_ERR(tb);
> ath12k_warn(ab, "failed to parse tlv: %d\n", ret);
> @@ -5819,7 +5807,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> ev = tb[WMI_TAG_REG_CHAN_LIST_CC_EXT_EVENT];
> if (!ev) {
> ath12k_warn(ab, "failed to fetch reg chan list ext update ev\n");
> - kfree(tb);
> return -EPROTO;
> }
>
> @@ -5849,7 +5836,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> if (num_2g_reg_rules > MAX_REG_RULES || num_5g_reg_rules > MAX_REG_RULES) {
> ath12k_warn(ab, "Num reg rules for 2G/5G exceeds max limit (num_2g_reg_rules: %d num_5g_reg_rules: %d max_rules: %d)\n",
> num_2g_reg_rules, num_5g_reg_rules, MAX_REG_RULES);
> - kfree(tb);
> return -EINVAL;
> }
>
> @@ -5859,7 +5845,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> if (num_6g_reg_rules_ap[i] > MAX_6GHZ_REG_RULES) {
> ath12k_warn(ab, "Num 6G reg rules for AP mode(%d) exceeds max limit (num_6g_reg_rules_ap: %d, max_rules: %d)\n",
> i, num_6g_reg_rules_ap[i], MAX_6GHZ_REG_RULES);
> - kfree(tb);
> return -EINVAL;
> }
>
> @@ -5884,14 +5869,12 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> num_6g_reg_rules_cl[WMI_REG_VLP_AP][i] > MAX_6GHZ_REG_RULES) {
> ath12k_warn(ab, "Num 6g client reg rules exceeds max limit, for client(type: %d)\n",
> i);
> - kfree(tb);
> return -EINVAL;
> }
> }
>
> if (!total_reg_rules) {
> ath12k_warn(ab, "No reg rules available\n");
> - kfree(tb);
> return -EINVAL;
> }
>
> @@ -5993,7 +5976,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> ext_wmi_reg_rule);
>
> if (!reg_info->reg_rules_2g_ptr) {
> - kfree(tb);
> ath12k_warn(ab, "Unable to Allocate memory for 2g rules\n");
> return -ENOMEM;
> }
> @@ -6027,7 +6009,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> ext_wmi_reg_rule);
>
> if (!reg_info->reg_rules_5g_ptr) {
> - kfree(tb);
> ath12k_warn(ab, "Unable to Allocate memory for 5g rules\n");
> return -ENOMEM;
> }
> @@ -6046,7 +6027,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> ext_wmi_reg_rule);
>
> if (!reg_info->reg_rules_6g_ap_ptr[i]) {
> - kfree(tb);
> ath12k_warn(ab, "Unable to Allocate memory for 6g ap rules\n");
> return -ENOMEM;
> }
> @@ -6061,7 +6041,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
> ext_wmi_reg_rule);
>
> if (!reg_info->reg_rules_6g_client_ptr[j][i]) {
> - kfree(tb);
> ath12k_warn(ab, "Unable to Allocate memory for 6g client rules\n");
> return -ENOMEM;
> }
> @@ -6096,7 +6075,6 @@ static int ath12k_pull_reg_chan_list_ext_update_ev(struct ath12k_base *ab,
>
> ath12k_dbg(ab, ATH12K_DBG_WMI, "processed regulatory ext channel list\n");
>
> - kfree(tb);
> return 0;
> }
>
> @@ -6107,7 +6085,7 @@ static int ath12k_pull_peer_del_resp_ev(struct ath12k_base *ab, struct sk_buff *
> const struct wmi_peer_delete_resp_event *ev;
> int ret;
>
> - tb = ath12k_wmi_tlv_parse_alloc(ab, skb, GFP_ATOMIC);
> + tb = ath12k_wmi_tlv_parse(ab, skb);
> if (IS_ERR(tb)) {
> ret = PTR_ERR(tb);
> ath12k_warn(ab, "failed to parse tlv: %d\n", ret);
> @@ -6117,7 +6095,6 @@ static int ath12k_pull_peer_del_resp_ev(struct ath12k_base *ab, struct sk_buff *
> ev = tb[WMI_TAG_PEER_DELETE_RESP_EVENT];
> if (!ev) {
> ath12k_warn(ab, "failed to fetch peer delete resp ev");
> - kfree(tb);
> return -EPROTO;
> }
>
> @@ -6127,7 +6104,6 @@ static int ath12k_pull_peer_del_resp_ev(struct ath12k_base *ab, struct sk_buff *
> ether_addr_copy(peer_del_resp->peer_macaddr.addr,
> ev->peer_macaddr.addr);
>
> - kfree(tb);
> return 0;
> }
>
> @@ -6139,7 +6115,7 @@ static int ath12k_pull_vdev_del_resp_ev(struct ath12k_base *ab,
> const struct wmi_vdev_delete_resp_event *ev;
> int ret;
>
> - tb = ath12k_wmi_tlv_parse_alloc(ab, skb, GFP_ATOMIC);
> + tb = ath12k_wmi_tlv_parse(ab, skb);
> if (IS_ERR(tb)) {
> ret = PTR_ERR(tb);
> ath12k_warn(ab, "failed to parse tlv: %d\n", ret);
> @@ -6149,13 +6125,11 @@ static int ath12k_pull_vdev_del_resp_ev(struct ath12k_base *ab,
> ev = tb[WMI_TAG_VDEV_DELETE_RESP_EVENT];
> if (!ev) {
> ath12k_warn(ab, "failed to fetch vdev delete resp ev");
> - kfree(tb);
> return -EPROTO;
> }
>
> *vdev_id = le32_to_cpu(ev->vdev_id);
>
> - kfree(tb);
> return 0;
> }
>

and one more case to be handled,
ath12k_wmi_obss_color_collision_event()
const void **tb __free(kfree) = ath12k_wmi_tlv_parse_alloc(ab, skb,
GFP_ATOMIC);

Apart from this, the approach looks like a reasonable fix for the issue.

Another possible direction could be to define WMI event‑specific
structures (or possibly a union of structs) and then leverage
ath12k_wmi_tlv_parse() to extract only the required tags in a typed manner.

That said, such an approach would require a fair amount of refactoring,
so sticking with the per‑CPU scratch buffer for now seems pragmatic.
Moving towards a typed‑parse model incrementally could be something we
consider over time.

--
Ramesh