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

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

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