Re: [PATCH net-next v4 3/3] net: ethernet: mtk_eth_soc: add 9K jumbo frame

[Sandeep Haemoon <sandeeph@loljews.com>]

On the v4 review of patch 3/3:

> [Medium] The shared RX length leaves no room for an 802.1Q tag at
> 1 KiB boundaries, including the advertised max_mtu.

Right, thank you.  v5 adds VLAN_HLEN to the length before the 1 KiB
round-up and lowers the advertised max_mtu by the same four bytes to
9194, so a tagged maximum-size frame is exactly rx_buf_len 9216 and
still at or under the XMAC 9216 cap.  XMAC_RX_CFG2 and the ring
buffers therefore cover a tagged frame at every MTU, including the
maximum.

> [Medium] GMAC1 (the built-in switch port and DSA conduit on MT7988)
> advertises max_mtu = 9198, but the only receive limit the driver
> can program caps at 2048.

You found the missing piece: the vendor driver has a second MCR
max-rx field (bits 31:28, 1024-byte units) that this port never
wrote.  v5 clears and programs MAC_MCR_MAX_RX_JUMBO for GMAC1, so the
same shared length is programmable for the switch port as for the
XGMII MACs.  The new field handling is limited to the
MTK_NETSYS_RX_9K SoCs, so every other SoC keeps its previous MCR
programming bit for bit.  Whether the internal switch path enforces
the field for switch-side frames at all remains a hardware question;
the commit message documents it honestly.

> [Medium] The unpolled QDMA RX ring is now filled with jumbo-sized
> buffers.

Right.  v5 changes mtk_max_frag_size()/mtk_max_buf_size() to take the
buffer length instead of struct mtk_eth, and mtk_rx_alloc() sizes the
never-polled QDMA RX ring at the fixed 2K length; only the polled
rings grow.  HW-LRO rings keep their 12288-byte sizing.

> [Medium] Possible kernel memory exposure (speculative).

Accepted as a robustness fix: v5 drops any descriptor whose PLEN0
exceeds ring->buf_size, with rx_length_errors, before the buffer can
be passed up, so the larger build_skb() size can no longer expose
unwritten memory even if the engine over-reports the length.

> [Medium] The PDMA RX max-frame (SDL) field may now be set 12 KiB
> above the posted buffer size (speculative).

The field is programmed the way the MediaTek downstream driver does
(frame length offset by MTK_PDMA_LRO_SDL in GENMASK(30,16)); the
local 6.18 backport drops jumbo frames without the write, and the
vendor 5.4 tree programs the plain 0x3000 LRO offset, both consistent
with the offset reading.  The encoding is the vendor's and not
publicly documented, and the comment and commit message now say
exactly that and ask for a datasheet confirmation.  No code change
in v5.

> [Medium] net: mtk_eth_soc: mtk_xdp_setup() rejects XDP detach when
> MTU is jumbo.

Right, the pool-size check now runs only when a program is attached,
so a detach always succeeds.  The shared eth->prog weakness the rest
of the analysis describes is pre-existing and is not addressed in
this series.

> [Low] The commit message hides a behaviour change on other SoCs and
> gets the buffer sizes wrong.

The message is corrected: order-1 (8 KiB) covers rx_buf_len
4096..7168 and order-2 (16 KiB) starts at rx_buf_len 8192, and it
now mentions that the build_skb()/truesize change also covers the
HW-LRO rings on mt2701, mt7622, mt7623 and mt7629.

> [High] On the first open, a failure while filling an RX ring leaks
> the buffers and frees with size 0.

Pre-existing in v7.3: ring->dma_size is set only after the fill loop
(introduced by ee40681037c0, "net: ethernet: mediatek: add HW LRO
functions of PDMA RX rings"), so this series does not create it.  It
will be sent as a separate fix that sets ring->dma_size before the
fill loop.

v5: https://lore.kernel.org/all/patch-f1e62a87eb0f914e1ea52a395f465998@loljews.com/

pw-bot: cr