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@xxxxxxxxxxx>
> ---
> 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 {
