[PATCH net v3 1/2] net: sparx5: fix RX ring stall on frames from inactive ports

[PATCH net v3 1/2] net: sparx5: fix RX ring stall on frames from inactive ports

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

When the FDMA RX path receives a frame whose source port does not map
to an active netdev, sparx5_fdma_rx_get_frame() logs an error, flushes
the extraction FIFO and returns false - without advancing the FDMA
software cursor and without releasing the skb, which has already been
extended with skb_put().

The DCB for this frame still carries its hardware DONE bit, so the next
NAPI poll picks up the same DCB, takes the same inactive-port branch and
returns false again. The software cursor never moves past this DCB, so
RX traffic on all ports stalls until the module is reloaded.

Treat a bogus frame as consumed: free the skb, clear its slot in the
rx->skb[][] array and return true, so the NAPI loop advances past the
DB the same way it does for any other received frame. The slot is
refilled by sparx5_fdma_rx_dataptr_cb() when the DCB is recycled. The
extraction FIFO flush is dropped since the frame is already consumed
into the DB, and the error is ratelimited.

Found by code inspection; the stall needs the hardware to emit a frame
with an invalid source port, which cannot be triggered without hardware.

Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 15ec9b0..e35ef0e 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -157,9 +157,11 @@ static bool sparx5_fdma_rx_get_frame(struct sparx5 *sparx5, struct sparx5_rx *rx
 		       sparx5->ports[fi.src_port] :
 		       NULL;
 	if (!port || !port->ndev) {
-		dev_err(sparx5->dev, "Data on inactive port %d\n", fi.src_port);
-		sparx5_xtr_flush(sparx5, XTR_QUEUE);
-		return false;
+		dev_err_ratelimited(sparx5->dev, "Data on inactive port %d\n",
+				    fi.src_port);
+		dev_kfree_skb_any(skb);
+		rx->skb[fdma->dcb_index][fdma->db_index] = NULL;
+		return true;
 	}
 	skb->dev = port->ndev;
 	skb_pull(skb, IFH_LEN * sizeof(u32));
-- 
2.39.5 (Apple Git-154)

[PATCH net v3 2/2] net: lan969x: fix RX ring stall on frames from inactive ports

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

When a received frame's source port does not map to an active netdev,
lan969x_fdma_rx_get_frame() recycles the page via the page pool and
returns NULL, which makes lan969x_fdma_napi_poll() break out of the RX
loop without advancing the FDMA software cursor.

As on sparx5, the DCB still carries its hardware DONE bit, so every
following NAPI poll picks the same DCB, hits the same inactive-port
branch and breaks again: RX traffic stalls on all ports until the
module is reloaded.

A NULL return currently covers two different situations: a failed
build_skb() (allocation failure - breaking out and retrying the same
DCB later is correct) and a bogus frame (which must be dropped and
skipped). Split the two: recycle the page, log a ratelimited error and
return ERR_PTR(-EINVAL) for a bogus frame, and advance past such a DB
in the poll loop. The WARN_ON() is also dropped as the backtrace slows
RX to a crawl on every bad frame.

Found by code inspection; the stall needs the hardware to emit a frame
with an invalid source port, which cannot be triggered without hardware.

Fixes: d84ad2c0d80c ("net: lan969x: add FDMA implementation")
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 .../microchip/sparx5/lan969x/lan969x_fdma.c     | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
index 053d125..5f5adb7 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
@@ -113,8 +113,12 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 	sparx5_ifh_parse(sparx5, page_address(page), &fi);
 	port = fi.src_port < consts->n_ports ? sparx5->ports[fi.src_port] :
 					       NULL;
-	if (WARN_ON(!port))
-		goto free_page;
+	if (!port) {
+		dev_err_ratelimited(sparx5->dev, "Data on inactive port %d\n",
+				    fi.src_port);
+		page_pool_recycle_direct(rx->page_pool, page);
+		return ERR_PTR(-EINVAL);
+	}
 
 	skb = build_skb(page_address(page), fdma->db_size);
 	if (unlikely(!skb))
@@ -258,6 +262,15 @@ int lan969x_fdma_napi_poll(struct napi_struct *napi, int weight)
 			break;
 
 		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
+		if (IS_ERR(skb)) {
+			fdma_db_advance(fdma);
+			counter++;
+			if (fdma_dcb_is_reusable(fdma))
+				continue;
+			fdma_db_reset(fdma);
+			fdma_dcb_advance(fdma);
+			continue;
+		}
 		if (!skb)
 			break;
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v3 2/2] net: lan969x: fix RX ring stall on frames from inactive ports

[Daniel Machon]

Hi,

> From: Zhang Hewei <memoriaru@foxmail.com>
> 
> When a received frame's source port does not map to an active netdev,
> lan969x_fdma_rx_get_frame() recycles the page via the page pool and
> returns NULL, which makes lan969x_fdma_napi_poll() break out of the RX
> loop without advancing the FDMA software cursor.
> 
> As on sparx5, the DCB still carries its hardware DONE bit, so every
> following NAPI poll picks the same DCB, hits the same inactive-port
> branch and breaks again: RX traffic stalls on all ports until the
> module is reloaded.
> 
> A NULL return currently covers two different situations: a failed
> build_skb() (allocation failure - breaking out and retrying the same
> DCB later is correct) and a bogus frame (which must be dropped and
> skipped). Split the two: recycle the page, log a ratelimited error and
> return ERR_PTR(-EINVAL) for a bogus frame, and advance past such a DB
> in the poll loop. The WARN_ON() is also dropped as the backtrace slows
> RX to a crawl on every bad frame.
> 
> Found by code inspection; the stall needs the hardware to emit a frame
> with an invalid source port, which cannot be triggered without hardware.
> 
> Fixes: d84ad2c0d80c ("net: lan969x: add FDMA implementation")
> Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
> ---
>  .../microchip/sparx5/lan969x/lan969x_fdma.c     | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> index 053d125..5f5adb7 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
> @@ -113,8 +113,12 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
>  	sparx5_ifh_parse(sparx5, page_address(page), &fi);
>  	port = fi.src_port < consts->n_ports ? sparx5->ports[fi.src_port] :
>  					       NULL;
> -	if (WARN_ON(!port))
> -		goto free_page;
> +	if (!port) {
> +		dev_err_ratelimited(sparx5->dev, "Data on inactive port %d\n",
> +				    fi.src_port);
> +		page_pool_recycle_direct(rx->page_pool, page);
> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	skb = build_skb(page_address(page), fdma->db_size);
>  	if (unlikely(!skb))
> @@ -258,6 +262,15 @@ int lan969x_fdma_napi_poll(struct napi_struct *napi, int weight)
>  			break;
>  
>  		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
> +		if (IS_ERR(skb)) {
> +			fdma_db_advance(fdma);
> +			counter++;
> +			if (fdma_dcb_is_reusable(fdma))
> +				continue;
> +			fdma_db_reset(fdma);
> +			fdma_dcb_advance(fdma);
> +			continue;
> +		}

It is still a copy of the advance at the end of the loop (as pointed out in v2).
Drop it and do:

		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
		if (skb)
			napi_gro_receive(&rx->napi, skb);

and let the loop advance as it does for any other frame. Then NULL always means
"frame dropped, page recycled", which covers both the bad port and the
build_skb() failure, and the ERR_PTR() goes away. I tested exactly this on
lan969x: RX keeps working, and only the bad frames are dropped. With this, the
page_pool_recycle_direct() and ERR_PTR changes further up becomes obsolete.

>  		if (!skb)
>  			break;
>  
> -- 
> 2.39.5 (Apple Git-154)
> 

Also, please wait at least 24 hours before resposting, and populate To and CC
properly.

/Daniel

pw-bot: cr

[PATCH net v4 0/2] net: sparx5/lan969x: fix RX ring stall on frames from inactive ports

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

The FDMA RX path on both sparx5 and its lan969x sibling stalls the RX
ring forever when the hardware delivers a frame whose source port does
not map to an active netdev: the inactive-port error branch in both
drivers leaves the FDMA software cursor untouched, while the DCB keeps
its hardware DONE bit - so every following NAPI poll picks the same
DCB again and RX traffic stalls on all ports until the module is
reloaded.

Patch 1 treats a bogus frame on sparx5 as consumed (free the skb, clear
the array slot, return true). Patch 2 unifies the NULL semantics on
lan969x: NULL always means "frame dropped, page recycled" (inactive
port or failed build_skb()), and the poll loop advances past such a DB
like it does for any other frame - dropping only the bogus frame and
keeping RX alive.

v4: per Daniel's review of v3 (tested on lan969x hardware), drop the
ERR_PTR()/IS_ERR() plumbing in favor of the unified-NULL form above -
NULL always meaning "dropped and recycled" covers both the bad-port and
build_skb-failure cases, and the duplicated advance block goes away.
v1 lore link:
https://lore.kernel.org/netdev/20261108184541.74618-1-memoriaru@foxmail.com/
v3 lore link:
https://lore.kernel.org/netdev/20261109164053.8928-1-memoriaru@foxmail.com/

Found by code inspection; the stall needs the hardware to emit a frame
with an invalid source port, which cannot be triggered without hardware.
Review from someone with access to sparx5/lan969x systems would be
appreciated.


Zhang Hewei (2):
  net: sparx5: fix RX ring stall on frames from inactive ports
  net: lan969x: fix RX ring stall on frames from inactive ports

 .../microchip/sparx5/lan969x/lan969x_fdma.c        | 14 ++++++++------
 .../net/ethernet/microchip/sparx5/sparx5_fdma.c    |  8 +++++---
 2 files changed, 13 insertions(+), 9 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH net v4 1/2] net: sparx5: fix RX ring stall on frames from inactive ports

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

When the FDMA RX path receives a frame whose source port does not map
to an active netdev, sparx5_fdma_rx_get_frame() logs an error, flushes
the extraction FIFO and returns false - without advancing the FDMA
software cursor and without releasing the skb, which has already been
extended with skb_put().

The DCB for this frame still carries its hardware DONE bit, so the next
NAPI poll picks up the same DCB, takes the same inactive-port branch and
returns false again. The software cursor never moves past this DCB, so
RX traffic on all ports stalls until the module is reloaded.

Treat a bogus frame as consumed: free the skb, clear its slot in the
rx->skb[][] array and return true, so the NAPI loop advances past the
DB the same way it does for any other received frame. The slot is
refilled by sparx5_fdma_rx_dataptr_cb() when the DCB is recycled. The
extraction FIFO flush is dropped since the frame is already consumed
into the DB, and the error is ratelimited.

Found by code inspection; the stall needs the hardware to emit a frame
with an invalid source port, which cannot be triggered without hardware.

Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index 15ec9b0..e35ef0e 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -157,9 +157,11 @@ static bool sparx5_fdma_rx_get_frame(struct sparx5 *sparx5, struct sparx5_rx *rx
 		       sparx5->ports[fi.src_port] :
 		       NULL;
 	if (!port || !port->ndev) {
-		dev_err(sparx5->dev, "Data on inactive port %d\n", fi.src_port);
-		sparx5_xtr_flush(sparx5, XTR_QUEUE);
-		return false;
+		dev_err_ratelimited(sparx5->dev, "Data on inactive port %d\n",
+				    fi.src_port);
+		dev_kfree_skb_any(skb);
+		rx->skb[fdma->dcb_index][fdma->db_index] = NULL;
+		return true;
 	}
 	skb->dev = port->ndev;
 	skb_pull(skb, IFH_LEN * sizeof(u32));
-- 
2.39.5 (Apple Git-154)

[PATCH net v4 2/2] net: lan969x: fix RX ring stall on frames from inactive ports

[memoriaru]

From: Zhang Hewei <memoriaru@foxmail.com>

When a received frame's source port does not map to an active netdev,
lan969x_fdma_rx_get_frame() recycles the page via the page pool and
returns NULL, which makes lan969x_fdma_napi_poll() break out of the RX
loop without advancing the FDMA software cursor.

As on sparx5, the DCB still carries its hardware DONE bit, so every
following NAPI poll picks the same DCB, hits the same inactive-port
branch and breaks again: RX traffic stalls on all ports until the
module is reloaded.

Unify the NULL semantics: NULL always means "frame dropped, page
recycled", covering both the inactive-port case and a failed
build_skb(). The poll loop then advances past such a DB like it does
for any other frame, dropping only the bogus frame and keeping RX
alive. The WARN_ON() backtrace is also gone, which previously slowed
RX to a crawl on every bad frame.

Found by code inspection; the stall needs the hardware to emit a frame
with an invalid source port, which cannot be triggered without
hardware. The drop-and-advance form was tested on lan969x hardware.

Fixes: d84ad2c0d80c ("net: lan969x: add FDMA implementation")
Signed-off-by: Zhang Hewei <memoriaru@foxmail.com>
---
 .../microchip/sparx5/lan969x/lan969x_fdma.c        | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
index 053d125..4181a9d 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c
@@ -113,8 +113,12 @@ static struct sk_buff *lan969x_fdma_rx_get_frame(struct sparx5 *sparx5,
 	sparx5_ifh_parse(sparx5, page_address(page), &fi);
 	port = fi.src_port < consts->n_ports ? sparx5->ports[fi.src_port] :
 					       NULL;
-	if (WARN_ON(!port))
-		goto free_page;
+	if (!port) {
+		dev_err_ratelimited(sparx5->dev, "Data on inactive port %d\n",
+				    fi.src_port);
+		page_pool_recycle_direct(rx->page_pool, page);
+		return NULL;
+	}
 
 	skb = build_skb(page_address(page), fdma->db_size);
 	if (unlikely(!skb))
@@ -258,10 +262,8 @@ int lan969x_fdma_napi_poll(struct napi_struct *napi, int weight)
 			break;
 
 		skb = lan969x_fdma_rx_get_frame(sparx5, rx);
-		if (!skb)
-			break;
-
-		napi_gro_receive(&rx->napi, skb);
+		if (skb)
+			napi_gro_receive(&rx->napi, skb);
 
 		fdma_db_advance(fdma);
 		counter++;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v4 1/2] net: sparx5: fix RX ring stall on frames from inactive ports

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.