[PATCH net-next v3 0/2] net: ethernet: mtk_eth_soc: 9K jumbo frame support

[PATCH net-next v3 0/2] net: ethernet: mtk_eth_soc: 9K jumbo frame support

[Sandeep Haemoon]

This series makes the MediaTek SoC ethernet driver support 9K jumbo
frames on the 10G XGMII MACs (MT7988).  The driver advertises a 2030-byte
max_mtu and programs the MAC max-rx field for at most 2048 bytes, so
MTU > 2K is rejected and 9K frames are not usable.

Patch 1 gates page_pool on the descriptor length of the ring: the pool
hands out order-0 pages and maps at most MTK_PP_MAX_BUF_SIZE, so a ring
whose descriptor length is larger has to use the
netdev_alloc_frag()/mtk_max_lro_buf_alloc() path.  It is pure
preparation, every ring fits today.

Patch 2 adds the MTK_NETSYS_RX_9K capability, advertises 9K max_mtu on
the XGMII MACs (from the DT phy_mode at probe, so DSA can raise the
conduit MTU before the netdev is opened), programs XMAC_RX_CFG2 and the
PDMA RX SDL, derives the shared RX buffer length from the largest MTU,
sizes the RX rings from it and keeps the MAC max-rx limits in sync with
it.  A runtime MTU change that needs larger buffers is rejected with
-EBUSY while DMA is running; shrinking is allowed.

This work 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".  The runtime reallocation of Mason's "dynamic rx buffer
adjustment" (feed 999-eth-33) is not included and will be sent
separately.

The equivalent change was tested on a BPI-R4 Pro 8X (MT7988, 10G lan6 /
MxL86252) on the 6.18.60 backport: with both ends at MTU 9000, 8K ICMP
passes and iperf3 with MSS 8900 reaches ~9.87 Gbit/s (vs ~2.57 Gbit/s at
MSS 1460); the DSA conduit is raised to 9004 at boot.  This v7.3 series
is compile-tested with CONFIG_NET_MEDIATEK_SOC=y and COMPILE_TEST=y; no
hardware run of this exact tree yet.

Changes in v3:
- gate page_pool on ring->buf_size <= MTK_PP_MAX_BUF_SIZE instead of
  ring->frag_size <= PAGE_SIZE: frag_size uses the netdev_alloc_frag()
  layout, so a ring could pass the old test while its descriptor length
  was already larger than the pool can map;
- keep 2K as the minimum RX buffer length, so MTUs up to 2K keep working
  at runtime on every SoC, DSA user ports included;
- restore MT7628's 1518 max_mtu, which v2 dropped by accident;
- in mtk_change_mtu() reject only a change that needs larger buffers, so
  a lowered MTU no longer locks the port out of further changes;
- program XMAC_RX_CFG2 with read-modify-write, keeping the bits outside
  the length field;
- replace the PDMA RX SDL field instead of OR-ing into it, which grew on
  every open with a different MTU;
- rewrite the MAC max-rx limits on every mac_config(), so a reopen with a
  smaller MTU narrows MACs that still carry the old limit;
- derive the XDP admission checks from the shared RX buffer length, so a
  program cannot attach and then never run;
- correct the commit messages: no runtime reallocation is included, and
  mtk_hw_init() now uses the shared length too;
- keep Simon Horman's Reviewed-by on 1/2 only, as 2/2 changed materially.

The order-2 allocation cost at 9K (one buffer per slot, high-order
GFP_ATOMIC refill) is unchanged from the downstream design and is called
out in the commit message.

v2: https://lore.kernel.org/all/patch-2793186c653124c4c0b3d463bde2ea3a@loljews.com/
v1: https://lore.kernel.org/all/patch-4f863a941889f0bceca1c082f2d79624@loljews.com/

Sandeep Haemoon (2):
  net: ethernet: mtk_eth_soc: skip page_pool for RX rings it cannot back
  net: ethernet: mtk_eth_soc: add 9K jumbo frame support

 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 215 +++++++++++++++-----
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  10 +-
 2 files changed, 173 insertions(+), 52 deletions(-)

base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.53.0

[PATCH net-next v3 1/2] net: ethernet: mtk_eth_soc: skip page_pool

[Sandeep Haemoon]

page_pool hands out order-0 pages and the pool is created with
MTK_PP_MAX_BUF_SIZE as the maximum DMA length, so it can only back a ring
whose descriptor length (ring->buf_size) fits in that.

Gate the pool on ring->buf_size instead of ring->frag_size: frag_size is
computed with the netdev_alloc_frag() layout, whose headroom and
skb_shared_info footprint differ from the pool's, so a ring can pass a
frag_size <= PAGE_SIZE test while its descriptor length is already larger
than the pool can map.

This is preparation for the 9K support in the next patch.  Today every
ring is smaller than the pool's limit and the new condition is always
true, so there is no functional change; once the normal ring follows a
jumbo MTU it grows past the limit and takes the netdev_alloc_frag()/
mtk_max_lro_buf_alloc() path that is already there.

Link: https://github.com/mediatek/mtk-openwrt-feeds
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Sandeep Haemoon <sandeeph@loljews.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index ad0fdc413..f2b2fb113 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -2819,7 +2819,15 @@ static int mtk_rx_alloc(struct mtk_eth *eth, int ring_no, int rx_flag)
 	if (!ring->data)
 		return -ENOMEM;
 
-	if (mtk_page_pool_enabled(eth)) {
+	/* page_pool hands out order-0 pages and caps the DMA mapping at
+	 * MTK_PP_MAX_BUF_SIZE, so it can only back a ring whose descriptor
+	 * 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.
+	 */
+	if (mtk_page_pool_enabled(eth) &&
+	    ring->buf_size <= MTK_PP_MAX_BUF_SIZE) {
 		struct page_pool *pp;
 
 		pp = mtk_create_page_pool(eth, &ring->xdp_q, ring_no,
-- 
2.53.0

Re: [PATCH net-next v3 1/2] net: ethernet: mtk_eth_soc: skip page_pool

[netdev-bot+sashiko]

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

Critical: 0 (1) · High: 0 · Medium: 1 · Low: 2

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] The new gate in mtk_rx_alloc() (`mtk_page_pool_enabled(eth) &&
  ring->buf_size <= MTK_PP_MAX_BUF_SIZE`) means a netsys v2+ ring can…
- [Low] The commit message does not match the code.
- [Low] The new comment in mtk_rx_alloc() describes changelog history
  rather than the code.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but in
  mtk_poll_rx() the release_desc path rebuilds the descriptor's high DMA…

--

net: ethernet: mtk_eth_soc: skip page_pool

This commit adds a ring->buf_size <= MTK_PP_MAX_BUF_SIZE condition to the
page_pool creation in mtk_rx_alloc(). Rings whose descriptor length is
larger than the pool can back fall back to netdev_alloc_frag() or
mtk_max_lro_buf_alloc(). It is described as preparation for 9K jumbo
support in the next patch, with no functional change at this commit.

Link: https://github.com/mediatek/mtk-openwrt-feeds

> net: ethernet: mtk_eth_soc: skip page_pool

[Severity: Low]
This isn't a bug, but the subject line looks cut off. It doesn't say when
the page_pool is skipped.

> Gate the pool on ring->buf_size instead of ring->frag_size: frag_size is
> computed with the netdev_alloc_frag() layout, whose headroom and
> skb_shared_info footprint differ from the pool's, so a ring can pass a
> frag_size <= PAGE_SIZE test while its descriptor length is already larger
> than the pool can map.

Is "instead of ring->frag_size" accurate? Before this patch, the only
gate on the pool was:

	if (mtk_page_pool_enabled(eth)) {

The ring->frag_size <= PAGE_SIZE test exists only in the non-page_pool
branch. There it chooses between netdev_alloc_frag() and
mtk_max_lro_buf_alloc().

So this patch adds a new size condition rather than replacing a frag_size
one. The frag_size vs. buf_size reasoning and the no-functional-change
claim both look correct. Could the wording avoid implying that a
frag_size gate existed before?

> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index ad0fdc4133fb6..f2b2fb1130ff9 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -2819,7 +2819,15 @@ static int mtk_rx_alloc(struct mtk_eth *eth, int ring_no, int rx_flag)
>  	if (!ring->data)
>  		return -ENOMEM;
>  
> -	if (mtk_page_pool_enabled(eth)) {
> +	/* page_pool hands out order-0 pages and caps the DMA mapping at
> +	 * MTK_PP_MAX_BUF_SIZE, so it can only back a ring whose descriptor
> +	 * 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.
> +	 */

[Severity: Low]
This isn't a bug, but this comment describes the series history rather
than the code. The follow-up "net: ethernet: mtk_eth_soc: add 9K jumbo"
leaves it unchanged. After that patch:

  - "Today every ring does" is no longer true. A 9K ring has buf_size
    9216 and no pool.
  - "the next patch" no longer refers to anything.
  - mtk_max_lro_buf_alloc() has been renamed to mtk_max_buf_alloc().

Also, does page_pool really cap the DMA mapping at MTK_PP_MAX_BUF_SIZE?
mtk_create_page_pool() uses:

	.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
	...
	.offset = MTK_PP_HEADROOM,
	.max_len = MTK_PP_MAX_BUF_SIZE,

PP_FLAG_DMA_MAP maps the whole order-0 page. max_len only limits the
sync-for-device length. The real constraint seems to be that
RX_DMA_PREP_PLEN0(ring->buf_size) must not exceed offset + max_len.

> +	if (mtk_page_pool_enabled(eth) &&
> +	    ring->buf_size <= MTK_PP_MAX_BUF_SIZE) {

[Severity: Medium]
With this condition, a netsys v2+ ring can now have
ring->page_pool == NULL. In mtk_poll_rx(), XDP only runs in the
page_pool branch:

	if (ring->page_pool) {
		...
		ret = mtk_xdp_run(eth, ring, &xdp, netdev);
		...
	} else {
		if (ring->frag_size <= PAGE_SIZE)
			new_data = napi_alloc_frag(ring->frag_size);

If an XDP program is accepted while the ring has no pool, the program
silently never runs.

This can't happen at this commit, because buf_size is always 2048. With
the follow-up "net: ethernet: mtk_eth_soc: add 9K jumbo" applied,
mtk_xdp_setup() checks the rx_buf_len recomputed from the current MTUs:

	if (dev->mtu > MTK_PP_MAX_BUF_SIZE ||
	    mtk_rx_buf_len(eth) > MTK_PP_MAX_BUF_SIZE) {

It does not check eth->rx_buf_len, which the live rings were allocated
with, or ring->page_pool itself.

Can this sequence on MT7988 with two GMACs attach an XDP program that
never runs?

1. With both interfaces down, set eth1 MTU to 9000. mtk_change_mtu()
   takes the !running branch and sets eth->rx_buf_len = 9216.

2. Bring eth0 and eth1 up. buf_size is 9216, so no page_pool is created.

3. While running, set eth1 MTU back to 1500. mtk_change_mtu() allows it:

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

   eth->rx_buf_len stays at 9216.

4. Attach XDP to eth0. mtk_xdp_setup() sees dev->mtu 1500 and
   mtk_rx_buf_len(eth) 2048, and accepts the program. Its mtk_stop() and
   mtk_open() calls only move dma_refcnt 2->1->2, because eth1 is still
   up:

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

   So the rings are not reallocated.

The attach returns success, but all RX traffic bypasses the program.
xdp_features is also still advertised based on mtk_page_pool_enabled()
alone.

Could mtk_xdp_setup() check eth->rx_buf_len, or whether the rings have a
page_pool, instead?

>  		struct page_pool *pp;
>  
>  		pp = mtk_create_page_pool(eth, &ring->xdp_q, ring_no,

[Severity: Critical]
This is a pre-existing issue and wasn't introduced by this patch. While
looking at the fallback path that this series moves MT7988 jumbo rings
onto, it looks like mtk_poll_rx() can hand a descriptor back to the
hardware with a mixed DMA address.

dma_addr is set once, outside the loop:

	dma_addr_t dma_addr = DMA_MAPPING_ERROR;

The release_desc path rebuilds the high address bits from it:

skip_rx:
		ring->data[idx] = new_data;
		rxd->rxd1 = (unsigned int)dma_addr;
release_desc:
		if (MTK_HAS_CAPS(eth->soc->caps, MTK_36BIT_DMA)) {
			if (unlikely(dma_addr == DMA_MAPPING_ERROR))
				addr64 = FIELD_GET(RX_DMA_ADDR64_MASK,
						   rxd->rxd2);
			else
				addr64 = RX_DMA_PREP_ADDR64(dma_addr);
		}

Suppose iteration N refills its slot with buffer X, which leaves
dma_addr set to X. Iteration N+1 can then jump straight to release_desc
in three cases:

  - there is no netdev for the source port
  - MTK_RESETTING is set
  - a GFP_ATOMIC allocation fails in mtk_page_pool_get_buff() or
    napi_alloc_frag()

mtk_page_pool_get_buff() only writes *dma_addr on success. In that case
rxd1 still holds lo(Y) for the buffer Y the slot owns, but rxd2 gets
hi(X).

On MTK_36BIT_DMA SoCs with DRAM above 4 GiB, can the NIC then DMA the
next frame to hi(X)|lo(Y), which is neither buffer? The driver would
later call dma_unmap_single() on that address and pass the stale
contents of Y up the stack.

Should dma_addr be reset to DMA_MAPPING_ERROR at the start of each loop
iteration?

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

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

[Sandeep Haemoon]

The driver caps max_mtu at 2030 bytes and programs the MAC max-rx field
for at most 2048-byte frames, so MTU > 2K is rejected and 9K jumbo
frames are not usable on the 10G XGMII MACs.

Add a MTK_NETSYS_RX_9K capability for MT7988 and:
- advertise 9K max_mtu on the XGMII MACs, from the DT phy_mode at probe
  time, so DSA can raise the conduit MTU before the netdev is opened;
- program XMAC_RX_CFG2 on the XGMII MACs, keeping the bits outside the
  length field;
- program the PDMA RX SDL max frame from the shared buffer length,
  replacing the field instead of OR-ing into the value already there;
- derive the shared RX buffer length from the largest MTU of any MAC and
  size the RX rings from it, with 2K as the minimum so the MTUs every SoC
  accepts today keep working, at runtime included;
- program the MAC max-rx limits from that shared length in
  mtk_mac_config(), so no MAC accepts a frame the rings cannot hold, not
  even after a reopen with a smaller MTU;
- reject only a runtime MTU change that would need larger buffers while
  DMA is running (-EBUSY).  Shrinking is allowed: the rings keep the
  buffers they already have.

XDP needs a page_pool, and the pool cannot back buffers larger than
MTK_PP_MAX_BUF_SIZE, so an MTU that needs a larger buffer cannot be
combined with XDP.  Both the XDP attach and the MTU change are rejected
in that case, instead of letting a program attach and never run.

The RX rings are shared, so the largest MTU of any MAC pins order-2
buffers for every ring while DMA is up, around 64 MiB at 9K; the refill
in mtk_poll_rx() is then a high-order GFP_ATOMIC allocation.  That is the
cost of the downstream design and is documented in the commit message on
purpose.

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".  The runtime reallocation of Mason's "dynamic rx buffer
adjustment" (feed 999-eth-33) is not included; it will be sent
separately.

Link: https://github.com/frank-w/BPI-Router-Linux/commit/b4c6cb1d5532
Link: https://github.com/mediatek/mtk-openwrt-feeds
Signed-off-by: Sandeep Haemoon <sandeeph@loljews.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 205 +++++++++++++++-----
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  10 +-
 2 files changed, 164 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f2b2fb113..c7bc8eaea 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -171,6 +171,7 @@ static const struct mtk_reg_map mt7988_reg_map = {
 		.glo_cfg	= 0x6a04,
 		.rst_idx	= 0x6a08,
 		.delay_irq	= 0x6a0c,
+		.rx_cfg		= 0x6a10,
 		.irq_status	= 0x6a20,
 		.irq_mask	= 0x6a28,
 		.adma_rx_dbg0	= 0x6a38,
@@ -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;
+
+		if (val <= 1518)
+			mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_1518);
+		else if (val <= 1536)
+			mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_1536);
+		else if (val <= 1552)
+			mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_1552);
+		else
+			mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_2048);
+
+		if (mcr_new != mcr_cur)
+			mtk_w32(mac->hw, mcr_new, MTK_MAC_MCR(mac->id));
+	} else if (mtk_is_netsys_v3_or_greater(mac->hw) &&
+		   mac->id != MTK_GMAC1_ID) {
+		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));
+
+		if (mcr_new != mcr_cur)
+			mtk_w32(mac->hw, mcr_new, MTK_XMAC_RX_CFG2(mac->id));
+	}
+}
+
 static void mtk_mac_config(struct phylink_config *config, unsigned int mode,
 			   const struct phylink_link_state *state)
 {
@@ -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);
+
 	/* SGMII */
 	if (state->interface == PHY_INTERFACE_MODE_SGMII ||
 	    phy_interface_mode_is_8023z(state->interface)) {
@@ -1242,22 +1285,22 @@ static void mtk_get_stats64(struct net_device *dev,
 	storage->tx_dropped = dev->stats.tx_dropped;
 }
 
-static inline int mtk_max_frag_size(int mtu)
+static inline int mtk_max_frag_size(struct mtk_eth *eth, int mtu)
 {
-	/* make sure buf_size will be at least MTK_MAX_RX_LENGTH */
-	if (mtu + MTK_RX_ETH_HLEN < MTK_MAX_RX_LENGTH_2K)
-		mtu = MTK_MAX_RX_LENGTH_2K - MTK_RX_ETH_HLEN;
+	/* make sure buf_size will be at least max rx buffer length */
+	if (mtu + MTK_RX_ETH_HLEN < eth->rx_buf_len)
+		mtu = eth->rx_buf_len - MTK_RX_ETH_HLEN;
 
 	return SKB_DATA_ALIGN(MTK_RX_HLEN + mtu) +
 		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 }
 
-static inline int mtk_max_buf_size(int frag_size)
+static inline int mtk_max_buf_size(struct mtk_eth *eth, int frag_size)
 {
 	int buf_size = frag_size - NET_SKB_PAD - NET_IP_ALIGN -
 		       SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-	WARN_ON(buf_size < MTK_MAX_RX_LENGTH_2K);
+	WARN_ON(buf_size < eth->rx_buf_len);
 
 	return buf_size;
 }
@@ -1280,9 +1323,8 @@ static bool mtk_rx_get_desc(struct mtk_eth *eth, struct mtk_rx_dma_v2 *rxd,
 	return true;
 }
 
-static void *mtk_max_lro_buf_alloc(gfp_t gfp_mask)
+static void *mtk_max_buf_alloc(unsigned int size, gfp_t gfp_mask)
 {
-	unsigned int size = mtk_max_frag_size(MTK_MAX_LRO_RX_LENGTH);
 	unsigned long data;
 
 	data = __get_free_pages(gfp_mask | __GFP_COMP | __GFP_NOWARN,
@@ -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);
 
 			if (unlikely(!new_data)) {
 				netdev->stats.rx_dropped++;
@@ -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);
 	ring->data = kcalloc(rx_dma_size, sizeof(*ring->data),
 			     GFP_KERNEL);
 	if (!ring->data)
@@ -2860,7 +2903,8 @@ static int mtk_rx_alloc(struct mtk_eth *eth, int ring_no, int rx_flag)
 			if (ring->frag_size <= PAGE_SIZE)
 				data = netdev_alloc_frag(ring->frag_size);
 			else
-				data = mtk_max_lro_buf_alloc(GFP_KERNEL);
+				data = mtk_max_buf_alloc(ring->frag_size,
+							 GFP_KERNEL);
 
 			if (!data)
 				return -ENOMEM;
@@ -3256,6 +3300,55 @@ static int mtk_dma_busy_wait(struct mtk_eth *eth)
 	return ret;
 }
 
+static int mtk_max_gmac_mtu(struct mtk_eth *eth)
+{
+	int i, max_mtu = ETH_DATA_LEN;
+
+	for (i = 0; i < ARRAY_SIZE(eth->netdev); i++)
+		if (eth->netdev[i] && eth->netdev[i]->mtu > max_mtu)
+			max_mtu = eth->netdev[i]->mtu;
+
+	return max_mtu;
+}
+
+static u32 mtk_rx_buf_len_from_mtu(int max_mtu)
+{
+	int length = max_mtu + MTK_RX_ETH_HLEN;
+
+	if (length <= MTK_MAX_RX_LENGTH_2K)
+		return MTK_MAX_RX_LENGTH_2K;
+
+	return DIV_ROUND_UP(length, 1024) * 1024;
+}
+
+/* The rx rings are shared by every MAC, so their buffers are sized for the
+ * largest MTU configured on any of them.
+ */
+static u32 mtk_rx_buf_len(struct mtk_eth *eth)
+{
+	return mtk_rx_buf_len_from_mtu(mtk_max_gmac_mtu(eth));
+}
+
+/* Program the PDMA RX max-frame field.  The frame engine is only reset at
+ * probe and on error recovery, not on every open, so the field has to be
+ * replaced rather than OR-ed: a later open with a different MTU would
+ * otherwise accumulate the old value.
+ */
+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);
+}
+
 static int mtk_dma_init(struct mtk_eth *eth)
 {
 	int err;
@@ -3264,6 +3357,12 @@ static int mtk_dma_init(struct mtk_eth *eth)
 	if (mtk_dma_busy_wait(eth))
 		return -EBUSY;
 
+	/* Derived here, at ring allocation, so eth->rx_buf_len always describes
+	 * the buffers the rings actually hold and mtk_mac_config() never lets
+	 * a MAC accept a frame the rings cannot take.
+	 */
+	eth->rx_buf_len = mtk_rx_buf_len(eth);
+
 	if (MTK_HAS_CAPS(eth->soc->caps, MTK_QDMA)) {
 		/* QDMA needs scratch memory for internal reordering of the
 		 * descriptors
@@ -3307,6 +3406,11 @@ static int mtk_dma_init(struct mtk_eth *eth)
 		mtk_w32(eth, 0x0, eth->soc->reg_map->qdma.hred);
 	}
 
+	/* Program the PDMA max RX frame (SDL) from rx_buf_len so jumbo
+	 * frames up to it are accepted by the DMA engine.
+	 */
+	mtk_pdma_set_sdl(eth);
+
 	return 0;
 }
 
@@ -3597,17 +3701,6 @@ static int mtk_device_event(struct notifier_block *n, unsigned long event, void
 	return NOTIFY_DONE;
 }
 
-static int mtk_max_gmac_mtu(struct mtk_eth *eth)
-{
-	int i, max_mtu = ETH_DATA_LEN;
-
-	for (i = 0; i < ARRAY_SIZE(eth->netdev); i++)
-		if (eth->netdev[i] && eth->netdev[i]->mtu > max_mtu)
-			max_mtu = eth->netdev[i]->mtu;
-
-	return max_mtu;
-}
-
 static int mtk_open(struct net_device *dev)
 {
 	struct mtk_mac *mac = netdev_priv(dev);
@@ -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) {
 		NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
 		return -EOPNOTSUPP;
 	}
@@ -3924,29 +4018,6 @@ static void mtk_dim_tx(struct work_struct *work)
 	dim->state = DIM_START_MEASURE;
 }
 
-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;
-
-	mcr_cur = mtk_r32(mac->hw, MTK_MAC_MCR(mac->id));
-	mcr_new = mcr_cur & ~MAC_MCR_MAX_RX_MASK;
-
-	if (val <= 1518)
-		mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_1518);
-	else if (val <= 1536)
-		mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_1536);
-	else if (val <= 1552)
-		mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_1552);
-	else
-		mcr_new |= MAC_MCR_MAX_RX(MAC_MCR_MAX_RX_2048);
-
-	if (mcr_new != mcr_cur)
-		mtk_w32(mac->hw, mcr_new, MTK_MAC_MCR(mac->id));
-}
 
 static void mtk_hw_reset(struct mtk_eth *eth)
 {
@@ -4215,8 +4286,7 @@ static int mtk_hw_init(struct mtk_eth *eth, bool reset)
 			continue;
 
 		mtk_w32(eth, MAC_MCR_FORCE_LINK_DOWN, MTK_MAC_MCR(i));
-		mtk_set_mcr_max_rx(netdev_priv(dev),
-				   dev->mtu + MTK_RX_ETH_HLEN);
+		mtk_set_mcr_max_rx(netdev_priv(dev), eth->rx_buf_len);
 	}
 
 	/* Indicates CDM to parse the MTK special tag from CPU
@@ -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);
+
 	if (rcu_access_pointer(eth->prog) &&
-	    length > MTK_PP_MAX_BUF_SIZE) {
+	    rx_buf_len > MTK_PP_MAX_BUF_SIZE) {
 		netdev_err(dev, "Invalid MTU for XDP mode\n");
 		return -EINVAL;
 	}
 
+	/* The rings cannot be resized in place while DMA is running.  A
+	 * larger buffer would need the reallocation at the next open; a
+	 * smaller one is fine, the buffers the rings hold are already big
+	 * enough for it.
+	 */
+	running = refcount_read(&eth->dma_refcnt);
+	if (running && rx_buf_len > eth->rx_buf_len)
+		return -EBUSY;
+
 	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);
+	}
+
 	return 0;
 }
 
@@ -5017,8 +5111,17 @@ static int mtk_add_mac(struct mtk_eth *eth, struct device_node *np)
 	eth->netdev[id]->irq = eth->irq[MTK_FE_IRQ_SHARED];
 	eth->netdev[id]->dev.of_node = np;
 
+	/* DSA sets the conduit MTU (user MTU + tag) before the netdev is
+	 * opened, so max_mtu must already be final here.  Deriving it only
+	 * in mtk_open() leaves the conduit at its 1504 default and blocks
+	 * 9K frames on the DSA user ports.
+	 */
 	if (MTK_HAS_CAPS(eth->soc->caps, MTK_SOC_MT7628))
 		eth->netdev[id]->max_mtu = MTK_MAX_RX_LENGTH - MTK_RX_ETH_HLEN;
+	else if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_RX_9K) &&
+		 mtk_interface_mode_is_xgmii(eth, phy_mode))
+		eth->netdev[id]->max_mtu =
+			MTK_MAX_RX_LENGTH_9K - MTK_RX_ETH_HLEN;
 	else
 		eth->netdev[id]->max_mtu = MTK_MAX_RX_LENGTH_2K - MTK_RX_ETH_HLEN;
 
@@ -5159,6 +5262,8 @@ static int mtk_probe(struct platform_device *pdev)
 	spin_lock_init(&eth->rx_irq_lock);
 	spin_lock_init(&eth->dim_lock);
 
+	eth->rx_buf_len = MTK_MAX_RX_LENGTH_2K;
+
 	eth->rx_dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE;
 	INIT_WORK(&eth->rx_dim.work, mtk_dim_rx);
 	INIT_DELAYED_WORK(&eth->reset.monitor_work, mtk_hw_reset_monitor_work);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 88a9b3b23..0cf2aaf23 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -29,6 +29,7 @@
 #define MTK_QDMA_PAGE_SIZE	2048
 #define MTK_MAX_RX_LENGTH	1536
 #define MTK_MAX_RX_LENGTH_2K	2048
+#define MTK_MAX_RX_LENGTH_9K	9216
 #define MTK_TX_DMA_BUF_LEN	0x3fff
 #define MTK_TX_DMA_BUF_LEN_V2	0xffff
 #define MTK_QDMA_RING_SIZE	2048
@@ -201,6 +202,7 @@
 /* PDMA Global Configuration Register */
 #define MTK_PDMA_LRO_SDL	0x3000
 #define MTK_RX_CFG_SDL_OFFSET	16
+#define MTK_RX_CFG_SDL_MASK	GENMASK(30, MTK_RX_CFG_SDL_OFFSET)
 
 /* PDMA Reset Index Register */
 #define MTK_PST_DRX_IDX0	BIT(16)
@@ -530,6 +532,8 @@
 /* XFI Mac control registers */
 #define MTK_XMAC_BASE(x)	(0x12000 + (((x) - 1) * 0x1000))
 #define MTK_XMAC_MCR(x)		(MTK_XMAC_BASE(x))
+#define MTK_XMAC_RX_CFG2(x)	(MTK_XMAC_MCR(x) + 0xd0)
+#define MTK_XMAC_MAX_RX_MASK	GENMASK(13, 0)
 #define XMAC_MCR_TRX_DISABLE	0xf
 #define XMAC_MCR_FORCE_TX_FC	BIT(5)
 #define XMAC_MCR_FORCE_RX_FC	BIT(4)
@@ -985,6 +989,7 @@ enum mkt_eth_capabilities {
 	MTK_INFRA_BIT,
 	MTK_SHARED_SGMII_BIT,
 	MTK_HWLRO_BIT,
+	MTK_NETSYS_RX_9K_BIT,
 	MTK_SHARED_INT_BIT,
 	MTK_TRGMII_MT7621_CLK_BIT,
 	MTK_QDMA_BIT,
@@ -1026,6 +1031,7 @@ enum mkt_eth_capabilities {
 #define MTK_INFRA		BIT_ULL(MTK_INFRA_BIT)
 #define MTK_SHARED_SGMII	BIT_ULL(MTK_SHARED_SGMII_BIT)
 #define MTK_HWLRO		BIT_ULL(MTK_HWLRO_BIT)
+#define MTK_NETSYS_RX_9K	BIT_ULL(MTK_NETSYS_RX_9K_BIT)
 #define MTK_SHARED_INT		BIT_ULL(MTK_SHARED_INT_BIT)
 #define MTK_TRGMII_MT7621_CLK	BIT_ULL(MTK_TRGMII_MT7621_CLK_BIT)
 #define MTK_QDMA		BIT_ULL(MTK_QDMA_BIT)
@@ -1127,7 +1133,7 @@ enum mkt_eth_capabilities {
 
 #define MT7988_CAPS  (MTK_36BIT_DMA | MTK_GDM1_ESW | MTK_GMAC2_2P5GPHY | \
 		      MTK_MUX_GMAC2_TO_2P5GPHY | MTK_QDMA | MTK_RSTCTRL_PPE1 | \
-		      MTK_RSTCTRL_PPE2 | MTK_SRAM)
+		      MTK_RSTCTRL_PPE2 | MTK_SRAM | MTK_NETSYS_RX_9K)
 
 struct mtk_tx_dma_desc_info {
 	dma_addr_t	addr;
@@ -1151,6 +1157,7 @@ struct mtk_reg_map {
 		u32	glo_cfg;	/* global configuration */
 		u32	rst_idx;	/* reset index */
 		u32	delay_irq;	/* delay interrupt */
+		u32	rx_cfg;		/* rx dma configuration */
 		u32	irq_status;	/* interrupt status */
 		u32	irq_mask;	/* interrupt mask */
 		u32	adma_rx_dbg0;
@@ -1314,6 +1321,7 @@ struct mtk_eth {
 	struct mtk_tx_ring		tx_ring;
 	struct mtk_rx_ring		rx_ring[MTK_MAX_RX_RING_NUM];
 	struct mtk_rx_ring		rx_ring_qdma;
+	u32				rx_buf_len;
 	struct napi_struct		tx_napi;
 	struct napi_struct		rx_napi;
 	void				*scratch_ring;
-- 
2.53.0

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

[netdev-bot+sashiko]

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