| Message ID | 20240409151415.1226109-5-quic_periyasa@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: Refactor hardware cookie conversion | expand |
Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > Move the data path Tx and Rx descriptor primary page table CMEM > configuration into a helper function. This will make the code more > scalable for configuring partner device in support of multi-device MLO. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 > > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> [...] > +static void ath12k_dp_cmem_init(struct ath12k_base *ab, > + struct ath12k_dp *dp, > + enum ath12k_dp_desc_type type) > +{ > + u32 cmem_base; > + int i, start, end; > + > + cmem_base = ab->qmi.dev_mem[ATH12K_QMI_DEVMEM_CMEM_INDEX].start; > + > + switch (type) { > + case ATH12K_DP_TX_DESC: > + start = ATH12K_TX_SPT_PAGE_OFFSET; > + end = start + ATH12K_NUM_TX_SPT_PAGES; > + break; > + case ATH12K_DP_RX_DESC: > + start = ATH12K_RX_SPT_PAGE_OFFSET; > + end = start + ATH12K_NUM_RX_SPT_PAGES; > + break; > + default: > + ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type); > + return; > + } > + > + /* Write to PPT in CMEM */ > + for (i = start; i < end; i++) > + ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), > + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); > +} Here's a good example why I don't like functions returning void. How do we handle the errors in this case?
On 4/11/2024 3:15 PM, Kalle Valo wrote: > Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: > >> Move the data path Tx and Rx descriptor primary page table CMEM >> configuration into a helper function. This will make the code more >> scalable for configuring partner device in support of multi-device MLO. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 >> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 >> >> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> >> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> > > [...] > >> +static void ath12k_dp_cmem_init(struct ath12k_base *ab, >> + struct ath12k_dp *dp, >> + enum ath12k_dp_desc_type type) >> +{ >> + u32 cmem_base; >> + int i, start, end; >> + >> + cmem_base = ab->qmi.dev_mem[ATH12K_QMI_DEVMEM_CMEM_INDEX].start; >> + >> + switch (type) { >> + case ATH12K_DP_TX_DESC: >> + start = ATH12K_TX_SPT_PAGE_OFFSET; >> + end = start + ATH12K_NUM_TX_SPT_PAGES; >> + break; >> + case ATH12K_DP_RX_DESC: >> + start = ATH12K_RX_SPT_PAGE_OFFSET; >> + end = start + ATH12K_NUM_RX_SPT_PAGES; >> + break; >> + default: >> + ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type); >> + return; >> + } >> + >> + /* Write to PPT in CMEM */ >> + for (i = start; i < end; i++) >> + ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), >> + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); >> +} > > Here's a good example why I don't like functions returning void. How do > we handle the errors in this case? > sure, will handle the error case in the caller.
On 4/11/2024 3:07 AM, Karthikeyan Periyasamy wrote: > > > On 4/11/2024 3:15 PM, Kalle Valo wrote: >> Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes: >> >>> Move the data path Tx and Rx descriptor primary page table CMEM >>> configuration into a helper function. This will make the code more >>> scalable for configuring partner device in support of multi-device MLO. >>> >>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-QCAHKSWPL_SILICONZ-1 >>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 >>> >>> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> >>> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> >> >> [...] >> >>> +static void ath12k_dp_cmem_init(struct ath12k_base *ab, >>> + struct ath12k_dp *dp, >>> + enum ath12k_dp_desc_type type) >>> +{ >>> + u32 cmem_base; >>> + int i, start, end; >>> + >>> + cmem_base = ab->qmi.dev_mem[ATH12K_QMI_DEVMEM_CMEM_INDEX].start; >>> + >>> + switch (type) { >>> + case ATH12K_DP_TX_DESC: >>> + start = ATH12K_TX_SPT_PAGE_OFFSET; >>> + end = start + ATH12K_NUM_TX_SPT_PAGES; >>> + break; >>> + case ATH12K_DP_RX_DESC: >>> + start = ATH12K_RX_SPT_PAGE_OFFSET; >>> + end = start + ATH12K_NUM_RX_SPT_PAGES; >>> + break; >>> + default: >>> + ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type); >>> + return; >>> + } >>> + >>> + /* Write to PPT in CMEM */ >>> + for (i = start; i < end; i++) >>> + ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), >>> + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); >>> +} >> >> Here's a good example why I don't like functions returning void. How do >> we handle the errors in this case? >> > > sure, will handle the error case in the caller. > this is a static function with one caller. the only error is the default case which will never be hit. adding logic to return an error and then check it in the caller seems like overkill. why not just WARN() in the default case since this would be a logic error with newly added code?
Jeff Johnson <quic_jjohnson@quicinc.com> writes: >>>> + default: >>>> + ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type); >>>> + return; >>>> + } >>>> + >>>> + /* Write to PPT in CMEM */ >>>> + for (i = start; i < end; i++) >>>> + ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), >>>> + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); >>>> +} >>> >>> Here's a good example why I don't like functions returning void. How do >>> we handle the errors in this case? >>> >> >> sure, will handle the error case in the caller. >> > > this is a static function with one caller. the only error is the default case > which will never be hit. adding logic to return an error and then check it in > the caller seems like overkill. why not just WARN() in the default case since > this would be a logic error with newly added code? I think the software will be more robust then all errors are properly handled in a uniform way. For example, will everyone notice the warning message? What if the function is extended later and then the person doesn't add any error handling "because it didn't have that even earlier"? It's also a lot easier to review if error handling follows the same style throughout the driver. I didn't do any measurements but the overhead from this shouldn't be that large, maybe few bytes in the binary and few new lines in the source code. I think that's a reasonable price to pay from having more robust software. This is why I want to avoid void functions as much as possible. Of course there also are good cases when to use void functions, like here: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next&id=6ef5b4c9598c928b3e2c4c4b543c81331941f136
On 4/22/2024 5:04 AM, Kalle Valo wrote: > Jeff Johnson <quic_jjohnson@quicinc.com> writes: > >>>>> + default: >>>>> + ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type); >>>>> + return; >>>>> + } >>>>> + >>>>> + /* Write to PPT in CMEM */ >>>>> + for (i = start; i < end; i++) >>>>> + ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), >>>>> + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); >>>>> +} >>>> >>>> Here's a good example why I don't like functions returning void. How do >>>> we handle the errors in this case? >>>> >>> >>> sure, will handle the error case in the caller. >>> >> >> this is a static function with one caller. the only error is the default case >> which will never be hit. adding logic to return an error and then check it in >> the caller seems like overkill. why not just WARN() in the default case since >> this would be a logic error with newly added code? > > I think the software will be more robust then all errors are properly > handled in a uniform way. For example, will everyone notice the warning > message? What if the function is extended later and then the person > doesn't add any error handling "because it didn't have that even > earlier"? It's also a lot easier to review if error handling follows the > same style throughout the driver. A large number of coding errors occur in exception paths. So minimizing the number of exception paths decreases the opportunity for introducing these kind of errors. So the real trick is making sure "all errors are properly handled in a uniform way" /jeff
diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c index c8b2eb80b160..11d44e180b54 100644 --- a/drivers/net/wireless/ath/ath12k/dp.c +++ b/drivers/net/wireless/ath/ath12k/dp.c @@ -14,6 +14,11 @@ #include "peer.h" #include "dp_mon.h" +enum ath12k_dp_desc_type { + ATH12K_DP_TX_DESC, + ATH12K_DP_RX_DESC, +}; + static void ath12k_dp_htt_htc_tx_complete(struct ath12k_base *ab, struct sk_buff *skb) { @@ -1455,11 +1460,39 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab) return 0; } +static void ath12k_dp_cmem_init(struct ath12k_base *ab, + struct ath12k_dp *dp, + enum ath12k_dp_desc_type type) +{ + u32 cmem_base; + int i, start, end; + + cmem_base = ab->qmi.dev_mem[ATH12K_QMI_DEVMEM_CMEM_INDEX].start; + + switch (type) { + case ATH12K_DP_TX_DESC: + start = ATH12K_TX_SPT_PAGE_OFFSET; + end = start + ATH12K_NUM_TX_SPT_PAGES; + break; + case ATH12K_DP_RX_DESC: + start = ATH12K_RX_SPT_PAGE_OFFSET; + end = start + ATH12K_NUM_RX_SPT_PAGES; + break; + default: + ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type); + return; + } + + /* Write to PPT in CMEM */ + for (i = start; i < end; i++) + ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), + dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); +} + static int ath12k_dp_cc_init(struct ath12k_base *ab) { struct ath12k_dp *dp = &ab->dp; int i, ret = 0; - u32 cmem_base; INIT_LIST_HEAD(&dp->rx_desc_free_list); spin_lock_init(&dp->rx_desc_lock); @@ -1482,8 +1515,6 @@ static int ath12k_dp_cc_init(struct ath12k_base *ab) return -ENOMEM; } - cmem_base = ab->qmi.dev_mem[ATH12K_QMI_DEVMEM_CMEM_INDEX].start; - for (i = 0; i < dp->num_spt_pages; i++) { dp->spt_info[i].vaddr = dma_alloc_coherent(ab->dev, ATH12K_PAGE_SIZE, @@ -1500,12 +1531,11 @@ static int ath12k_dp_cc_init(struct ath12k_base *ab) ret = -EINVAL; goto free; } - - /* Write to PPT in CMEM */ - ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i), - dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET); } + ath12k_dp_cmem_init(ab, dp, ATH12K_DP_TX_DESC); + ath12k_dp_cmem_init(ab, dp, ATH12K_DP_RX_DESC); + ret = ath12k_dp_cc_desc_init(ab); if (ret) { ath12k_warn(ab, "HW CC desc init failed %d", ret);