Wrong place for rxbaddr/txbaddr to be in struct ath12k_spt_info

Wrong place for rxbaddr/txbaddr to be in struct ath12k_spt_info

[Nicolas Escande]

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:

diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
index 61aa78d8bd8c..ecd3b5c76d26 100644
--- a/drivers/net/wireless/ath/ath12k/dp.c
+++ b/drivers/net/wireless/ath/ath12k/dp.c
@@ -1162,7 +1162,7 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
 	spin_lock_bh(&dp->rx_desc_lock);
 
 	for (i = 0; i < ATH12K_NUM_RX_SPT_PAGES; i++) {
-		desc_info = dp->spt_info->rxbaddr[i];
+		desc_info = dp->rxbaddr[i];
 
 		for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
 			if (!desc_info[j].in_use) {
@@ -1181,11 +1181,11 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
 	}
 
 	for (i = 0; i < ATH12K_NUM_RX_SPT_PAGES; i++) {
-		if (!dp->spt_info->rxbaddr[i])
+		if (!dp->rxbaddr[i])
 			continue;
 
-		kfree(dp->spt_info->rxbaddr[i]);
-		dp->spt_info->rxbaddr[i] = NULL;
+		kfree(dp->rxbaddr[i]);
+		dp->rxbaddr[i] = NULL;
 	}
 
 	spin_unlock_bh(&dp->rx_desc_lock);
@@ -1220,11 +1220,11 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
 
 		for (i = 0; i < ATH12K_TX_SPT_PAGES_PER_POOL; i++) {
 			tx_spt_page = i + pool_id * ATH12K_TX_SPT_PAGES_PER_POOL;
-			if (!dp->spt_info->txbaddr[tx_spt_page])
+			if (!dp->txbaddr[tx_spt_page])
 				continue;
 
-			kfree(dp->spt_info->txbaddr[tx_spt_page]);
-			dp->spt_info->txbaddr[tx_spt_page] = NULL;
+			kfree(dp->txbaddr[tx_spt_page]);
+			dp->txbaddr[tx_spt_page] = NULL;
 		}
 
 		spin_unlock_bh(&dp->tx_desc_lock[pool_id]);
@@ -1415,7 +1415,7 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
 
 		ppt_idx = ATH12K_RX_SPT_PAGE_OFFSET + i;
 		cookie_ppt_idx = dp->rx_ppt_base + ppt_idx;
-		dp->spt_info->rxbaddr[i] = &rx_descs[0];
+		dp->rxbaddr[i] = &rx_descs[0];
 
 		for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
 			rx_descs[j].cookie = ath12k_dp_cc_cookie_gen(cookie_ppt_idx, j);
@@ -1445,7 +1445,7 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
 			tx_spt_page = i + pool_id * ATH12K_TX_SPT_PAGES_PER_POOL;
 			ppt_idx = ATH12K_TX_SPT_PAGE_OFFSET + tx_spt_page;
 
-			dp->spt_info->txbaddr[tx_spt_page] = &tx_descs[0];
+			dp->txbaddr[tx_spt_page] = &tx_descs[0];
 
 			for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
 				tx_descs[j].desc_id = ath12k_dp_cc_cookie_gen(ppt_idx, j);
diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
index b77497c14ac4..28c8bf22810c 100644
--- a/drivers/net/wireless/ath/ath12k/dp.h
+++ b/drivers/net/wireless/ath/ath12k/dp.h
@@ -300,8 +300,6 @@ struct ath12k_tx_desc_info {
 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_reo_queue_ref {
@@ -352,6 +350,8 @@ struct ath12k_dp {
 	struct ath12k_spt_info *spt_info;
 	u32 num_spt_pages;
 	u32 rx_ppt_base;
+	struct ath12k_rx_desc_info *rxbaddr[ATH12K_NUM_RX_SPT_PAGES];
+	struct ath12k_tx_desc_info *txbaddr[ATH12K_NUM_TX_SPT_PAGES];
 	struct list_head rx_desc_free_list;
 	/* protects the free desc list */
 	spinlock_t rx_desc_lock;

Right ?

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

[Karthikeyan Periyasamy]

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:
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
> index 61aa78d8bd8c..ecd3b5c76d26 100644
> --- a/drivers/net/wireless/ath/ath12k/dp.c
> +++ b/drivers/net/wireless/ath/ath12k/dp.c
> @@ -1162,7 +1162,7 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
>   	spin_lock_bh(&dp->rx_desc_lock);
>   
>   	for (i = 0; i < ATH12K_NUM_RX_SPT_PAGES; i++) {
> -		desc_info = dp->spt_info->rxbaddr[i];
> +		desc_info = dp->rxbaddr[i];
>   
>   		for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
>   			if (!desc_info[j].in_use) {
> @@ -1181,11 +1181,11 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
>   	}
>   
>   	for (i = 0; i < ATH12K_NUM_RX_SPT_PAGES; i++) {
> -		if (!dp->spt_info->rxbaddr[i])
> +		if (!dp->rxbaddr[i])
>   			continue;
>   
> -		kfree(dp->spt_info->rxbaddr[i]);
> -		dp->spt_info->rxbaddr[i] = NULL;
> +		kfree(dp->rxbaddr[i]);
> +		dp->rxbaddr[i] = NULL;
>   	}
>   
>   	spin_unlock_bh(&dp->rx_desc_lock);
> @@ -1220,11 +1220,11 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
>   
>   		for (i = 0; i < ATH12K_TX_SPT_PAGES_PER_POOL; i++) {
>   			tx_spt_page = i + pool_id * ATH12K_TX_SPT_PAGES_PER_POOL;
> -			if (!dp->spt_info->txbaddr[tx_spt_page])
> +			if (!dp->txbaddr[tx_spt_page])
>   				continue;
>   
> -			kfree(dp->spt_info->txbaddr[tx_spt_page]);
> -			dp->spt_info->txbaddr[tx_spt_page] = NULL;
> +			kfree(dp->txbaddr[tx_spt_page]);
> +			dp->txbaddr[tx_spt_page] = NULL;
>   		}
>   
>   		spin_unlock_bh(&dp->tx_desc_lock[pool_id]);
> @@ -1415,7 +1415,7 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
>   
>   		ppt_idx = ATH12K_RX_SPT_PAGE_OFFSET + i;
>   		cookie_ppt_idx = dp->rx_ppt_base + ppt_idx;
> -		dp->spt_info->rxbaddr[i] = &rx_descs[0];
> +		dp->rxbaddr[i] = &rx_descs[0];
>   
>   		for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
>   			rx_descs[j].cookie = ath12k_dp_cc_cookie_gen(cookie_ppt_idx, j);
> @@ -1445,7 +1445,7 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
>   			tx_spt_page = i + pool_id * ATH12K_TX_SPT_PAGES_PER_POOL;
>   			ppt_idx = ATH12K_TX_SPT_PAGE_OFFSET + tx_spt_page;
>   
> -			dp->spt_info->txbaddr[tx_spt_page] = &tx_descs[0];
> +			dp->txbaddr[tx_spt_page] = &tx_descs[0];
>   
>   			for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
>   				tx_descs[j].desc_id = ath12k_dp_cc_cookie_gen(ppt_idx, j);
> diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
> index b77497c14ac4..28c8bf22810c 100644
> --- a/drivers/net/wireless/ath/ath12k/dp.h
> +++ b/drivers/net/wireless/ath/ath12k/dp.h
> @@ -300,8 +300,6 @@ struct ath12k_tx_desc_info {
>   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_reo_queue_ref {
> @@ -352,6 +350,8 @@ struct ath12k_dp {
>   	struct ath12k_spt_info *spt_info;
>   	u32 num_spt_pages;
>   	u32 rx_ppt_base;
> +	struct ath12k_rx_desc_info *rxbaddr[ATH12K_NUM_RX_SPT_PAGES];
> +	struct ath12k_tx_desc_info *txbaddr[ATH12K_NUM_TX_SPT_PAGES];
>   	struct list_head rx_desc_free_list;
>   	/* protects the free desc list */
>   	spinlock_t rx_desc_lock;
> 
> Right ?
> 

Yes, we always use the first element of dp->spt_info to store all the 
descriptors arrays.

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

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

[Nicolas Escande]

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.

Re: 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
--
கார்த்திகேயன் பெரியசாமி

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

[Nicolas Escande]

On Thu Aug 29, 2024 at 2:21 AM CEST, Karthikeyan Periyasamy wrote:
>
[...]
>
> Agree your point, can move rxbaddr[], txbaddr[] out of the spt_info.

Thanks, I'll send a patch for this then.