Re: Wrong place for rxbaddr/txbaddr to be in struct ath12k_spt_info

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

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.