[PATCH v4 1/2] wifi: ath11k: use union for vaddr and iaddr in target_mem_chunk
Miaoqing Pan
quic_miaoqing at quicinc.com
Tue Jul 30 18:14:43 PDT 2024
On 7/31/2024 1:42 AM, Jeff Johnson wrote:
> 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 at 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
Actually, I want ath11k to be the same as ath12k. Anyway, it's a good
suggestion, will update.
More information about the ath11k
mailing list