| Message ID | 20240730035955.788768-1-quic_miaoqing@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v4,1/2] wifi: ath11k: use union for vaddr and iaddr in target_mem_chunk | expand |
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 {
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@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.
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 {
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(-)