diff mbox series

[v4,1/2] wifi: ath11k: use union for vaddr and iaddr in target_mem_chunk

Message ID 20240730035955.788768-1-quic_miaoqing@quicinc.com
State New
Headers show
Series [v4,1/2] wifi: ath11k: use union for vaddr and iaddr in target_mem_chunk | expand

Commit Message

Miaoqing Pan July 30, 2024, 3:59 a.m. UTC
The value of 'ab->hw_params.fixed_mem_region' determins that
only one variable 'vaddr' or 'iaddr' is used in target_mem_chunk.
So use union instead, easy to check whether the memory is set
or not.

Tested-on: WCN6855 hw2.1 PCI WLAN.HSP.1.1-04358-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1

Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/qmi.c | 45 ++++++++++++++-------------
 drivers/net/wireless/ath/ath11k/qmi.h |  8 +++--
 2 files changed, 29 insertions(+), 24 deletions(-)

Comments

Jeff Johnson July 30, 2024, 5:42 p.m. UTC | #1
On 7/29/2024 8:59 PM, Miaoqing Pan wrote:
> The value of 'ab->hw_params.fixed_mem_region' determins that

s/determins/determines/

> only one variable 'vaddr' or 'iaddr' is used in target_mem_chunk.
> So use union instead, easy to check whether the memory is set
> or not.
> 
> Tested-on: WCN6855 hw2.1 PCI WLAN.HSP.1.1-04358-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
> 
> Signed-off-by: Miaoqing Pan <quic_miaoqing@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath11k/qmi.c | 45 ++++++++++++++-------------
>  drivers/net/wireless/ath/ath11k/qmi.h |  8 +++--
>  2 files changed, 29 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
> index 1bc648920ab6..ee32027badcf 100644
> --- a/drivers/net/wireless/ath/ath11k/qmi.c
> +++ b/drivers/net/wireless/ath/ath11k/qmi.c
> @@ -1955,19 +1955,21 @@ static void ath11k_qmi_free_target_mem_chunk(struct ath11k_base *ab)
>  	int i;
>  
>  	for (i = 0; i < ab->qmi.mem_seg_count; i++) {
> -		if ((ab->hw_params.fixed_mem_region ||
> -		     test_bit(ATH11K_FLAG_FIXED_MEM_RGN, &ab->dev_flags)) &&
> -		     ab->qmi.target_mem[i].iaddr)
> -			iounmap(ab->qmi.target_mem[i].iaddr);
> +		if (!ab->qmi.target_mem[i].v.iaddr)

see my comment at the end about potentially adding a new member to test for NULL

> +			continue;
>  
> -		if (!ab->qmi.target_mem[i].vaddr)
> +		if (ab->hw_params.fixed_mem_region ||
> +		    test_bit(ATH11K_FLAG_FIXED_MEM_RGN, &ab->dev_flags)) {
> +			iounmap(ab->qmi.target_mem[i].v.iaddr);
> +			ab->qmi.target_mem[i].v.iaddr = NULL;
>  			continue;
> +		}
>  
>  		dma_free_coherent(ab->dev,
>  				  ab->qmi.target_mem[i].prev_size,
> -				  ab->qmi.target_mem[i].vaddr,
> +				  ab->qmi.target_mem[i].v.vaddr,
>  				  ab->qmi.target_mem[i].paddr);
> -		ab->qmi.target_mem[i].vaddr = NULL;
> +		ab->qmi.target_mem[i].v.vaddr = NULL;
>  	}
>  }
>  
> @@ -1984,22 +1986,22 @@ static int ath11k_qmi_alloc_target_mem_chunk(struct ath11k_base *ab)
>  		/* Firmware reloads in coldboot/firmware recovery.
>  		 * in such case, no need to allocate memory for FW again.
>  		 */
> -		if (chunk->vaddr) {
> +		if (chunk->v.vaddr) {
>  			if (chunk->prev_type == chunk->type &&
>  			    chunk->prev_size == chunk->size)
>  				continue;
>  
>  			/* cannot reuse the existing chunk */
>  			dma_free_coherent(ab->dev, chunk->prev_size,
> -					  chunk->vaddr, chunk->paddr);
> -			chunk->vaddr = NULL;
> +					  chunk->v.vaddr, chunk->paddr);
> +			chunk->v.vaddr = NULL;
>  		}
>  
> -		chunk->vaddr = dma_alloc_coherent(ab->dev,
> -						  chunk->size,
> -						  &chunk->paddr,
> -						  GFP_KERNEL | __GFP_NOWARN);
> -		if (!chunk->vaddr) {
> +		chunk->v.vaddr = dma_alloc_coherent(ab->dev,
> +						    chunk->size,
> +						    &chunk->paddr,
> +						    GFP_KERNEL | __GFP_NOWARN);
> +		if (!chunk->v.vaddr) {
>  			if (ab->qmi.mem_seg_count <= ATH11K_QMI_FW_MEM_REQ_SEGMENT_CNT) {
>  				ath11k_dbg(ab, ATH11K_DBG_QMI,
>  					   "dma allocation failed (%d B type %u), will try later with small size\n",
> @@ -2055,10 +2057,10 @@ static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
>  			}
>  
>  			ab->qmi.target_mem[idx].paddr = res.start;
> -			ab->qmi.target_mem[idx].iaddr =
> +			ab->qmi.target_mem[idx].v.iaddr =
>  				ioremap(ab->qmi.target_mem[idx].paddr,
>  					ab->qmi.target_mem[i].size);
> -			if (!ab->qmi.target_mem[idx].iaddr)
> +			if (!ab->qmi.target_mem[idx].v.iaddr)
>  				return -EIO;
>  
>  			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
> @@ -2068,7 +2070,7 @@ static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
>  			break;
>  		case BDF_MEM_REGION_TYPE:
>  			ab->qmi.target_mem[idx].paddr = ab->hw_params.bdf_addr;
> -			ab->qmi.target_mem[idx].vaddr = NULL;
> +			ab->qmi.target_mem[idx].v.iaddr = NULL;
>  			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
>  			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
>  			idx++;
> @@ -2083,18 +2085,19 @@ static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
>  				if (hremote_node) {
>  					ab->qmi.target_mem[idx].paddr =
>  							res.start + host_ddr_sz;
> -					ab->qmi.target_mem[idx].iaddr =
> +					ab->qmi.target_mem[idx].v.iaddr =
>  						ioremap(ab->qmi.target_mem[idx].paddr,
>  							ab->qmi.target_mem[i].size);
> -					if (!ab->qmi.target_mem[idx].iaddr)
> +					if (!ab->qmi.target_mem[idx].v.iaddr)
>  						return -EIO;
>  				} else {
>  					ab->qmi.target_mem[idx].paddr =
>  						ATH11K_QMI_CALDB_ADDRESS;
> +					ab->qmi.target_mem[idx].v.iaddr = NULL;
>  				}
>  			} else {
>  				ab->qmi.target_mem[idx].paddr = 0;
> -				ab->qmi.target_mem[idx].vaddr = NULL;
> +				ab->qmi.target_mem[idx].v.iaddr = NULL;
>  			}
>  			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
>  			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
> diff --git a/drivers/net/wireless/ath/ath11k/qmi.h b/drivers/net/wireless/ath/ath11k/qmi.h
> index 7e06d100af57..63c957a7075e 100644
> --- a/drivers/net/wireless/ath/ath11k/qmi.h
> +++ b/drivers/net/wireless/ath/ath11k/qmi.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: BSD-3-Clause-Clear */
>  /*
>   * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
> - * Copyright (c) 2021-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #ifndef ATH11K_QMI_H
> @@ -102,8 +102,10 @@ struct target_mem_chunk {
>  	u32 prev_size;
>  	u32 prev_type;
>  	dma_addr_t paddr;
> -	u32 *vaddr;
> -	void __iomem *iaddr;
> +	union {
> +		u32 *vaddr;
> +		void __iomem *iaddr;
> +	} v;

is there a reason you didn't incorporate my prior observation:

...if you make it an anonymous union then most, if not all, of the
code changes are unnecessary.

I'm also thinking it may make the code even cleaner to add a third member to
the union:
	void *anyaddr

So you set and clear either vaddr or iaddr based upon the usage, but test
anyaddr when testing for NULL

>  };
>  
>  struct target_info {
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index 1bc648920ab6..ee32027badcf 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -1955,19 +1955,21 @@  static void ath11k_qmi_free_target_mem_chunk(struct ath11k_base *ab)
 	int i;
 
 	for (i = 0; i < ab->qmi.mem_seg_count; i++) {
-		if ((ab->hw_params.fixed_mem_region ||
-		     test_bit(ATH11K_FLAG_FIXED_MEM_RGN, &ab->dev_flags)) &&
-		     ab->qmi.target_mem[i].iaddr)
-			iounmap(ab->qmi.target_mem[i].iaddr);
+		if (!ab->qmi.target_mem[i].v.iaddr)
+			continue;
 
-		if (!ab->qmi.target_mem[i].vaddr)
+		if (ab->hw_params.fixed_mem_region ||
+		    test_bit(ATH11K_FLAG_FIXED_MEM_RGN, &ab->dev_flags)) {
+			iounmap(ab->qmi.target_mem[i].v.iaddr);
+			ab->qmi.target_mem[i].v.iaddr = NULL;
 			continue;
+		}
 
 		dma_free_coherent(ab->dev,
 				  ab->qmi.target_mem[i].prev_size,
-				  ab->qmi.target_mem[i].vaddr,
+				  ab->qmi.target_mem[i].v.vaddr,
 				  ab->qmi.target_mem[i].paddr);
-		ab->qmi.target_mem[i].vaddr = NULL;
+		ab->qmi.target_mem[i].v.vaddr = NULL;
 	}
 }
 
@@ -1984,22 +1986,22 @@  static int ath11k_qmi_alloc_target_mem_chunk(struct ath11k_base *ab)
 		/* Firmware reloads in coldboot/firmware recovery.
 		 * in such case, no need to allocate memory for FW again.
 		 */
-		if (chunk->vaddr) {
+		if (chunk->v.vaddr) {
 			if (chunk->prev_type == chunk->type &&
 			    chunk->prev_size == chunk->size)
 				continue;
 
 			/* cannot reuse the existing chunk */
 			dma_free_coherent(ab->dev, chunk->prev_size,
-					  chunk->vaddr, chunk->paddr);
-			chunk->vaddr = NULL;
+					  chunk->v.vaddr, chunk->paddr);
+			chunk->v.vaddr = NULL;
 		}
 
-		chunk->vaddr = dma_alloc_coherent(ab->dev,
-						  chunk->size,
-						  &chunk->paddr,
-						  GFP_KERNEL | __GFP_NOWARN);
-		if (!chunk->vaddr) {
+		chunk->v.vaddr = dma_alloc_coherent(ab->dev,
+						    chunk->size,
+						    &chunk->paddr,
+						    GFP_KERNEL | __GFP_NOWARN);
+		if (!chunk->v.vaddr) {
 			if (ab->qmi.mem_seg_count <= ATH11K_QMI_FW_MEM_REQ_SEGMENT_CNT) {
 				ath11k_dbg(ab, ATH11K_DBG_QMI,
 					   "dma allocation failed (%d B type %u), will try later with small size\n",
@@ -2055,10 +2057,10 @@  static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
 			}
 
 			ab->qmi.target_mem[idx].paddr = res.start;
-			ab->qmi.target_mem[idx].iaddr =
+			ab->qmi.target_mem[idx].v.iaddr =
 				ioremap(ab->qmi.target_mem[idx].paddr,
 					ab->qmi.target_mem[i].size);
-			if (!ab->qmi.target_mem[idx].iaddr)
+			if (!ab->qmi.target_mem[idx].v.iaddr)
 				return -EIO;
 
 			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
@@ -2068,7 +2070,7 @@  static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
 			break;
 		case BDF_MEM_REGION_TYPE:
 			ab->qmi.target_mem[idx].paddr = ab->hw_params.bdf_addr;
-			ab->qmi.target_mem[idx].vaddr = NULL;
+			ab->qmi.target_mem[idx].v.iaddr = NULL;
 			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
 			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
 			idx++;
@@ -2083,18 +2085,19 @@  static int ath11k_qmi_assign_target_mem_chunk(struct ath11k_base *ab)
 				if (hremote_node) {
 					ab->qmi.target_mem[idx].paddr =
 							res.start + host_ddr_sz;
-					ab->qmi.target_mem[idx].iaddr =
+					ab->qmi.target_mem[idx].v.iaddr =
 						ioremap(ab->qmi.target_mem[idx].paddr,
 							ab->qmi.target_mem[i].size);
-					if (!ab->qmi.target_mem[idx].iaddr)
+					if (!ab->qmi.target_mem[idx].v.iaddr)
 						return -EIO;
 				} else {
 					ab->qmi.target_mem[idx].paddr =
 						ATH11K_QMI_CALDB_ADDRESS;
+					ab->qmi.target_mem[idx].v.iaddr = NULL;
 				}
 			} else {
 				ab->qmi.target_mem[idx].paddr = 0;
-				ab->qmi.target_mem[idx].vaddr = NULL;
+				ab->qmi.target_mem[idx].v.iaddr = NULL;
 			}
 			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
 			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
diff --git a/drivers/net/wireless/ath/ath11k/qmi.h b/drivers/net/wireless/ath/ath11k/qmi.h
index 7e06d100af57..63c957a7075e 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.h
+++ b/drivers/net/wireless/ath/ath11k/qmi.h
@@ -1,7 +1,7 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause-Clear */
 /*
  * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef ATH11K_QMI_H
@@ -102,8 +102,10 @@  struct target_mem_chunk {
 	u32 prev_size;
 	u32 prev_type;
 	dma_addr_t paddr;
-	u32 *vaddr;
-	void __iomem *iaddr;
+	union {
+		u32 *vaddr;
+		void __iomem *iaddr;
+	} v;
 };
 
 struct target_info {