[PATCH net v3 0/2] net: ethernet: ti: am65-cpsw: Fixes to multi queue RX feature

[PATCH net v3 0/2] net: ethernet: ti: am65-cpsw: Fixes to multi queue RX feature

[Roger Quadros]

On J7 platforms, setting up multiple RX flows was failing
as the RX free descriptor ring 0 is shared among all flows
and we did not allocate enough elements in the RX free descriptor
ring 0 to accommodate for all RX flows. Patch 1 fixes this.

The second patch fixes a warning if there was any error in
am65_cpsw_nuss_init_rx_chns() and am65_cpsw_nuss_cleanup_rx_chns()
was called after that.

Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
Changes in v3:
- Reverse Xmas tree style fixes
- Split out unrelated code fix to a new patch

Changes in v2:
- Dropped unused variables desc-idx and flow
- Link to v1: https://lore.kernel.org/r/20241029-am65-cpsw-multi-rx-j7-fix-v1-1-426ca805918c@kernel.org

---
Roger Quadros (2):
      net: ethernet: ti: am65-cpsw: Fix multi queue Rx on J7
      net: ethernet: ti: am65-cpsw: fix warning in am65_cpsw_nuss_remove_rx_chns()

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 75 ++++++++++++++------------------
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  6 ++-
 2 files changed, 37 insertions(+), 44 deletions(-)
---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241023-am65-cpsw-multi-rx-j7-fix-f9a2149be6dd

Best regards,
-- 
Roger Quadros <rogerq@kernel.org>

[PATCH net v3 1/2] net: ethernet: ti: am65-cpsw: Fix multi queue Rx on J7

[Roger Quadros]

On J7 platforms, setting up multiple RX flows was failing
as the RX free descriptor ring 0 is shared among all flows
and we did not allocate enough elements in the RX free descriptor
ring 0 to accommodate for all RX flows.

This issue is not present on AM62 as separate pair of
rings are used for free and completion rings for each flow.

Fix this by allocating enough elements for RX free descriptor
ring 0.

However, we can no longer rely on desc_idx (descriptor based
offsets) to identify the pages in the respective flows as
free descriptor ring includes elements for all flows.
To solve this, introduce a new swdata data structure to store
flow_id and page. This can be used to identify which flow (page_pool)
and page the descriptor belonged to when popped out of the
RX rings.

Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 73 +++++++++++++-------------------
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  6 ++-
 2 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 0520e9f4bea7..70aea654c79f 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -337,9 +337,9 @@ static int am65_cpsw_nuss_rx_push(struct am65_cpsw_common *common,
 	struct am65_cpsw_rx_chn *rx_chn = &common->rx_chns;
 	struct cppi5_host_desc_t *desc_rx;
 	struct device *dev = common->dev;
+	struct am65_cpsw_swdata *swdata;
 	dma_addr_t desc_dma;
 	dma_addr_t buf_dma;
-	void *swdata;
 
 	desc_rx = k3_cppi_desc_pool_alloc(rx_chn->desc_pool);
 	if (!desc_rx) {
@@ -363,7 +363,8 @@ static int am65_cpsw_nuss_rx_push(struct am65_cpsw_common *common,
 	cppi5_hdesc_attach_buf(desc_rx, buf_dma, AM65_CPSW_MAX_PACKET_SIZE,
 			       buf_dma, AM65_CPSW_MAX_PACKET_SIZE);
 	swdata = cppi5_hdesc_get_swdata(desc_rx);
-	*((void **)swdata) = page_address(page);
+	swdata->page = page;
+	swdata->flow_id = flow_idx;
 
 	return k3_udma_glue_push_rx_chn(rx_chn->rx_chn, flow_idx,
 					desc_rx, desc_dma);
@@ -519,36 +520,31 @@ static enum am65_cpsw_tx_buf_type am65_cpsw_nuss_buf_type(struct am65_cpsw_tx_ch
 
 static inline void am65_cpsw_put_page(struct am65_cpsw_rx_flow *flow,
 				      struct page *page,
-				      bool allow_direct,
-				      int desc_idx)
+				      bool allow_direct)
 {
 	page_pool_put_full_page(flow->page_pool, page, allow_direct);
-	flow->pages[desc_idx] = NULL;
 }
 
 static void am65_cpsw_nuss_rx_cleanup(void *data, dma_addr_t desc_dma)
 {
-	struct am65_cpsw_rx_flow *flow = data;
+	struct am65_cpsw_rx_chn *rx_chn = data;
 	struct cppi5_host_desc_t *desc_rx;
-	struct am65_cpsw_rx_chn *rx_chn;
+	struct am65_cpsw_swdata *swdata;
 	dma_addr_t buf_dma;
+	struct page *page;
 	u32 buf_dma_len;
-	void *page_addr;
-	void **swdata;
-	int desc_idx;
+	u32 flow_id;
 
-	rx_chn = &flow->common->rx_chns;
 	desc_rx = k3_cppi_desc_pool_dma2virt(rx_chn->desc_pool, desc_dma);
 	swdata = cppi5_hdesc_get_swdata(desc_rx);
-	page_addr = *swdata;
+	page = swdata->page;
+	flow_id = swdata->flow_id;
 	cppi5_hdesc_get_obuf(desc_rx, &buf_dma, &buf_dma_len);
 	k3_udma_glue_rx_cppi5_to_dma_addr(rx_chn->rx_chn, &buf_dma);
 	dma_unmap_single(rx_chn->dma_dev, buf_dma, buf_dma_len, DMA_FROM_DEVICE);
 	k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx);
 
-	desc_idx = am65_cpsw_nuss_desc_idx(rx_chn->desc_pool, desc_rx,
-					   rx_chn->dsize_log2);
-	am65_cpsw_put_page(flow, virt_to_page(page_addr), false, desc_idx);
+	am65_cpsw_put_page(&rx_chn->flows[flow_id], page, false);
 }
 
 static void am65_cpsw_nuss_xmit_free(struct am65_cpsw_tx_chn *tx_chn,
@@ -703,14 +699,13 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 				ret = -ENOMEM;
 				goto fail_rx;
 			}
-			flow->pages[i] = page;
 
 			ret = am65_cpsw_nuss_rx_push(common, page, flow_idx);
 			if (ret < 0) {
 				dev_err(common->dev,
 					"cannot submit page to rx channel flow %d, error %d\n",
 					flow_idx, ret);
-				am65_cpsw_put_page(flow, page, false, i);
+				am65_cpsw_put_page(flow, page, false);
 				goto fail_rx;
 			}
 		}
@@ -764,8 +759,8 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
 
 fail_rx:
 	for (i = 0; i < common->rx_ch_num_flows; i++)
-		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, &rx_chn->flows[i],
-					  am65_cpsw_nuss_rx_cleanup, 0);
+		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
+					  am65_cpsw_nuss_rx_cleanup, !!i);
 
 	am65_cpsw_destroy_xdp_rxqs(common);
 
@@ -817,11 +812,11 @@ static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
 			dev_err(common->dev, "rx teardown timeout\n");
 	}
 
-	for (i = 0; i < common->rx_ch_num_flows; i++) {
+	for (i = common->rx_ch_num_flows - 1; i >= 0; i--) {
 		napi_disable(&rx_chn->flows[i].napi_rx);
 		hrtimer_cancel(&rx_chn->flows[i].rx_hrtimer);
-		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, &rx_chn->flows[i],
-					  am65_cpsw_nuss_rx_cleanup, 0);
+		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
+					  am65_cpsw_nuss_rx_cleanup, !!i);
 	}
 
 	k3_udma_glue_disable_rx_chn(rx_chn->rx_chn);
@@ -1028,7 +1023,7 @@ static int am65_cpsw_xdp_tx_frame(struct net_device *ndev,
 static int am65_cpsw_run_xdp(struct am65_cpsw_rx_flow *flow,
 			     struct am65_cpsw_port *port,
 			     struct xdp_buff *xdp,
-			     int desc_idx, int cpu, int *len)
+			     int cpu, int *len)
 {
 	struct am65_cpsw_common *common = flow->common;
 	struct am65_cpsw_ndev_priv *ndev_priv;
@@ -1101,7 +1096,7 @@ static int am65_cpsw_run_xdp(struct am65_cpsw_rx_flow *flow,
 	}
 
 	page = virt_to_head_page(xdp->data);
-	am65_cpsw_put_page(flow, page, true, desc_idx);
+	am65_cpsw_put_page(flow, page, true);
 
 out:
 	return ret;
@@ -1150,16 +1145,16 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
 	struct am65_cpsw_ndev_stats *stats;
 	struct cppi5_host_desc_t *desc_rx;
 	struct device *dev = common->dev;
+	struct am65_cpsw_swdata *swdata;
 	struct page *page, *new_page;
 	dma_addr_t desc_dma, buf_dma;
 	struct am65_cpsw_port *port;
-	int headroom, desc_idx, ret;
 	struct net_device *ndev;
 	u32 flow_idx = flow->id;
 	struct sk_buff *skb;
 	struct xdp_buff	xdp;
+	int headroom, ret;
 	void *page_addr;
-	void **swdata;
 	u32 *psdata;
 
 	*xdp_state = AM65_CPSW_XDP_PASS;
@@ -1182,8 +1177,8 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
 		__func__, flow_idx, &desc_dma);
 
 	swdata = cppi5_hdesc_get_swdata(desc_rx);
-	page_addr = *swdata;
-	page = virt_to_page(page_addr);
+	page = swdata->page;
+	page_addr = page_address(page);
 	cppi5_hdesc_get_obuf(desc_rx, &buf_dma, &buf_dma_len);
 	k3_udma_glue_rx_cppi5_to_dma_addr(rx_chn->rx_chn, &buf_dma);
 	pkt_len = cppi5_hdesc_get_pktlen(desc_rx);
@@ -1199,9 +1194,6 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
 
 	k3_cppi_desc_pool_free(rx_chn->desc_pool, desc_rx);
 
-	desc_idx = am65_cpsw_nuss_desc_idx(rx_chn->desc_pool, desc_rx,
-					   rx_chn->dsize_log2);
-
 	skb = am65_cpsw_build_skb(page_addr, ndev,
 				  AM65_CPSW_MAX_PACKET_SIZE);
 	if (unlikely(!skb)) {
@@ -1213,7 +1205,7 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
 		xdp_init_buff(&xdp, PAGE_SIZE, &port->xdp_rxq[flow->id]);
 		xdp_prepare_buff(&xdp, page_addr, AM65_CPSW_HEADROOM,
 				 pkt_len, false);
-		*xdp_state = am65_cpsw_run_xdp(flow, port, &xdp, desc_idx,
+		*xdp_state = am65_cpsw_run_xdp(flow, port, &xdp,
 					       cpu, &pkt_len);
 		if (*xdp_state != AM65_CPSW_XDP_PASS)
 			goto allocate;
@@ -1247,10 +1239,8 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
 		return -ENOMEM;
 	}
 
-	flow->pages[desc_idx] = new_page;
-
 	if (netif_dormant(ndev)) {
-		am65_cpsw_put_page(flow, new_page, true, desc_idx);
+		am65_cpsw_put_page(flow, new_page, true);
 		ndev->stats.rx_dropped++;
 		return 0;
 	}
@@ -1258,7 +1248,7 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_rx_flow *flow,
 requeue:
 	ret = am65_cpsw_nuss_rx_push(common, new_page, flow_idx);
 	if (WARN_ON(ret < 0)) {
-		am65_cpsw_put_page(flow, new_page, true, desc_idx);
+		am65_cpsw_put_page(flow, new_page, true);
 		ndev->stats.rx_errors++;
 		ndev->stats.rx_dropped++;
 	}
@@ -2402,10 +2392,6 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 	for (i = 0; i < common->rx_ch_num_flows; i++) {
 		flow = &rx_chn->flows[i];
 		flow->page_pool = NULL;
-		flow->pages = devm_kcalloc(dev, AM65_CPSW_MAX_RX_DESC,
-					   sizeof(*flow->pages), GFP_KERNEL);
-		if (!flow->pages)
-			return -ENOMEM;
 	}
 
 	rx_chn->rx_chn = k3_udma_glue_request_rx_chn(dev, "rx", &rx_cfg);
@@ -2458,7 +2444,8 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 
 		rx_flow_cfg.ring_rxfdq0_id = fdqring_id;
 		rx_flow_cfg.rx_cfg.size = max_desc_num;
-		rx_flow_cfg.rxfdq_cfg.size = max_desc_num;
+		/* share same FDQ for all flows */
+		rx_flow_cfg.rxfdq_cfg.size = max_desc_num * rx_cfg.flow_id_num;
 		rx_flow_cfg.rxfdq_cfg.mode = common->pdata.fdqring_mode;
 
 		ret = k3_udma_glue_rx_flow_init(rx_chn->rx_chn,
@@ -3349,8 +3336,8 @@ static int am65_cpsw_nuss_register_ndevs(struct am65_cpsw_common *common)
 
 	for (i = 0; i < common->rx_ch_num_flows; i++)
 		k3_udma_glue_reset_rx_chn(rx_chan->rx_chn, i,
-					  &rx_chan->flows[i],
-					  am65_cpsw_nuss_rx_cleanup, 0);
+					  rx_chan,
+					  am65_cpsw_nuss_rx_cleanup, !!i);
 
 	k3_udma_glue_disable_rx_chn(rx_chan->rx_chn);
 
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index dc8d544230dc..92a27ba4c601 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -101,10 +101,14 @@ struct am65_cpsw_rx_flow {
 	struct hrtimer rx_hrtimer;
 	unsigned long rx_pace_timeout;
 	struct page_pool *page_pool;
-	struct page **pages;
 	char name[32];
 };
 
+struct am65_cpsw_swdata {
+	u32 flow_id;
+	struct page *page;
+};
+
 struct am65_cpsw_rx_chn {
 	struct device *dev;
 	struct device *dma_dev;

-- 
2.34.1

Re: [PATCH net v3 1/2] net: ethernet: ti: am65-cpsw: Fix multi queue Rx on J7

[Simon Horman]

On Fri, Nov 01, 2024 at 12:18:50PM +0200, Roger Quadros wrote:
> On J7 platforms, setting up multiple RX flows was failing
> as the RX free descriptor ring 0 is shared among all flows
> and we did not allocate enough elements in the RX free descriptor
> ring 0 to accommodate for all RX flows.
> 
> This issue is not present on AM62 as separate pair of
> rings are used for free and completion rings for each flow.
> 
> Fix this by allocating enough elements for RX free descriptor
> ring 0.
> 
> However, we can no longer rely on desc_idx (descriptor based
> offsets) to identify the pages in the respective flows as
> free descriptor ring includes elements for all flows.
> To solve this, introduce a new swdata data structure to store
> flow_id and page. This can be used to identify which flow (page_pool)
> and page the descriptor belonged to when popped out of the
> RX rings.
> 
> Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")
> Signed-off-by: Roger Quadros <rogerq@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

> @@ -764,8 +759,8 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>  
>  fail_rx:
>  	for (i = 0; i < common->rx_ch_num_flows; i++)
> -		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, &rx_chn->flows[i],
> -					  am65_cpsw_nuss_rx_cleanup, 0);
> +		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
> +					  am65_cpsw_nuss_rx_cleanup, !!i);

Hi Roger,

I wonder if, as a follow-up, the skip_fdq (last) parameter of
k3_udma_glue_reset_rx_chn() can be dropped. It seems that all callers
follow the pattern above of passing i as the flow_num (2nd) argument,
and !!i as the skip_fdq argument. If so, k3_udma_glue_reset_rx_chn could
simply derive skip_fdq as !!flow_num.

Re: [PATCH net v3 1/2] net: ethernet: ti: am65-cpsw: Fix multi queue Rx on J7

[Roger Quadros]

Hi Simon,

On 05/11/2024 15:21, Simon Horman wrote:
> On Fri, Nov 01, 2024 at 12:18:50PM +0200, Roger Quadros wrote:
>> On J7 platforms, setting up multiple RX flows was failing
>> as the RX free descriptor ring 0 is shared among all flows
>> and we did not allocate enough elements in the RX free descriptor
>> ring 0 to accommodate for all RX flows.
>>
>> This issue is not present on AM62 as separate pair of
>> rings are used for free and completion rings for each flow.
>>
>> Fix this by allocating enough elements for RX free descriptor
>> ring 0.
>>
>> However, we can no longer rely on desc_idx (descriptor based
>> offsets) to identify the pages in the respective flows as
>> free descriptor ring includes elements for all flows.
>> To solve this, introduce a new swdata data structure to store
>> flow_id and page. This can be used to identify which flow (page_pool)
>> and page the descriptor belonged to when popped out of the
>> RX rings.
>>
>> Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
>> @@ -764,8 +759,8 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  
>>  fail_rx:
>>  	for (i = 0; i < common->rx_ch_num_flows; i++)
>> -		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, &rx_chn->flows[i],
>> -					  am65_cpsw_nuss_rx_cleanup, 0);
>> +		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
>> +					  am65_cpsw_nuss_rx_cleanup, !!i);
> 
> Hi Roger,
> 
> I wonder if, as a follow-up, the skip_fdq (last) parameter of
> k3_udma_glue_reset_rx_chn() can be dropped. It seems that all callers
> follow the pattern above of passing i as the flow_num (2nd) argument,
> and !!i as the skip_fdq argument. If so, k3_udma_glue_reset_rx_chn could
> simply derive skip_fdq as !!flow_num.

Added Peter to the discussion.

My understanding is that this is not always the case as some users can still
decide to use separate free descriptor rings per flow. i.e. K3_RINGACC_RING_SHARED
flag not set in rxfdq_cfg.

But we could infer that bit from the flow configuration that is passed into
k3_udma_glue_rx_flow_init() and save this information into k3_udma_glue_rx_channel.

This could then be used internally and we drop skip_fdq argument.

Peter, do you agree?

-- 
cheers,
-roger

[PATCH net v3 2/2] net: ethernet: ti: am65-cpsw: fix warning in am65_cpsw_nuss_remove_rx_chns()

[Roger Quadros]

flow->irq is initialized to 0 which is a valid IRQ. Set it to -EINVAL
in error path of am65_cpsw_nuss_init_rx_chns() so we do not try
to free an unallocated IRQ in am65_cpsw_nuss_remove_rx_chns().

If user tried to change number of RX queues and am65_cpsw_nuss_init_rx_chns()
failed due to any reason, the warning will happen if user tries to change
the number of RX queues after the error condition.

root@am62xx-evm:~# ethtool -L eth0 rx 3
[   40.385293] am65-cpsw-nuss 8000000.ethernet: set new flow-id-base 19
[   40.393211] am65-cpsw-nuss 8000000.ethernet: Failed to init rx flow2
netlink error: Invalid argument
root@am62xx-evm:~# ethtool -L eth0 rx 2
[   82.306427] ------------[ cut here ]------------
[   82.311075] WARNING: CPU: 0 PID: 378 at kernel/irq/devres.c:144 devm_free_irq+0x84/0x90
[   82.469770] Call trace:
[   82.472208]  devm_free_irq+0x84/0x90
[   82.475777]  am65_cpsw_nuss_remove_rx_chns+0x6c/0xac [ti_am65_cpsw_nuss]
[   82.482487]  am65_cpsw_nuss_update_tx_rx_chns+0x2c/0x9c [ti_am65_cpsw_nuss]
[   82.489442]  am65_cpsw_set_channels+0x30/0x4c [ti_am65_cpsw_nuss]
[   82.495531]  ethnl_set_channels+0x224/0x2dc
[   82.499713]  ethnl_default_set_doit+0xb8/0x1b8
[   82.504149]  genl_family_rcv_msg_doit+0xc0/0x124
[   82.508757]  genl_rcv_msg+0x1f0/0x284
[   82.512409]  netlink_rcv_skb+0x58/0x130
[   82.516239]  genl_rcv+0x38/0x50
[   82.519374]  netlink_unicast+0x1d0/0x2b0
[   82.523289]  netlink_sendmsg+0x180/0x3c4
[   82.527205]  __sys_sendto+0xe4/0x158
[   82.530779]  __arm64_sys_sendto+0x28/0x38
[   82.534782]  invoke_syscall+0x44/0x100
[   82.538526]  el0_svc_common.constprop.0+0xc0/0xe0
[   82.543221]  do_el0_svc+0x1c/0x28
[   82.546528]  el0_svc+0x28/0x98
[   82.549578]  el0t_64_sync_handler+0xc0/0xc4
[   82.553752]  el0t_64_sync+0x190/0x194
[   82.557407] ---[ end trace 0000000000000000 ]---

Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")
Signed-off-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 70aea654c79f..ba6db61dd227 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2441,6 +2441,7 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 		flow = &rx_chn->flows[i];
 		flow->id = i;
 		flow->common = common;
+		flow->irq = -EINVAL;
 
 		rx_flow_cfg.ring_rxfdq0_id = fdqring_id;
 		rx_flow_cfg.rx_cfg.size = max_desc_num;
@@ -2483,6 +2484,7 @@ static int am65_cpsw_nuss_init_rx_chns(struct am65_cpsw_common *common)
 		if (ret) {
 			dev_err(dev, "failure requesting rx %d irq %u, %d\n",
 				i, flow->irq, ret);
+			flow->irq = -EINVAL;
 			goto err;
 		}
 	}

-- 
2.34.1

Re: [PATCH net v3 2/2] net: ethernet: ti: am65-cpsw: fix warning in am65_cpsw_nuss_remove_rx_chns()

[Simon Horman]

On Fri, Nov 01, 2024 at 12:18:51PM +0200, Roger Quadros wrote:
> flow->irq is initialized to 0 which is a valid IRQ. Set it to -EINVAL
> in error path of am65_cpsw_nuss_init_rx_chns() so we do not try
> to free an unallocated IRQ in am65_cpsw_nuss_remove_rx_chns().
> 
> If user tried to change number of RX queues and am65_cpsw_nuss_init_rx_chns()
> failed due to any reason, the warning will happen if user tries to change
> the number of RX queues after the error condition.
> 
> root@am62xx-evm:~# ethtool -L eth0 rx 3
> [   40.385293] am65-cpsw-nuss 8000000.ethernet: set new flow-id-base 19
> [   40.393211] am65-cpsw-nuss 8000000.ethernet: Failed to init rx flow2
> netlink error: Invalid argument
> root@am62xx-evm:~# ethtool -L eth0 rx 2
> [   82.306427] ------------[ cut here ]------------
> [   82.311075] WARNING: CPU: 0 PID: 378 at kernel/irq/devres.c:144 devm_free_irq+0x84/0x90
> [   82.469770] Call trace:
> [   82.472208]  devm_free_irq+0x84/0x90
> [   82.475777]  am65_cpsw_nuss_remove_rx_chns+0x6c/0xac [ti_am65_cpsw_nuss]
> [   82.482487]  am65_cpsw_nuss_update_tx_rx_chns+0x2c/0x9c [ti_am65_cpsw_nuss]
> [   82.489442]  am65_cpsw_set_channels+0x30/0x4c [ti_am65_cpsw_nuss]
> [   82.495531]  ethnl_set_channels+0x224/0x2dc
> [   82.499713]  ethnl_default_set_doit+0xb8/0x1b8
> [   82.504149]  genl_family_rcv_msg_doit+0xc0/0x124
> [   82.508757]  genl_rcv_msg+0x1f0/0x284
> [   82.512409]  netlink_rcv_skb+0x58/0x130
> [   82.516239]  genl_rcv+0x38/0x50
> [   82.519374]  netlink_unicast+0x1d0/0x2b0
> [   82.523289]  netlink_sendmsg+0x180/0x3c4
> [   82.527205]  __sys_sendto+0xe4/0x158
> [   82.530779]  __arm64_sys_sendto+0x28/0x38
> [   82.534782]  invoke_syscall+0x44/0x100
> [   82.538526]  el0_svc_common.constprop.0+0xc0/0xe0
> [   82.543221]  do_el0_svc+0x1c/0x28
> [   82.546528]  el0_svc+0x28/0x98
> [   82.549578]  el0t_64_sync_handler+0xc0/0xc4
> [   82.553752]  el0t_64_sync+0x190/0x194
> [   82.557407] ---[ end trace 0000000000000000 ]---
> 
> Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")

Hi Roger,

I wonder if the problem predates the cited commit and was, rather,
introduced by:

Fixes: 24bc19b05f1f ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")

...

Re: [PATCH net v3 2/2] net: ethernet: ti: am65-cpsw: fix warning in am65_cpsw_nuss_remove_rx_chns()

[Roger Quadros]

Hi Simon,

On 05/11/2024 15:30, Simon Horman wrote:
> On Fri, Nov 01, 2024 at 12:18:51PM +0200, Roger Quadros wrote:
>> flow->irq is initialized to 0 which is a valid IRQ. Set it to -EINVAL
>> in error path of am65_cpsw_nuss_init_rx_chns() so we do not try
>> to free an unallocated IRQ in am65_cpsw_nuss_remove_rx_chns().
>>
>> If user tried to change number of RX queues and am65_cpsw_nuss_init_rx_chns()
>> failed due to any reason, the warning will happen if user tries to change
>> the number of RX queues after the error condition.
>>
>> root@am62xx-evm:~# ethtool -L eth0 rx 3
>> [   40.385293] am65-cpsw-nuss 8000000.ethernet: set new flow-id-base 19
>> [   40.393211] am65-cpsw-nuss 8000000.ethernet: Failed to init rx flow2
>> netlink error: Invalid argument
>> root@am62xx-evm:~# ethtool -L eth0 rx 2
>> [   82.306427] ------------[ cut here ]------------yes.
>> [   82.311075] WARNING: CPU: 0 PID: 378 at kernel/irq/devres.c:144 devm_free_irq+0x84/0x90
>> [   82.469770] Call trace:
>> [   82.472208]  devm_free_irq+0x84/0x90
>> [   82.475777]  am65_cpsw_nuss_remove_rx_chns+0x6c/0xac [ti_am65_cpsw_nuss]
>> [   82.482487]  am65_cpsw_nuss_update_tx_rx_chns+0x2c/0x9c [ti_am65_cpsw_nuss]
>> [   82.489442]  am65_cpsw_set_channels+0x30/0x4c [ti_am65_cpsw_nuss]
>> [   82.495531]  ethnl_set_channels+0x224/0x2dc
>> [   82.499713]  ethnl_default_set_doit+0xb8/0x1b8
>> [   82.504149]  genl_family_rcv_msg_doit+0xc0/0x124
>> [   82.508757]  genl_rcv_msg+0x1f0/0x284
>> [   82.512409]  netlink_rcv_skb+0x58/0x130
>> [   82.516239]  genl_rcv+0x38/0x50
>> [   82.519374]  netlink_unicast+0x1d0/0x2b0
>> [   82.523289]  netlink_sendmsg+0x180/0x3c4
>> [   82.527205]  __sys_sendto+0xe4/0x158
>> [   82.530779]  __arm64_sys_sendto+0x28/0x38
>> [   82.534782]  invoke_syscall+0x44/0x100
>> [   82.538526]  el0_svc_common.constprop.0+0xc0/0xe0
>> [   82.543221]  do_el0_svc+0x1c/0x28
>> [   82.546528]  el0_svc+0x28/0x98
>> [   82.549578]  el0t_64_sync_handler+0xc0/0xc4
>> [   82.553752]  el0t_64_sync+0x190/0x194
>> [   82.557407] ---[ end trace 0000000000000000 ]---
>>
>> Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")
> 
> Hi Roger,
> 
> I wonder if the problem predates the cited commit and was, rather,
> introduced by:
> 
> Fixes: 24bc19b05f1f ("net: ethernet: ti: am65-cpsw: Add suspend/resume support")

Partly yes. But at that commit AM65_CPSW_MAX_RX_FLOWS is 1 so there is no
opportunity for user to change the number of RX queues.

-- 
cheers,
-roger

Re: [PATCH net v3 0/2] net: ethernet: ti: am65-cpsw: Fixes to multi queue RX feature

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 01 Nov 2024 12:18:49 +0200 you wrote:
> On J7 platforms, setting up multiple RX flows was failing
> as the RX free descriptor ring 0 is shared among all flows
> and we did not allocate enough elements in the RX free descriptor
> ring 0 to accommodate for all RX flows. Patch 1 fixes this.
> 
> The second patch fixes a warning if there was any error in
> am65_cpsw_nuss_init_rx_chns() and am65_cpsw_nuss_cleanup_rx_chns()
> was called after that.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] net: ethernet: ti: am65-cpsw: Fix multi queue Rx on J7
    https://git.kernel.org/netdev/net/c/de794169cf17
  - [net,v3,2/2] net: ethernet: ti: am65-cpsw: fix warning in am65_cpsw_nuss_remove_rx_chns()
    https://git.kernel.org/netdev/net/c/ba3b7ac4f714

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html