[PATCH net] net: macb: limit jumbo frames to the RX buffer size

[PATCH net] net: macb: limit jumbo frames to the RX buffer size

[Han Gao via B4 Relay]

From: Han Gao <gaohan@iscas.ac.cn>

GEM variants with MACB_CAPS_JUMBO accept frames up to jumbo_max_len
regardless of the MTU, but the RX buffers are sized from the MTU. A
frame longer than one RX buffer is split over several descriptors and
dropped by gem_rx() with "not whole frame pointed by descriptor".

Set JML from the RX buffer size in macb_configure_dma(), so that it
follows MTU changes and longer frames are dropped by the MAC instead.

Fixes: 98b5a0f4a228 ("net: macb: Add support for jumbo frames")
Reported-by: Jingkun Zheng <zhengjingkun@iscas.ac.cn>
Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
---
Seen on a SiFive FU740 with the default MTU: every jumbo frame on the
network caused two "not whole frame pointed by descriptor" errors.
---
 drivers/net/ethernet/cadence/macb_main.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 5ad0e32e2b4001c37e230a2eb7badb468b14ab57..e7c9bb24c377babc8dfd57e82ebf55a8e996873d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3240,6 +3240,18 @@ static u32 macb_dbw(struct macb *bp)
 	}
 }
 
+/* Longest frame that fits in a single RX buffer */
+static u32 gem_jumbo_max_len(struct macb *bp)
+{
+	u32 len = bp->ctx->rx_buffer_size;
+
+	/* Frame data starts NET_IP_ALIGN bytes into the buffer without RSC */
+	if (!(bp->caps & MACB_CAPS_RSC))
+		len -= NET_IP_ALIGN;
+
+	return min(len, bp->jumbo_max_len);
+}
+
 /* Configure the receive DMA engine
  * - use the correct receive buffer size
  * - set best burst length for DMA operations
@@ -3286,6 +3298,13 @@ static void macb_configure_dma(struct macb *bp)
 		netdev_dbg(bp->netdev, "Cadence configure DMA with 0x%08x\n",
 			   dmacfg);
 		gem_writel(bp, DMACFG, dmacfg);
+
+		/* Jumbo frames are always enabled, so limit their length to
+		 * what fits in one RX buffer. Longer frames would otherwise be
+		 * split across several descriptors and dropped by gem_rx().
+		 */
+		if ((bp->caps & MACB_CAPS_JUMBO) && bp->jumbo_max_len)
+			gem_writel(bp, JML, gem_jumbo_max_len(bp));
 	}
 }
 
@@ -3422,8 +3441,6 @@ static void macb_init_hw(struct macb *bp)
 		config |= MACB_BIT(NBC);	/* No BroadCast */
 	config |= macb_dbw(bp);
 	macb_writel(bp, NCFGR, config);
-	if ((bp->caps & MACB_CAPS_JUMBO) && bp->jumbo_max_len)
-		gem_writel(bp, JML, bp->jumbo_max_len);
 	bp->rx_frm_len_mask = MACB_RX_FRMLEN_MASK;
 	if (bp->caps & MACB_CAPS_JUMBO)
 		bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;

---
base-commit: aa40afa831ebfe45e40a23721711059f2ac584d4
change-id: 20261111-macb-jumbo-jml-49d07c0cd9d2

Best regards,
-- 
Han Gao <gaohan@iscas.ac.cn>

Re: [PATCH net] net: macb: limit jumbo frames to the RX buffer size

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/ethernet/cadence/macb_main.c,
  gem_jumbo_max_len() sets the GEM JML register to rx_buffer_size -…

--

net: macb: limit jumbo frames to the RX buffer size

This commit moves the JML register write from macb_init_hw() into
macb_configure_dma(). It now derives the JML value from the RX buffer
size rather than from jumbo_max_len. With this, the MAC drops frames that
would otherwise be split over several RX descriptors and then rejected
by gem_rx().

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5ad0e32e2b400..e7c9bb24c377b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3240,6 +3240,18 @@ static u32 macb_dbw(struct macb *bp)
>  	}
>  }
>  
> +/* Longest frame that fits in a single RX buffer */
> +static u32 gem_jumbo_max_len(struct macb *bp)
> +{
> +	u32 len = bp->ctx->rx_buffer_size;
> +
> +	/* Frame data starts NET_IP_ALIGN bytes into the buffer without RSC */
> +	if (!(bp->caps & MACB_CAPS_RSC))
> +		len -= NET_IP_ALIGN;
> +
> +	return min(len, bp->jumbo_max_len);
> +}

[Severity: Medium]
Is the value returned here 4 bytes smaller than the longest frame that
actually fits in one RX buffer?

macb_init_hw() always sets DRFCS. The MAC strips the FCS before DMA, so
the FCS never takes up space in the RX buffer:

	config |= MACB_BIT(DRFCS);		/* Discard Rx FCS */

The rest of the driver treats jumbo_max_len, and so JML, as an on-wire
length that includes the FCS. For example, macb_probe() does:

	netdev->max_mtu = MIN(bp->jumbo_max_len, RX_BUFFER_MAX) -
				ETH_HLEN - ETH_FCS_LEN;

Without RSC, that makes the largest wire frame that fits in one
descriptor rx_buffer_size - NET_IP_ALIGN + ETH_FCS_LEN. Wire frames from
rx_buffer_size - 1 to rx_buffer_size + 2 bytes fit in a single buffer
and were received before this patch. With the JML programmed by
macb_configure_dma(), the MAC now drops them.

Some concrete cases:

On pic64hpsc (jumbo_max_len = 16383), max_mtu is
MIN(16383, 16320) - 18 = 16302. macb_rx_buffer_size() returns
MIN(16302 + 20, RX_BUFFER_MAX) = 16320, so JML becomes 16318. A
full-size untagged frame at MTU 16302 is 16320 bytes on the wire, so the
MAC drops it. Yet its 16316 stored bytes plus the 2 byte offset fit in
the buffer. MTU 16301 fails the same way.

macb_rx_buffer_size() does not reserve VLAN_HLEN. At MTU 1513-1516,
rx_buffer_size is 1536 and JML is 1534. MTU-sized VLAN tagged frames
(1535-1538 bytes on the wire) are therefore dropped, even though they
fit in the buffer. MTU 9001-9004 has the same problem.

With QinQ at MTU 9000, a 9026 byte frame is larger than JML 9022 and is
dropped. Its 9022 stored bytes plus 2 fit in the 9024 byte buffer.

Should this add ETH_FCS_LEN, relying on DRFCS always being set, before
clamping to jumbo_max_len? The reported FU740 default MTU case would
still be fixed with that change.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114-macb-jumbo-jml-v1-1-f501395eaf9e%40iscas.ac.cn