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

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

On Tue Mar 3, 2026 at 10:53 AM CET, Rameshkumar Sundaram wrote:
[...]
>>   
>> @@ -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
>

I'll look into it.

>>   	init_completion(&ab->driver_recovery);
>>   
>>   	ab->workqueue = create_singlethread_workqueue("ath12k_wq");

[...]

>> -	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 *)

sure

>
>>   
>> -	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;
>>   }

[...]

>
> 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);
>

Sure as I said, this was whipped up from an older kernel to see if there was
interrest. I'll fix all usages for the proper submission 

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

Good

> 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.
>

I looked briefly into this as it seems it was in fact done that way in ath10k.
But as in some rare occasion there are more than one single struct that gets
parsed from one wmi message, I guess thats the reason it was changed to this
pattern in ath11k.

> 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.
>

Indeed you would gain back type safety but this would mean a fair amount of code
change.

>
> --
> Ramesh