* [PATCH v2 0/4] wifi: ath12k: Refactor hardware cookie conversion
@ 2024-04-09 15:14 Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 1/4] wifi: ath12k: avoid redundant code in Rx cookie conversion init Karthikeyan Periyasamy
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Karthikeyan Periyasamy @ 2024-04-09 15:14 UTC (permalink / raw
To: ath12k; +Cc: linux-wireless, Karthikeyan Periyasamy
Refactor the hardware cokie conversion initialization procedure to
support multi-device MLO.
This change is applicable to all platforms.
v2:
- Updated the cover letter message
- Updated the tested tag for WCN7850
Karthikeyan Periyasamy (4):
wifi: ath12k: avoid redundant code in Rx cookie conversion init
wifi: ath12k: Refactor the hardware cookie conversion init
wifi: ath12k: displace the Tx and Rx descriptor in cookie conversion
table
wifi: ath12k: Refactor data path cmem init
drivers/net/wireless/ath/ath12k/dp.c | 70 ++++++++++++++++++++++------
drivers/net/wireless/ath/ath12k/dp.h | 3 ++
2 files changed, 58 insertions(+), 15 deletions(-)
base-commit: dc410c4accd2fe64479a1f4ebc47ec9cd3928f4a
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/4] wifi: ath12k: avoid redundant code in Rx cookie conversion init
2024-04-09 15:14 [PATCH v2 0/4] wifi: ath12k: Refactor hardware cookie conversion Karthikeyan Periyasamy
@ 2024-04-09 15:14 ` Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 2/4] wifi: ath12k: Refactor the hardware " Karthikeyan Periyasamy
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Karthikeyan Periyasamy @ 2024-04-09 15:14 UTC (permalink / raw
To: ath12k; +Cc: linux-wireless, Karthikeyan Periyasamy, Jeff Johnson
Currently, in the Rx data path cookie conversion initialization procedure,
the primary page table index (ppt_idx) is computed within the secondary
page table iteration. However this is invariant, and hence the ppt_idx
should be calculated outside of the iteration to avoid repeated execution
of the statement.
Found in code review.
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>
---
drivers/net/wireless/ath/ath12k/dp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
index a0aa8c571867..796c757c0f58 100644
--- a/drivers/net/wireless/ath/ath12k/dp.c
+++ b/drivers/net/wireless/ath/ath12k/dp.c
@@ -1425,10 +1425,11 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
}
tx_spt_page = i + pool_id * ATH12K_TX_SPT_PAGES_PER_POOL;
+ ppt_idx = ATH12K_NUM_RX_SPT_PAGES + tx_spt_page;
+
dp->spt_info->txbaddr[tx_spt_page] = &tx_descs[0];
for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
- ppt_idx = ATH12K_NUM_RX_SPT_PAGES + tx_spt_page;
tx_descs[j].desc_id = ath12k_dp_cc_cookie_gen(ppt_idx, j);
tx_descs[j].pool_id = pool_id;
list_add_tail(&tx_descs[j].list,
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] wifi: ath12k: Refactor the hardware cookie conversion init
2024-04-09 15:14 [PATCH v2 0/4] wifi: ath12k: Refactor hardware cookie conversion Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 1/4] wifi: ath12k: avoid redundant code in Rx cookie conversion init Karthikeyan Periyasamy
@ 2024-04-09 15:14 ` Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 3/4] wifi: ath12k: displace the Tx and Rx descriptor in cookie conversion table Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init Karthikeyan Periyasamy
3 siblings, 0 replies; 10+ messages in thread
From: Karthikeyan Periyasamy @ 2024-04-09 15:14 UTC (permalink / raw
To: ath12k; +Cc: linux-wireless, Karthikeyan Periyasamy, Jeff Johnson
Currently, the Rx descriptor is placed before the Tx descriptor in the
primary page table of the hardware cookie conversion configuration. The
Tx and Rx descriptor offsets are implicitly hardcoded. To allow for easy
displacement of Tx and Rx descriptors, introduce Tx and Rx offset based
cookie conversion initializationi. Additionally, should consider
validating the respective offset ranges while retrieving the Tx and Rx
descriptors. This change will be utilize by the next patch in the series.
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>
---
drivers/net/wireless/ath/ath12k/dp.c | 25 +++++++++++++++++--------
drivers/net/wireless/ath/ath12k/dp.h | 3 +++
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
index 796c757c0f58..c8b2eb80b160 100644
--- a/drivers/net/wireless/ath/ath12k/dp.c
+++ b/drivers/net/wireless/ath/ath12k/dp.c
@@ -1344,12 +1344,16 @@ struct ath12k_rx_desc_info *ath12k_dp_get_rx_desc(struct ath12k_base *ab,
u32 cookie)
{
struct ath12k_rx_desc_info **desc_addr_ptr;
- u16 ppt_idx, spt_idx;
+ u16 start_ppt_idx, end_ppt_idx, ppt_idx, spt_idx;
ppt_idx = u32_get_bits(cookie, ATH12K_DP_CC_COOKIE_PPT);
spt_idx = u32_get_bits(cookie, ATH12K_DP_CC_COOKIE_SPT);
- if (ppt_idx > ATH12K_NUM_RX_SPT_PAGES ||
+ start_ppt_idx = ATH12K_RX_SPT_PAGE_OFFSET;
+ end_ppt_idx = start_ppt_idx + ATH12K_NUM_RX_SPT_PAGES;
+
+ if (ppt_idx < start_ppt_idx ||
+ ppt_idx >= end_ppt_idx ||
spt_idx > ATH12K_MAX_SPT_ENTRIES)
return NULL;
@@ -1362,13 +1366,17 @@ struct ath12k_tx_desc_info *ath12k_dp_get_tx_desc(struct ath12k_base *ab,
u32 cookie)
{
struct ath12k_tx_desc_info **desc_addr_ptr;
- u16 ppt_idx, spt_idx;
+ u16 start_ppt_idx, end_ppt_idx, ppt_idx, spt_idx;
ppt_idx = u32_get_bits(cookie, ATH12K_DP_CC_COOKIE_PPT);
spt_idx = u32_get_bits(cookie, ATH12K_DP_CC_COOKIE_SPT);
- if (ppt_idx < ATH12K_NUM_RX_SPT_PAGES ||
- ppt_idx > ab->dp.num_spt_pages ||
+ start_ppt_idx = ATH12K_TX_SPT_PAGE_OFFSET;
+ end_ppt_idx = start_ppt_idx +
+ (ATH12K_TX_SPT_PAGES_PER_POOL * ATH12K_HW_MAX_QUEUES);
+
+ if (ppt_idx < start_ppt_idx ||
+ ppt_idx >= end_ppt_idx ||
spt_idx > ATH12K_MAX_SPT_ENTRIES)
return NULL;
@@ -1397,15 +1405,16 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
return -ENOMEM;
}
+ ppt_idx = ATH12K_RX_SPT_PAGE_OFFSET + i;
dp->spt_info->rxbaddr[i] = &rx_descs[0];
for (j = 0; j < ATH12K_MAX_SPT_ENTRIES; j++) {
- rx_descs[j].cookie = ath12k_dp_cc_cookie_gen(i, j);
+ rx_descs[j].cookie = ath12k_dp_cc_cookie_gen(ppt_idx, j);
rx_descs[j].magic = ATH12K_DP_RX_DESC_MAGIC;
list_add_tail(&rx_descs[j].list, &dp->rx_desc_free_list);
/* Update descriptor VA in SPT */
- rx_desc_addr = ath12k_dp_cc_get_desc_addr_ptr(ab, i, j);
+ rx_desc_addr = ath12k_dp_cc_get_desc_addr_ptr(ab, ppt_idx, j);
*rx_desc_addr = &rx_descs[j];
}
}
@@ -1425,7 +1434,7 @@ static int ath12k_dp_cc_desc_init(struct ath12k_base *ab)
}
tx_spt_page = i + pool_id * ATH12K_TX_SPT_PAGES_PER_POOL;
- ppt_idx = ATH12K_NUM_RX_SPT_PAGES + tx_spt_page;
+ ppt_idx = ATH12K_TX_SPT_PAGE_OFFSET + tx_spt_page;
dp->spt_info->txbaddr[tx_spt_page] = &tx_descs[0];
diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
index 4939aa41dd87..57aba97cbdce 100644
--- a/drivers/net/wireless/ath/ath12k/dp.h
+++ b/drivers/net/wireless/ath/ath12k/dp.h
@@ -223,6 +223,9 @@ struct ath12k_pdev_dp {
#define ATH12K_NUM_TX_SPT_PAGES (ATH12K_TX_SPT_PAGES_PER_POOL * ATH12K_HW_MAX_QUEUES)
#define ATH12K_NUM_SPT_PAGES (ATH12K_NUM_RX_SPT_PAGES + ATH12K_NUM_TX_SPT_PAGES)
+#define ATH12K_TX_SPT_PAGE_OFFSET ATH12K_NUM_RX_SPT_PAGES
+#define ATH12K_RX_SPT_PAGE_OFFSET 0
+
/* The SPT pages are divided for RX and TX, first block for RX
* and remaining for TX
*/
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/4] wifi: ath12k: displace the Tx and Rx descriptor in cookie conversion table
2024-04-09 15:14 [PATCH v2 0/4] wifi: ath12k: Refactor hardware cookie conversion Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 1/4] wifi: ath12k: avoid redundant code in Rx cookie conversion init Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 2/4] wifi: ath12k: Refactor the hardware " Karthikeyan Periyasamy
@ 2024-04-09 15:14 ` Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init Karthikeyan Periyasamy
3 siblings, 0 replies; 10+ messages in thread
From: Karthikeyan Periyasamy @ 2024-04-09 15:14 UTC (permalink / raw
To: ath12k; +Cc: linux-wireless, Karthikeyan Periyasamy, Jeff Johnson
In the hardware cookie conversion table configuration, place the Rx
descriptor at the end. This will allow for easier addition of partner
device Rx descriptors in the future to support 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>
---
drivers/net/wireless/ath/ath12k/dp.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/dp.h b/drivers/net/wireless/ath/ath12k/dp.h
index 57aba97cbdce..5cf0d21ef184 100644
--- a/drivers/net/wireless/ath/ath12k/dp.h
+++ b/drivers/net/wireless/ath/ath12k/dp.h
@@ -223,8 +223,8 @@ struct ath12k_pdev_dp {
#define ATH12K_NUM_TX_SPT_PAGES (ATH12K_TX_SPT_PAGES_PER_POOL * ATH12K_HW_MAX_QUEUES)
#define ATH12K_NUM_SPT_PAGES (ATH12K_NUM_RX_SPT_PAGES + ATH12K_NUM_TX_SPT_PAGES)
-#define ATH12K_TX_SPT_PAGE_OFFSET ATH12K_NUM_RX_SPT_PAGES
-#define ATH12K_RX_SPT_PAGE_OFFSET 0
+#define ATH12K_TX_SPT_PAGE_OFFSET 0
+#define ATH12K_RX_SPT_PAGE_OFFSET ATH12K_NUM_TX_SPT_PAGES
/* The SPT pages are divided for RX and TX, first block for RX
* and remaining for TX
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
2024-04-09 15:14 [PATCH v2 0/4] wifi: ath12k: Refactor hardware cookie conversion Karthikeyan Periyasamy
` (2 preceding siblings ...)
2024-04-09 15:14 ` [PATCH v2 3/4] wifi: ath12k: displace the Tx and Rx descriptor in cookie conversion table Karthikeyan Periyasamy
@ 2024-04-09 15:14 ` Karthikeyan Periyasamy
2024-04-11 9:45 ` Kalle Valo
3 siblings, 1 reply; 10+ messages in thread
From: Karthikeyan Periyasamy @ 2024-04-09 15:14 UTC (permalink / raw
To: ath12k; +Cc: linux-wireless, Karthikeyan Periyasamy, Jeff Johnson
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>
---
drivers/net/wireless/ath/ath12k/dp.c | 44 +++++++++++++++++++++++-----
1 file changed, 37 insertions(+), 7 deletions(-)
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);
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
2024-04-09 15:14 ` [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init Karthikeyan Periyasamy
@ 2024-04-11 9:45 ` Kalle Valo
2024-04-11 10:07 ` Karthikeyan Periyasamy
0 siblings, 1 reply; 10+ messages in thread
From: Kalle Valo @ 2024-04-11 9:45 UTC (permalink / raw
To: Karthikeyan Periyasamy; +Cc: ath12k, linux-wireless, Jeff Johnson
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?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
2024-04-11 9:45 ` Kalle Valo
@ 2024-04-11 10:07 ` Karthikeyan Periyasamy
2024-04-11 15:20 ` Jeff Johnson
0 siblings, 1 reply; 10+ messages in thread
From: Karthikeyan Periyasamy @ 2024-04-11 10:07 UTC (permalink / raw
To: Kalle Valo; +Cc: ath12k, linux-wireless, Jeff Johnson
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.
--
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
2024-04-11 10:07 ` Karthikeyan Periyasamy
@ 2024-04-11 15:20 ` Jeff Johnson
2024-04-22 12:04 ` Kalle Valo
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Johnson @ 2024-04-11 15:20 UTC (permalink / raw
To: Karthikeyan Periyasamy, Kalle Valo; +Cc: ath12k, linux-wireless
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
2024-04-11 15:20 ` Jeff Johnson
@ 2024-04-22 12:04 ` Kalle Valo
2024-04-22 16:09 ` Jeff Johnson
0 siblings, 1 reply; 10+ messages in thread
From: Kalle Valo @ 2024-04-22 12:04 UTC (permalink / raw
To: Jeff Johnson; +Cc: Karthikeyan Periyasamy, ath12k, linux-wireless
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
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
2024-04-22 12:04 ` Kalle Valo
@ 2024-04-22 16:09 ` Jeff Johnson
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Johnson @ 2024-04-22 16:09 UTC (permalink / raw
To: Kalle Valo; +Cc: Karthikeyan Periyasamy, ath12k, linux-wireless
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-22 16:09 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-09 15:14 [PATCH v2 0/4] wifi: ath12k: Refactor hardware cookie conversion Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 1/4] wifi: ath12k: avoid redundant code in Rx cookie conversion init Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 2/4] wifi: ath12k: Refactor the hardware " Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 3/4] wifi: ath12k: displace the Tx and Rx descriptor in cookie conversion table Karthikeyan Periyasamy
2024-04-09 15:14 ` [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init Karthikeyan Periyasamy
2024-04-11 9:45 ` Kalle Valo
2024-04-11 10:07 ` Karthikeyan Periyasamy
2024-04-11 15:20 ` Jeff Johnson
2024-04-22 12:04 ` Kalle Valo
2024-04-22 16:09 ` Jeff Johnson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.