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

Rameshkumar Sundaram
2026-03-23T12:41:14Z

On 3/19/2026 9:29 PM, Jeff Johnson wrote:
> On 3/19/2026 7:35 AM, Nicolas Escande wrote:
>> On Thu Mar 19, 2026 at 12:08 PM CET, Rameshkumar Sundaram wrote:
>>>
>>> Since CONFIG_ATH12K is tristate, a built-in boot can continue past a
>>> failed ath12k_init() and still run ath12k_wifi7_init().
>>>
>> I genuinely thought the kernel prevented this. I was wrong.
>>
>>> Please ensure that later initialization path is guarded against
>>> allocation failure.
>>>
>> I can add a flag like so to be able to check from ath12k_wifi7_init() if the
>> init finished ok. Something in the lines of
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
>> index 6c034071cc6d..742fb33f41ff 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.c
>> +++ b/drivers/net/wireless/ath/ath12k/core.c
>> @@ -34,6 +34,9 @@ module_param_named(ftm_mode, ath12k_ftm_mode, bool, 0444);
>> MODULE_PARM_DESC(ftm_mode, "Boots up in factory test mode");
>> EXPORT_SYMBOL(ath12k_ftm_mode);
>>
>> +bool ath12k_init_ok = false;
>> +EXPORT_SYMBOL(ath12k_init_ok);
>> +
>> /* protected with ath12k_hw_group_mutex */
>> static struct list_head ath12k_hw_group_list = LIST_HEAD_INIT(ath12k_hw_group_list);
>>
>> @@ -2323,7 +2326,14 @@ struct ath12k_base *ath12k_core_alloc(struct device *dev, size_t priv_size,
>>
>> static int ath12k_init(void)
>> {
>> - return ath12k_wmi_alloc();
>> + int ret;
>> +
>> + ret = ath12k_wmi_alloc();
>> + if (ret)
>> + return -ENOMEM;
>> +
>> + ath12k_init_ok = true;
>> + return 0;
>> }
>>
>> static void ath12k_exit(void)
>> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
>> index 59c193b24764..f35571b1a541 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.h
>> +++ b/drivers/net/wireless/ath/ath12k/core.h
>> @@ -101,6 +101,8 @@ enum ath12k_crypt_mode {
>> ATH12K_CRYPT_MODE_SW,
>> };
>>
>> +extern bool ath12k_init_ok;
>> +
>> static inline enum wme_ac ath12k_tid_to_ac(u32 tid)
>> {
>> return (((tid == 0) || (tid == 3)) ? WME_AC_BE :
>> diff --git a/drivers/net/wireless/ath/ath12k/wifi7/core.c b/drivers/net/wireless/ath/ath12k/wifi7/core.c
>> index a02c57acf137..542ec10fabf1 100644
>> --- a/drivers/net/wireless/ath/ath12k/wifi7/core.c
>> +++ b/drivers/net/wireless/ath/ath12k/wifi7/core.c
>> @@ -38,6 +38,9 @@ void ath12k_wifi7_arch_deinit(struct ath12k_base *ab)
>>
>> static int ath12k_wifi7_init(void)
>> {
>> + if (!ath12k_init_ok)
>> + return -ENOTSUPP;
>> +
>> ahb_err = ath12k_wifi7_ahb_init();
>> if (ahb_err)
>> pr_warn("Failed to initialize ath12k Wi-Fi 7 AHB device: %d\n",
>>
>>
>> I don't like it much but it is easy enough.
>> But I don't know if there is a more idiomatic way of doing things
>
> I'd prefer to expose a function rather than a global variable.
> In other words keep the flag static, and expose a function that returns the
> value of the flag, i.e.:
>
> bool ath12k_core_initialized(void)
> {
> return ath12k_init_ok;
> }
> EXPORT_SYMBOL(ath12k_core_initialized);
>
>>
>>> Or may be have this allocated on first device probe and free it on last
>>> device deinit ?
>>
>> That seems even more involved. It would be easier to go back to the previous
>> version and simply, alloc it once per ath12k_base
>>
>> What do you guys think ?
>>
>
> Going back to that may be the better solution. It isn't nice that this current
> solution may allocate memory when the driver isn't actually used. But I'll let
> others on the team weigh in as well.
>

Yeah, allocating once per ath12k_base is definitely the simpler
ownership model.
I was only wondering whether sharing it across devices might be worth a
look, since this is per-CPU scratch space and the table itself is fairly
large.

--
Ramesh