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

[netdev-bot+sashiko@kernel.org]

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