Wrong place for rxbaddr/txbaddr to be in struct ath12k_spt_info

[Karthikeyan Periyasamy]

On 8/29/2024 2:32 AM, Nicolas Escande wrote:
> On Wed Aug 28, 2024 at 6:58 PM CEST, Karthikeyan Periyasamy wrote:
>>
>>
>> On 8/28/2024 7:34 PM, Nicolas Escande wrote:
>>> Hi there,
>>>
>>> Looking into a problem we have on an ath12k platform at work with a colleague,
>>> I stumbled upon something that seems weird.
>>>
>>> The interresting parts of dh.h:
>>>
>>> struct ath12k_spt_info {
>>> 	dma_addr_t paddr;
>>> 	u64 *vaddr;
>>> 	struct ath12k_rx_desc_info *rxbaddr[ATH12K_NUM_RX_SPT_PAGES];
>>> 	struct ath12k_tx_desc_info *txbaddr[ATH12K_NUM_TX_SPT_PAGES];
>>> };
>>>
>>> ...
>>>
>>> struct ath12k_dp {
>>> 	struct ath12k_base *ab;
>>> 	...
>>> 	struct ath12k_spt_info *spt_info;
>>> 	u32 num_spt_pages;
>>> 	u32 rx_ppt_base;
>>> 	...
>>> };
>>>
>>> In dp.c we have ath12k_dp_cc_desc_init that allocs arrays of ath12k_rx_desc_info
>>> stores the addresses of the individual desc in each of the spt entries using
>>> ath12k_dp_cc_get_desc_addr_ptr(), but we also save the address of the whole
>>> array for later cleanup by dp->spt_info->rxbaddr[i] = &rx_descs[0];
>>>
>>> Surely this is wrong, with the current code we always use the first element of
>>> dp->spt_info to store all the descriptors arrays. And the same goes for txbaddr.
>>> I mean it works, but we loose memory for no purpose & make this part of the
>>> driver harder to understand.
>>>
>>> It should be something like:
>>>
> ...
> 
>>>
>>> Right ?
>>>
>>
>> Yes, we always use the first element of dp->spt_info to store all the
>> descriptors arrays.
> 
> Well that was not what I expected. So please enlighten me, what's the rationale
> behind deliberately wasting around ATH12K_NUM_SPT_PAGES * sizeof(uintptr_t) *
> (ATH12K_NUM_RX_SPT_PAGES + ATH12K_NUM_TX_SPT_PAGES) of memory ? I'm quite
> puzzled by this decision... I feel it really doesn't help understanding this
> already complicated enough piece of code.

Agree your point, can move rxbaddr[], txbaddr[] out of the spt_info.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி