Re: [PATCH] wifi: ath11k: move .max_tx_ring to struct ath11k_hw_hal_params

On 12/11/2025 12:28 AM, [email protected] wrote:
> On 12/10/25 12:46 AM, Baochen Qiang wrote:
>>
>>
>> On 12/10/2025 10:40 AM, Alexandru Gagniuc wrote:
>>> ".max_tx_ring" is an upper bounds to indexing ".tcl2wbm_rbm_map". It
>>> is initialized in, core.c, a different file than the array init. This
>>> spaghetti-like relation is fragile and not obvious. Accidentally
>>> setting ".max_tx_ring" too high leads to a hard to track out-of-
>>> bounds access and memory corruption.
>>>
>>> Clarify this dependency by moving ".max_tx_ring" adjacent to the array
>>> ".tcl2wbm_rbm_map". Use ARRAY_SIZE() instead of #defines to initialize
>>> the length field. Remove DP_TCL_NUM_RING_MAX_QCA6390, as it is no
>>> longer required.
>>>
>>> The intent is to make the code easier to understand rather than fix
>>> an existing bug.
>>>
>>
>> Even the code chane works, I am not sure whether we should do this. Because, logically
>> max_tx_ring represents hardware capability which is static. However the change actually
>> implies max_tx_ring varies on tcl2wbm_rbm_map definition.
> 
> I see what you mean, although tcl2wbm_rbm_map is const. More details below.
> 
> 
>> If we are going to add something to avoid the potential out-of-bound access or to improve
>> code readability, how about something like
>>
>>     ASSERT(hw_params.max_tx_ring <= ARRAR_SIZE(tcl2wbm_rbm_map))
> A static assert might be a good solution. I don't know how to do that.
> By the time we have hw_params.max_tx_ring and tcl2wbm_rbm_map, the
> latter is a pointer, so we can't use ARRAY_SIZE(). We could try to do

Yeah, that is a problem ...

> it dynamically, but I feel that's spaghetti code:
> 
>     if (tcl2wbm_rbm_map == &ath11k_hw_hal_params_ipq8074)
>         ASSERT(hw_params.max_tx_ring <= ARRAY_SIZE(ath11k_hw_hal_params_ipq8074));
>     else if (...)
>         ...
> 

Hmm, this is ugly. I would rather have

	map_size = ARRAR_SIZE()

in each hal_params, and then

	ASSERT(hw_params.max_tx_ring <= hw_params.hal_params->map_size)

> Alternatively, I can take the suggestion from your other email, and
> keep the "max_tx_ring", or "num_tx_rings" name. Because it is part of
> the hw_params struct (via .hal_params), it still describes the
> hardware. While the value is derived from a constant array, instead
> of being hardcoded, it remains an immutable quantity, consistent with
> a static hardware descriptor, would you agree?

Also works for me. You can make the decision.

> 
> Alex
>