linux-kernel - Re: [PATCH] wifi: ath11k: move .max_tx_ring to struct ath11k_hw_hal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <16e02bce-18c3-42e4-a51d-60c4bdf8a763@gmail.com>
Date: Wed, 10 Dec 2025 10:28:30 -0600
From: mr.nuke.me@...il.com
To: Baochen Qiang <baochen.qiang@....qualcomm.com>,
 ath11k@...ts.infradead.org, Jeff Johnson <jjohnson@...nel.org>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] wifi: ath11k: move .max_tx_ring to struct
 ath11k_hw_hal_params

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

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?

Alex