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

["Nicolas Escande" <nico.escande@gmail.com>]

On Mon Mar 9, 2026 at 9:16 PM CET, Jeff Johnson wrote:
> On 3/9/2026 8:20 AM, Nicolas Escande wrote:
>
> I have some nit comments...
>
>> On each WMI message received from the hardware, we alloc a temporary array
>
> note that not every WMI message handler uses this pattern.

Technically yes indeed. Maybe if I rephrase with something in the lines of:

When we receive a WMI message, we need to extract the relevant data from the
hardware depending on the event type. A common pattern is to alloc a temp
array of WMI_TAG_MAX entries of type void * in which we store pointers to
structs holding the actual data send by the chipset. Once it is no longer used
this array is then freed. This short lived allocation 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
>
> add hard stop .

ok

>
>> 
>> 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()
>
> s/accros/across/

ok

>
>> is also renamed into / merged with ath12k_wmi_tlv_parse() as it no longer
>> allocs memory but returns the existing per-cpu one.
>
> note that imperative voice is preferred.

s/So instead lets move/Move/ ?

>
>> 
>> 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 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()
>> 
>> Note I did try to move to a DEFINE_PER_CPU() allocated array but the module
>> loader complained about the array size. So I stuck to the per ab alloc. 
>> ---
>>  drivers/net/wireless/ath/ath12k/core.c |   7 +
>>  drivers/net/wireless/ath/ath12k/core.h |   2 +
>>  drivers/net/wireless/ath/ath12k/wmi.c  | 178 ++++++-------------------
>>  3 files changed, 51 insertions(+), 136 deletions(-)
> ...
>> @@ -3913,7 +3903,7 @@ ath12k_wmi_obss_color_collision_event(struct ath12k_base *ab, struct sk_buff *sk
>>  	u32 vdev_id, evt_type;
>>  	u64 bitmap;
>>  
>> -	const void **tb __free(kfree) = ath12k_wmi_tlv_parse_alloc(ab, skb, GFP_ATOMIC);
>> +	const void **tb = ath12k_wmi_tlv_parse(ab, skb);
>
> checkpatch complains:
> Missing a blank line after declarations

Weird that my checkpatch (from yesterday's ath-next) did not catch this:
scripts/checkpatch.pl \
	mail/0001-wifi-ath12k-avoid-dynamic-alloc-when-parsing-wmi-tb.patch
total: 0 errors, 0 warnings, 0 checks, 807 lines checked

Is there additionnal flags or tools I should run ?

>
> Note that when __free() is used that this rule is not enforced since __free()
> declarations should appear at first use
>
> So I'd separate the declaration of tb from the assignment to be aligned with
> all the other WMI functions

No problem I hesitated to be honest. And I think I prefer seperate declaration
anyway.

>
>>  	if (IS_ERR(tb)) {
>>  		ath12k_warn(ab, "failed to parse OBSS color collision tlv %ld\n",
>>  			    PTR_ERR(tb));
>
> that's it for the nits. the development is stress testing the functionality.

On that front, I can say that it has been running at scale in production on
around half a million devices for one mounth and so far so good. But there is
no need to rush this and we can gladly wait till you guys make the necessary
checks so you feel confident enough with it.

>
> /jeff