Re: [PATCH net-next v3 2/2] net: ethernet: mtk_eth_soc: add 9K jumbo

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] mtk_xdp_setup() can accept an XDP program that then never runs.
- [Medium] The patch changes the GMAC MCR max-RX limit on every non-
  MT7628 SoC, not just MT7988.
- [Medium] mtk_pdma_set_sdl() writes `FIELD_PREP(GENMASK(30,16),
  MTK_PDMA_LRO_SDL + eth->rx_buf_len)` into PDMA RX_CFG (0x6a10) on…
- [Medium] Normal and QDMA RX rings now use high-order compound pages
  whenever frag_size > PAGE_SIZE: `mtk_max_buf_alloc(ring->frag_size,…
- [Medium] On MT7988, GMAC1 (built-in switch, DT phy-mode 'internal')
  counts as XGMII in mtk_interface_mode_is_xgmii(), so mtk_add_mac()…
- [Medium] In the XGMII branch, mtk_set_mcr_max_rx() writes the exact
  byte value it receives into MTK_XMAC_RX_CFG2[13:0] (`min_t(u32, val,…
- [Low] The commit message says the patch programs the PDMA RX SDL
  'replacing the field instead of OR-ing into the value already there',…
- [Low] The stated motivation is to let DSA raise the conduit MTU for 9K,
  but the -EBUSY check in mtk_change_mtu() (lines 4453-4455) tests the…
- [Low] The commit message says the patch is a forward-port of Mason
  Chang's 'change default rx buffer length' and 'add 9k jumbo frame…
- [Low] The comment above the page_pool gate in mtk_rx_alloc() (lines
  2865-2871) is now wrong in two ways.
- [Low] The comment in mtk_dma_init() (lines 3360-3364) says
  eth->rx_buf_len is 'Derived here, at ring allocation, so…

--

net: ethernet: mtk_eth_soc: add 9K jumbo

This adds a MTK_NETSYS_RX_9K capability for MT7988 and advertises a 9K
max_mtu on the XGMII MACs. The shared RX rings are sized from the largest
MTU of any MAC, and the GMAC/XMAC max-rx limits and the PDMA RX SDL are
programmed from that shared length. While DMA is running, MTU increases
that need larger buffers fail with -EBUSY, and XDP is rejected when the
buffer length is larger than MTK_PP_MAX_BUF_SIZE.

Link: https://github.com/frank-w/BPI-Router-Linux/commit/b4c6cb1d5532
Link: https://github.com/mediatek/mtk-openwrt-feeds

> - program the PDMA RX SDL max frame from the shared buffer length,
>   replacing the field instead of OR-ing into the value already there;

[Severity: Low]
Is this description accurate? In the baseline, MTK_PDMA_LRO_SDL and
MTK_RX_CFG_SDL_OFFSET are only defined in mtk_eth_soc.h. Nothing in
mtk_eth_soc.c uses them, and this patch adds the .rx_cfg reg map entry.

So there was no OR-ing to replace. The patch adds a new write to PDMA
RX_CFG through mtk_pdma_set_sdl(). It runs on every MT7988 DMA init and
on MTU changes while DMA is down, including at the default 1500 MTU.

Could the commit message say this is a new register write and not a fix
to existing behaviour?

> This is a forward-port of the MediaTek downstream jumbo work: Mason
> Chang's "change default rx buffer length" and "add 9k jumbo frame
> support" (MediaTek OpenWrt feed 999-eth-30/31, carried in
> frank-w/BPI-Router-Linux), plus Frank Wunderlich's "try to fix crash the
> right way".

[ ... ]

> Signed-off-by: Sandeep Haemoon <sandeeph@loljews.com>

[Severity: Low]
Should this carry credit tags for the original authors? The only trailer
is the submitter's Signed-off-by. There is no Co-developed-by: and
Signed-off-by: pair for Mason Chang or Frank Wunderlich, and no original
From:. Documentation/process/submitting-patches.rst asks for these when
porting someone else's work.

> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index f2b2fb1130ff9..c7bc8eaea7ab0 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c

[ ... ]

> @@ -554,6 +555,41 @@ static int mtk_mac_prepare(struct phylink_config *config, unsigned int mode,
>  	return 0;
>  }
>  
> +static void mtk_set_mcr_max_rx(struct mtk_mac *mac, u32 val)
> +{
> +	struct mtk_eth *eth = mac->hw;
> +	u32 mcr_cur, mcr_new;
> +
> +	if (MTK_HAS_CAPS(eth->soc->caps, MTK_SOC_MT7628))
> +		return;
> +
> +	if (!mtk_interface_mode_is_xgmii(eth, mac->interface)) {
> +		mcr_cur = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id));
> +		mcr_new = mcr_cur & ~MAC_MCR_MAX_RX_MASK;

[ ... ]

> +	} else if (mtk_is_netsys_v3_or_greater(mac->hw) &&
> +		   mac->id != MTK_GMAC1_ID) {

[Severity: Medium]
After this patch, does anything program an RX length limit for GMAC1 on
MT7988? GMAC1 is the built-in switch port with phy-mode internal, and
mtk_interface_mode_is_xgmii() treats PHY_INTERFACE_MODE_INTERNAL as
XGMII.

Once mtk_mac_config() sets mac->interface to INTERNAL, the MCR branch
above is skipped because the interface is XGMII. This branch is then
skipped by the mac->id != MTK_GMAC1_ID check.

Next, mtk_mac_config() does:

	if (mtk_interface_mode_is_xgmii(eth, state->interface)) {
		mtk_w32(mac->hw, MTK_GDMA_XGDM_SEL, MTK_GDMA_EG_CTRL(mac->id));
		mtk_w32(mac->hw, MAC_MCR_FORCE_LINK_DOWN, MTK_MAC_MCR(mac->id));

That leaves MAX_RX in MTK_MAC_MCR(0) at the 1518 encoding.
mtk_xgdm_mac_link_up() also returns early for GMAC1. In the baseline,
mtk_change_mtu() wrote MAX_RX into MTK_MAC_MCR(0).

At the same time, mtk_add_mac() advertises a 9K max_mtu for GMAC1,
because INTERNAL counts as XGMII.

Depending on the hardware, one of two things seems to follow. Either 9K
never works on the DSA conduit, or GMAC1 has no programmed limit at all.
In the second case, does the commit message's "no MAC accepts a frame
the rings cannot hold" hold for this MAC?

> +		mcr_cur = mtk_r32(mac->hw, MTK_XMAC_RX_CFG2(mac->id));
> +		mcr_new = mcr_cur & ~MTK_XMAC_MAX_RX_MASK;
> +		mcr_new |= FIELD_PREP(MTK_XMAC_MAX_RX_MASK,
> +				      min_t(u32, val, MTK_MAX_RX_LENGTH_9K));

[Severity: Medium]
Unlike the coarse buckets used for the GMAC MCR, this writes the exact
byte value into XMAC_RX_CFG2. mtk_change_mtu() passes:

	int length = new_mtu + MTK_RX_ETH_HLEN;

and MTK_RX_ETH_HLEN is ETH_HLEN + ETH_FCS_LEN, with no VLAN_HLEN.

Shrinking the MTU is allowed while running. After a runtime shrink to
MTU 1500 on MT7988 GMAC2, the XMAC limit becomes 1518. Would full-size
802.1Q tagged frames (1522 bytes) then be dropped by the MAC, even
though the rings hold at least 2048 bytes?

Likewise, at max_mtu the limit is capped at 9216, so tagged frames at
max_mtu could not be received either.

mtk_mac_config() also writes eth->rx_buf_len (at least 2048) into this
register on every open. The effective limit then depends on whether the
last operation was an MTU change or a reopen.

[ ... ]

> @@ -649,6 +685,13 @@ static void mtk_mac_config(struct phylink_config *config, unsigned int mode,
>  		mac->interface = state->interface;
>  	}
>  
> +	/* What a MAC may accept is a property of the shared RX rings, so
> +	 * rewrite the limit on every mac_config and not only when the
> +	 * interface mode changes: a reopen after an MTU change must narrow
> +	 * the MACs that still carry the previous, larger limit.
> +	 */
> +	mtk_set_mcr_max_rx(mac, eth->rx_buf_len);

[Severity: Medium]
Is it intended that this affects every non-MT7628 SoC, and not only
MT7988?

mtk_rx_buf_len_from_mtu() never returns less than MTK_MAX_RX_LENGTH_2K.
Every open goes through
phylink_start()->phylink_mac_initial_config()->mtk_mac_config(). So on
MT7621/7622/7623/7629/7981/7986 the GMAC MCR now gets
MAC_MCR_MAX_RX_2048 at any MTU.

In the baseline, mtk_mac_config() did not touch this field, and
mtk_hw_init() used dev->mtu + MTK_RX_ETH_HLEN. A 1500 MTU interface got
the 1518 bucket and a 1504 DSA conduit got 1536. mtk_hw_init() now uses
eth->rx_buf_len as well.

So on all of these SoCs, frames up to 2048 bytes are accepted at a 1500
MTU, where before they were dropped and counted as rx_long_errors. The
commit message doesn't seem to mention this change.

mtk_change_mtu() also still calls:

	mtk_set_mcr_max_rx(mac, length);

with the per-device new_mtu + MTK_RX_ETH_HLEN. Doesn't that make the
same MTU give a different hardware limit, depending on whether an MTU
change or a reopen happened last?

[ ... ]

> @@ -2310,7 +2352,8 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
>  			if (ring->frag_size <= PAGE_SIZE)
>  				new_data = napi_alloc_frag(ring->frag_size);
>  			else
> -				new_data = mtk_max_lro_buf_alloc(GFP_ATOMIC);
> +				new_data = mtk_max_buf_alloc(ring->frag_size,
> +							     GFP_ATOMIC);

[Severity: Medium]
With a 9K MTU, the normal and QDMA rings now take this path.
mtk_max_buf_alloc() allocates whole pages:

	data = __get_free_pages(gfp_mask | __GFP_COMP | __GFP_NOWARN,
				get_order(size));

For a frag_size of about 9.6K that is 16 KiB. Later in mtk_poll_rx(),
the skb is still built with:

	skb = build_skb(data, ring->frag_size);

So skb->truesize comes from frag_size, not from the 16 KiB actually
allocated. Does that under-report the memory pinned by each skb by
about 40%? Remote traffic could then pin about 1.7x what the socket
rcvbuf and tcp_mem limits allow.

Before this patch only the HWLRO rings had this mismatch. Now it covers
every RX buffer once any MAC's MTU is above about 3.5K.

[ ... ]

> @@ -2812,8 +2855,8 @@ static int mtk_rx_alloc(struct mtk_eth *eth, int ring_no, int rx_flag)
>  		rx_dma_size = soc->rx.dma_size;
>  	}
>  
> -	ring->frag_size = mtk_max_frag_size(rx_data_len);
> -	ring->buf_size = mtk_max_buf_size(ring->frag_size);
> +	ring->frag_size = mtk_max_frag_size(eth, rx_data_len);
> +	ring->buf_size = mtk_max_buf_size(eth, ring->frag_size);

[Severity: Low]
This isn't a bug, but the comment a few lines below, above the page_pool
check in mtk_rx_alloc(), is now stale:

	 * length fits that.  Today every ring does; this is the preparation
	 * for the 9K support in the next patch, where the normal ring grows
	 * past the pool's limit and takes the netdev_alloc_frag()/
	 * mtk_max_lro_buf_alloc() path below instead.

This patch renames mtk_max_lro_buf_alloc() to mtk_max_buf_alloc(). This
patch is also the 9K support. With eth->rx_buf_len up to 9216, not every
ring fits MTK_PP_MAX_BUF_SIZE any more.

[ ... ]

> @@ -3256,6 +3300,55 @@ static int mtk_dma_busy_wait(struct mtk_eth *eth)

[ ... ]

> +static void mtk_pdma_set_sdl(struct mtk_eth *eth)
> +{
> +	const struct mtk_reg_map *reg_map = eth->soc->reg_map;
> +	u32 val;
> +
> +	if (!mtk_is_netsys_v3_or_greater(eth))
> +		return;
> +
> +	val = mtk_r32(eth, reg_map->pdma.rx_cfg);
> +	val &= ~MTK_RX_CFG_SDL_MASK;
> +	val |= FIELD_PREP(MTK_RX_CFG_SDL_MASK,
> +			  MTK_PDMA_LRO_SDL + eth->rx_buf_len);
> +	mtk_w32(eth, val, reg_map->pdma.rx_cfg);

[Severity: Medium]
Is MTK_PDMA_LRO_SDL + eth->rx_buf_len the intended value? The comment in
mtk_dma_init() says the SDL is programmed "from rx_buf_len so jumbo
frames up to it are accepted by the DMA engine".

The code writes 0x3800 (14336) at 2K and 0x5400 (21504) at 9K. That is
about 12 KiB more than both rx_buf_len and the per-descriptor
ring->buf_size.

MTK_PDMA_LRO_SDL is 0x3000, the same as MTK_MAX_LRO_RX_LENGTH. That is
an HW LRO constant, and MT7988 does not use HW LRO here.

The new MTK_RX_CFG_SDL_MASK, GENMASK(30, 16), sits under the "PDMA Global
Configuration Register" comment in mtk_eth_soc.h but is applied to
rx_cfg. At 9K, + and | give different values (0x5400 vs 0x3400), and the
write sets bit 30. If the real field is narrower, could this overwrite a
neighbouring RX_CFG control bit?

This register was never written before, so every MT7988 board now loses
its reset default, even at the default 1500 MTU. Is the field layout
documented somewhere?

[ ... ]

> @@ -3784,7 +3877,8 @@ static int mtk_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (dev->mtu > MTK_PP_MAX_BUF_SIZE) {
> +	if (dev->mtu > MTK_PP_MAX_BUF_SIZE ||
> +	    mtk_rx_buf_len(eth) > MTK_PP_MAX_BUF_SIZE) {

[Severity: High]
Can this accept an XDP program that never runs?

mtk_rx_buf_len() recomputes the length from the current MTUs, but the
live rings were built from eth->rx_buf_len. mtk_change_mtu() allows a
shrink while DMA is running and leaves eth->rx_buf_len unchanged, so the
two values can differ.

On MT7988:

1. Set GMAC2 to MTU 9000 while it is down, then bring up GMAC2 and
   GMAC1. mtk_dma_init() sets eth->rx_buf_len to 9216, so
   ring->buf_size is larger than MTK_PP_MAX_BUF_SIZE and
   mtk_rx_alloc() creates no page_pool.

2. Run "ip link set <gmac2> mtu 1500" while it is running. The old 9000
   MTU still gives 9216, which is not larger than eth->rx_buf_len, so
   there is no -EBUSY and the rings stay at 9K.

3. Attach XDP to either netdev. dev->mtu is 1500 and mtk_rx_buf_len(eth)
   is 2048, so this check passes.

If the target is running, mtk_stop() returns early here, because the
other MAC still holds a reference:

	if (!refcount_dec_and_test(&eth->dma_refcnt))
		return 0;

mtk_open() then only does refcount_inc(). If the target is down,
nothing is reopened. Either way the rings are not rebuilt.

mtk_poll_rx() only calls mtk_xdp_run() inside if (ring->page_pool). So
every packet goes through build_skb() and up the stack while the program
shows as attached.

The commit message says this case is rejected "instead of letting a
program attach and never run". Should this check also look at
eth->rx_buf_len, or at whether the live rings have a page_pool?

[ ... ]

> @@ -4357,14 +4427,33 @@ static int mtk_change_mtu(struct net_device *dev, int new_mtu)
>  	int length = new_mtu + MTK_RX_ETH_HLEN;
>  	struct mtk_mac *mac = netdev_priv(dev);
>  	struct mtk_eth *eth = mac->hw;
> +	bool running;
> +	u32 rx_buf_len;
>  	int max_mtu, i;
>  
> +	/* The RX rings are shared by every MAC, so the buffer length follows
> +	 * the largest MTU of all of them, including the new one.
> +	 */
> +	max_mtu = mtk_max_gmac_mtu(eth);
> +	if (new_mtu > max_mtu)
> +		max_mtu = new_mtu;
> +	rx_buf_len = mtk_rx_buf_len_from_mtu(max_mtu);

[ ... ]

> +	running = refcount_read(&eth->dma_refcnt);
> +	if (running && rx_buf_len > eth->rx_buf_len)
> +		return -EBUSY;

[Severity: Low]
running comes from the DMA refcount that all MACs share, not from
whether this netdev is up.

On MT7988 boards where any other GMAC is up (for example a WAN port on
GMAC2 or GMAC3), raising the DSA conduit to a jumbo MTU returns -EBUSY,
even if the conduit itself is down. dsa_user_change_mtu() passes that
error back to the user port.

DSA brings the conduit up whenever a user port is up. Wouldn't raising a
user port MTU on a live system then always fail, so that 9K DSA only
works once every GMAC is down?

The commit message describes the -EBUSY rule, but not that it limits the
DSA flow it is meant to enable.

> +
>  	mtk_set_mcr_max_rx(mac, length);
>  	WRITE_ONCE(dev->mtu, new_mtu);
>  
> @@ -4372,6 +4461,11 @@ static int mtk_change_mtu(struct net_device *dev, int new_mtu)
>  	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++)
>  		mtk_ppe_update_mtu(eth->ppe[i], max_mtu);
>  
> +	if (!running) {
> +		eth->rx_buf_len = rx_buf_len;
> +		mtk_pdma_set_sdl(eth);
> +	}

[Severity: Low]
The comment in mtk_dma_init() says eth->rx_buf_len is "Derived here, at
ring allocation, so eth->rx_buf_len always describes the buffers the
rings actually hold". Here it is assigned while no rings exist.

rx_buf_len is also computed from mtk_max_gmac_mtu() before
WRITE_ONCE(dev->mtu, new_mtu), so this device's old MTU still counts.

For example, shrinking the only 9000 MTU device to 1500 while it is down
leaves eth->rx_buf_len at 9216 and the SDL programmed for 9216.
mtk_xdp_setup() computes 2048 from the new MTUs, so the two disagree
until the next mtk_dma_init() recomputes the value.

Should this use the new MTUs, or leave eth->rx_buf_len to mtk_dma_init()?

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/patch-aad4cfa8d9522d1f7dbc9893ea12e813%40loljews.com