[PATCH net-next 0/7] net: bcmgenet: support larger MTUs

[PATCH net-next 0/7] net: bcmgenet: support larger MTUs

[Nicolai Buchwitz]

Larger MTU aka. jumbo frames are requested often by Raspberry Pi users, but
the hardware has its quirks with larger frames, so it never got done. Dave
Stevenson's PoC [1] had the receive side working and Justin checked the RTL
what the hardware does with long frames, which got me past the rest.

GENET never sets dev->max_mtu, and the packet ready thresholds cut frames
off at their 2048 byte reset default. With this the receive dropped them as
fragmented and transmit never sent them out. See commit messages of the
patches for more details.

Patches 1 to 5 implement the MTU as far as one descriptor reaches (3564
bytes with 4K pages). Patch 6 pads around a transmit quirk where a frame
ending just past the threshold stops the transmitter [3]. Patch 7
reassembles the fragments the hardware produces above it, which gets the
ceiling to 16347 [2].

Tested on a CM4 (GENET v5). MTU 1500 is unchanged, MTU 9000 reaches 991/987
Mbit/s. Pierre-Marin Leclercq tested on a Pi 4 (also GENET v5) with various
MTU sizes.

[1] https://github.com/raspberrypi/linux/pull/7614
[2] https://github.com/raspberrypi/linux/pull/7617#issuecomment-5669564355
[3] https://github.com/raspberrypi/linux/pull/7617#issuecomment-5735557324

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
Nicolai Buchwitz (7):
      net: bcmgenet: let the caller decide whether to start the PHY
      net: bcmgenet: allow a continuation descriptor without the alignment pad
      net: bcmgenet: rename ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN
      net: bcmgenet: derive the receive buffer length from the MTU
      net: bcmgenet: allow the MTU to be changed
      net: bcmgenet: pad transmit frames out of the packet ready window
      net: bcmgenet: reassemble jumbo frames from status block fragments

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 303 +++++++++++++++++++++++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  19 +-
 2 files changed, 295 insertions(+), 27 deletions(-)
---
base-commit: 071876fd50482a68603a9460d80dd6dd58827ee1
change-id: 20261002-nb-genet-mtu-nn-v2-402b5e6f306f

Best regards,
-- 
Nicolai Buchwitz <nb@tipi-net.de>

[PATCH net-next 1/7] net: bcmgenet: let the caller decide whether to start the PHY

[Nicolai Buchwitz]

bcmgenet_netif_stop() already takes stop_phy, bcmgenet_netif_start() does
not. The MTU change in a later patch leaves the PHY running while the
datapath goes down and comes back, and phy_start() expects a stopped PHY.

Add the same parameter to the start side.

No functional change.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 4c9db2f9fc25..a82579879f4b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3348,7 +3348,7 @@ static void bcmgenet_get_hw_addr(struct bcmgenet_priv *priv,
 	put_unaligned_be16(addr_tmp, &addr[4]);
 }
 
-static void bcmgenet_netif_start(struct net_device *dev)
+static void bcmgenet_netif_start(struct net_device *dev, bool start_phy)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 
@@ -3365,7 +3365,8 @@ static void bcmgenet_netif_start(struct net_device *dev)
 	/* Monitor link interrupts now */
 	bcmgenet_link_intr_enable(priv);
 
-	phy_start(dev->phydev);
+	if (start_phy)
+		phy_start(dev->phydev);
 }
 
 static int bcmgenet_open(struct net_device *dev)
@@ -3428,7 +3429,7 @@ static int bcmgenet_open(struct net_device *dev)
 
 	bcmgenet_phy_pause_set(dev, priv->rx_pause, priv->tx_pause);
 
-	bcmgenet_netif_start(dev);
+	bcmgenet_netif_start(dev, true);
 
 	netif_tx_start_all_queues(dev);
 
@@ -4312,7 +4313,7 @@ static int bcmgenet_resume(struct device *d)
 	if (!device_may_wakeup(d))
 		phy_resume(dev->phydev);
 
-	bcmgenet_netif_start(dev);
+	bcmgenet_netif_start(dev, true);
 
 	netif_device_attach(dev);
 

-- 
2.53.0

Re: [PATCH net-next 1/7] net: bcmgenet: let the caller decide whether to start the PHY

[Florian Fainelli]

On 10/2/26 08:03, Nicolai Buchwitz wrote:
> bcmgenet_netif_stop() already takes stop_phy, bcmgenet_netif_start() does
> not. The MTU change in a later patch leaves the PHY running while the
> datapath goes down and comes back, and phy_start() expects a stopped PHY.
> 
> Add the same parameter to the start side.
> 
> No functional change.
> 
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next 2/7] net: bcmgenet: allow a continuation descriptor without the alignment pad

[Nicolai Buchwitz]

A frame longer than the packet ready threshold arrives in several
descriptors, each with its own status block. Only the first one also
carries the two alignment bytes. The length check assumes the pad is always
there, so a continuation holding a single byte looks two bytes too short
and the whole frame is dropped.

Account for the pad on the first descriptor only.

The MTU cannot produce a frame past the threshold yet, so nothing hits this
today. It is preparation for the larger MTU.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
Tested-by: Pierre-Marin Leclercq <pierremarinleclercq88@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index a82579879f4b..d89ae6599760 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2329,6 +2329,7 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 		unsigned int rx_offset, rx_size;
 		struct status_64 *status;
 		struct page *rx_page;
+		unsigned int min_len;
 		void *hard_start;
 		__be16 rx_csum;
 
@@ -2365,8 +2366,12 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 			  __func__, p_index, ring->c_index,
 			  ring->read_ptr, dma_length_status);
 
+		/* Only the first descriptor carries the alignment pad */
+		min_len = dma_flag & DMA_SOP ? GENET_RSB_PAD
+					     : sizeof(struct status_64);
+
 		/* Reject lengths that would underflow the SKB build path. */
-		if (unlikely(len > RX_BUF_LENGTH || len < GENET_RSB_PAD)) {
+		if (unlikely(len > RX_BUF_LENGTH || len < min_len)) {
 			netif_err(priv, rx_status, dev,
 				  "invalid packet length %d\n", len);
 			BCMGENET_STATS64_INC(stats, length_errors);

-- 
2.53.0

Re: [PATCH net-next 2/7] net: bcmgenet: allow a continuation descriptor without the alignment pad

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Possible commit message mismatch in "net: bcmgenet: allow a
  continuation descriptor without the alignment pad".
- [Low] The RX buffer layout comment above GENET_RSB_PAD
  (bcmgenet.c:54-58 at this commit, 67-72 at the end of the series) is…

--

net: bcmgenet: allow a continuation descriptor without the alignment pad

This patch changes the RX length check in bcmgenet_desc_rx(). Only the
first (DMA_SOP) descriptor of a frame now has to carry the 2-byte
alignment pad after the status block. Continuation descriptors only need
room for the 64-byte struct status_64. The patch prepares for a larger
MTU.

> A frame longer than the packet ready threshold arrives in several
> descriptors, each with its own status block. Only the first one also
> carries the two alignment bytes. The length check assumes the pad is always
> there, so a continuation holding a single byte looks two bytes too short
> and the whole frame is dropped.

[Severity: Low]
Is this description accurate for the tree at this commit?

A continuation holding a single byte has len = 64 + 1 = 65. That is one
byte below the old GENET_RSB_PAD minimum of 66, not two.

Also, a continuation descriptor that passes the new min_len check still
reaches the unchanged check just below it in bcmgenet_desc_rx():

	if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
		netif_err(priv, rx_status, dev,
			  "dropping fragmented packet!\n");
		BCMGENET_STATS64_INC(stats, fragmented_errors);
		...
		goto next;
	}

So at this commit, the only change for such a descriptor is the counter
and log message. It goes to fragmented_errors instead of length_errors.

The behaviour the message describes, where a length error drops the whole
frame through bcmgenet_discard_frags(), only exists once the later patch
in the series is applied: "net: bcmgenet: reassemble jumbo frames from
status block fragments".

Could the message say that this bound is meant for the reassembly path
added later in the series? Could it also correct the one versus two byte
arithmetic?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index a82579879f4b5..d89ae65997604 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2365,8 +2366,12 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  			  __func__, p_index, ring->c_index,
>  			  ring->read_ptr, dma_length_status);
>  
> +		/* Only the first descriptor carries the alignment pad */
> +		min_len = dma_flag & DMA_SOP ? GENET_RSB_PAD
> +					     : sizeof(struct status_64);

[Severity: Low]
This isn't a bug, but the RX buffer layout comment above GENET_RSB_PAD
still says every buffer has the pad:

/* Page pool RX buffer layout:
 * RSB(64) + pad(2) | frame data | skb_shared_info
 * The HW writes the 64B RSB + 2B alignment padding before the frame.
 */
#define GENET_RSB_PAD		(sizeof(struct status_64) + 2)

That contradicts the rule added here. The later reassembly patch in the
series depends on this rule: bcmgenet_add_frag() reads continuation data
from offset + sizeof(struct status_64), not offset + GENET_RSB_PAD.
However, no patch in the series updates the layout comment.

Should the layout comment say that only the SOP buffer carries the 2-byte
pad?

> +
>  		/* Reject lengths that would underflow the SKB build path. */
> -		if (unlikely(len > RX_BUF_LENGTH || len < GENET_RSB_PAD)) {
> +		if (unlikely(len > RX_BUF_LENGTH || len < min_len)) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de

[PATCH net-next 3/7] net: bcmgenet: rename ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN

[Nicolai Buchwitz]

ENET_MAX_MTU_SIZE holds a frame length, not an MTU. Both users program it
into hardware that expects a frame length. The name is wrong once the MTU
is no longer fixed at ETH_DATA_LEN.

No functional change.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h | 7 +++----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d89ae6599760..5cb3d25482a0 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2638,7 +2638,7 @@ static void init_umac(struct bcmgenet_priv *priv)
 			     UMAC_MIB_CTRL);
 	bcmgenet_umac_writel(priv, 0, UMAC_MIB_CTRL);
 
-	bcmgenet_umac_writel(priv, ENET_MAX_MTU_SIZE, UMAC_MAX_FRAME_LEN);
+	bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN, UMAC_MAX_FRAME_LEN);
 
 	/* init tx registers, enable TSB */
 	reg = bcmgenet_tbuf_ctrl_get(priv);
@@ -2744,7 +2744,7 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 
 	/* Set flow period for ring != 0 */
 	if (index)
-		flow_period_val = ENET_MAX_MTU_SIZE << 16;
+		flow_period_val = ENET_MAX_FRAME_LEN << 16;
 
 	bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_PROD_INDEX);
 	bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_CONS_INDEX);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 86f2aed20dbe..501dd1256693 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -28,12 +28,11 @@
 /* which ring is descriptor based */
 #define DESC_INDEX				16
 
-/* Body(1500) + EH_SIZE(14) + VLANTAG(4) + BRCMTAG(6) + FCS(4) = 1528.
- * 1536 is multiple of 256 bytes
- */
 #define ENET_BRCM_TAG_LEN	6
 #define ENET_PAD		8
-#define ENET_MAX_MTU_SIZE	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
+
+/* Longest frame the MAC must accept for the default MTU */
+#define ENET_MAX_FRAME_LEN	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
 				 ENET_BRCM_TAG_LEN + ETH_FCS_LEN + ENET_PAD)
 #define DMA_MAX_BURST_LENGTH    0x10
 

-- 
2.53.0

Re: [PATCH net-next 3/7] net: bcmgenet: rename ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In drivers/net/ethernet/broadcom/genet/bcmgenet.h, the patch adds
  the comment "Longest frame the MAC must accept for the default MTU"…

--

net: bcmgenet: rename ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN

This commit renames ENET_MAX_MTU_SIZE to ENET_MAX_FRAME_LEN, because the
value is a frame length that gets programmed into the hardware and not an
MTU. It also replaces the comment above the definition. The commit says
there is no functional change.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 86f2aed20dbe8..501dd12566936 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -28,12 +28,11 @@
>  /* which ring is descriptor based */
>  #define DESC_INDEX				16
>  
> -/* Body(1500) + EH_SIZE(14) + VLANTAG(4) + BRCMTAG(6) + FCS(4) = 1528.
> - * 1536 is multiple of 256 bytes
> - */
>  #define ENET_BRCM_TAG_LEN	6
>  #define ENET_PAD		8
> -#define ENET_MAX_MTU_SIZE	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
> +
> +/* Longest frame the MAC must accept for the default MTU */
> +#define ENET_MAX_FRAME_LEN	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
>  				 ENET_BRCM_TAG_LEN + ETH_FCS_LEN + ENET_PAD)

[Severity: Low]
This isn't a bug, but does the new comment match the value of the macro?

ENET_MAX_FRAME_LEN works out to:

  1500 + 14 + 4 + 6 + 4 + 8 = 1536

ENET_PAD (8 bytes) is not part of an Ethernet frame. The longest frame for
the default MTU is 1528 bytes, which the removed comment spelled out.

The removed comment also explained that the value was rounded up to 1536
because 1536 is a multiple of 256. Without it, nothing in the driver says
why ENET_PAD exists. The new comment presents the padded value as a frame
length.

The same wording is still there at the end of the series. In "net: bcmgenet:
allow the MTU to be changed", the macro becomes:

  /* Longest frame the MAC must accept for a given MTU */
  #define ENET_FRAME_OVERHEAD (ETH_HLEN + VLAN_HLEN + ENET_BRCM_TAG_LEN + \
                               ETH_FCS_LEN + ENET_PAD)
  #define ENET_MAX_FRAME_LEN(mtu) ((mtu) + ENET_FRAME_OVERHEAD)

With an arbitrary MTU there is no rounding to a 256-byte multiple, so
ENET_PAD is just 8 bytes of slack with no stated reason.

Could the comment either keep the 1528/1536 explanation, or say why ENET_PAD
is still counted as frame overhead?

>  #define DMA_MAX_BURST_LENGTH    0x10
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de

[PATCH net-next 4/7] net: bcmgenet: derive the receive buffer length from the MTU

[Nicolai Buchwitz]

The receive buffer length is a fixed 2048 bytes. The packet ready
thresholds keep whatever value the reset left. Neither follows the MTU.

Compute the receive threshold from the MTU and program it into RBUF. The
buffer length follows from it, with the status block on top. The MTU is
still fixed at ETH_DATA_LEN, so the threshold comes out at the reset
default and the buffer only grows by the status block the hardware already
wrote.

Program the transmit threshold at its maximum as well. It sets how much of
a frame the MAC holds before it starts sending, and holding less buys
nothing. A later patch lowers it for the few MTUs that need the room.

Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 80 ++++++++++++++++++++++----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 ++
 2 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 5cb3d25482a0..bf889558f6ad 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -48,17 +48,34 @@
 #define GENET_Q0_TX_BD_CNT	\
 	(TOTAL_DESC - priv->hw_params->tx_queues * priv->hw_params->tx_bds_per_q)
 
-#define RX_BUF_LENGTH		2048
 #define SKB_ALIGNMENT		32
 
+/* RBUF and TBUF hand a frame to the DMA once the threshold is reached. Both
+ * registers are 8 bit in units of 16 bytes and want a multiple of the 256
+ * byte burst size, so 0xf0 is the largest usable value.
+ */
+#define ENET_THLD_UNIT		16
+#define ENET_THLD_BURST		256
+#define ENET_THLD_DEFAULT	0x80
+#define ENET_THLD_MAX		0xf0
+
 /* Page pool RX buffer layout:
  * RSB(64) + pad(2) | frame data | skb_shared_info
  * The HW writes the 64B RSB + 2B alignment padding before the frame.
  */
-#define GENET_RSB_PAD		(sizeof(struct status_64) + 2)
+#define GENET_RBUF_ALIGN	2
+#define GENET_RSB_PAD		(sizeof(struct status_64) + GENET_RBUF_ALIGN)
 
-/* RX buffer plus the skb_shared_info napi_build_skb() places behind it */
-#define GENET_RX_BUF_SIZE	SKB_HEAD_ALIGN(RX_BUF_LENGTH)
+/* A descriptor is one page, which also holds skb_shared_info behind the frame,
+ * so on 4K pages the page bounds the threshold before the register does.
+ */
+#define ENET_SHINFO_LEN		SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
+#define ENET_THLD_PAGE_LEN	round_down(PAGE_SIZE - ENET_SHINFO_LEN - \
+					   sizeof(struct status_64), \
+					   ENET_THLD_BURST)
+#define ENET_THLD_MAX_LEN	min_t(unsigned int, \
+				      ENET_THLD_MAX * ENET_THLD_UNIT, \
+				      ENET_THLD_PAGE_LEN)
 
 /* Tx/Rx DMA register offset, skip 256 descriptors */
 #define WORDS_PER_BD(p)		(p->hw_params->words_per_bd)
@@ -2252,7 +2269,7 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
 			      struct enet_cb *cb)
 {
 	struct bcmgenet_priv *priv = ring->priv;
-	unsigned int size = GENET_RX_BUF_SIZE;
+	unsigned int size = SKB_HEAD_ALIGN(priv->rx_buf_len);
 	unsigned int offset;
 	dma_addr_t mapping;
 	struct page *page;
@@ -2267,7 +2284,7 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
 
 	/* page_pool handles DMA mapping via PP_FLAG_DMA_MAP */
 	mapping = page_pool_get_dma_addr(page) + offset;
-	dma_sync_single_for_device(&priv->pdev->dev, mapping, RX_BUF_LENGTH,
+	dma_sync_single_for_device(&priv->pdev->dev, mapping, priv->rx_buf_len,
 				   DMA_FROM_DEVICE);
 
 	cb->rx_page = page;
@@ -2346,10 +2363,10 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 		}
 
 		/* Sync the full buffer; the HW may have written anywhere
-		 * up to RX_BUF_LENGTH.
+		 * up to priv->rx_buf_len.
 		 */
 		page_pool_dma_sync_for_cpu(ring->page_pool, rx_page, rx_offset,
-					   RX_BUF_LENGTH);
+					   priv->rx_buf_len);
 
 		hard_start = page_address(rx_page) + rx_offset;
 		status = (struct status_64 *)hard_start;
@@ -2371,7 +2388,7 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 					     : sizeof(struct status_64);
 
 		/* Reject lengths that would underflow the SKB build path. */
-		if (unlikely(len > RX_BUF_LENGTH || len < min_len)) {
+		if (unlikely(len > priv->rx_buf_len || len < min_len)) {
 			netif_err(priv, rx_status, dev,
 				  "invalid packet length %d\n", len);
 			BCMGENET_STATS64_INC(stats, length_errors);
@@ -2622,6 +2639,44 @@ static void bcmgenet_link_intr_enable(struct bcmgenet_priv *priv)
 	bcmgenet_intrl2_0_writel(priv, int0_enable, INTRL2_CPU_MASK_CLEAR);
 }
 
+/* Receive threshold in register units. Covers the alignment bytes and the
+ * frame, but not the status block, which the hardware adds on top.
+ */
+static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
+{
+	unsigned int len = GENET_RBUF_ALIGN + mtu + ETH_HLEN + VLAN_HLEN;
+
+	len = round_up(len, ENET_THLD_BURST) / ENET_THLD_UNIT;
+
+	/* Keep the reset default for the common MTUs */
+	return clamp_t(unsigned int, len, ENET_THLD_DEFAULT,
+		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
+}
+
+/* A buffer has to hold everything the threshold lets the hardware deliver */
+static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
+{
+	return sizeof(struct status_64) +
+	       bcmgenet_pkt_rdy_thld(mtu) * ENET_THLD_UNIT;
+}
+
+/* Program the MTU dependent registers. Call with the MAC disabled. */
+static void bcmgenet_set_mtu_regs(struct bcmgenet_priv *priv, unsigned int mtu)
+{
+	u32 thld = bcmgenet_pkt_rdy_thld(mtu);
+
+	bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN, UMAC_MAX_FRAME_LEN);
+
+	/* GENET v1 maps other registers at these offsets */
+	if (GENET_IS_V1(priv))
+		return;
+
+	bcmgenet_rbuf_writel(priv, thld, RBUF_PKT_RDY_THLD);
+	bcmgenet_writel(ENET_THLD_MAX,
+			priv->base + priv->hw_params->tbuf_offset +
+			TBUF_PKT_RDY_THLD);
+}
+
 static void init_umac(struct bcmgenet_priv *priv)
 {
 	struct device *kdev = &priv->pdev->dev;
@@ -2638,7 +2693,7 @@ static void init_umac(struct bcmgenet_priv *priv)
 			     UMAC_MIB_CTRL);
 	bcmgenet_umac_writel(priv, 0, UMAC_MIB_CTRL);
 
-	bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN, UMAC_MAX_FRAME_LEN);
+	bcmgenet_set_mtu_regs(priv, priv->dev->mtu);
 
 	/* init tx registers, enable TSB */
 	reg = bcmgenet_tbuf_ctrl_get(priv);
@@ -2754,7 +2809,7 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 				  TDMA_FLOW_PERIOD);
 	bcmgenet_tdma_ring_writel(priv, index,
 				  ((size << DMA_RING_SIZE_SHIFT) |
-				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
+				   priv->rx_buf_len), DMA_RING_BUF_SIZE);
 
 	/* Set start and end address, read and write pointers */
 	bcmgenet_tdma_ring_writel(priv, index, start_ptr * words_per_bd,
@@ -2837,7 +2892,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
 	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_CONS_INDEX);
 	bcmgenet_rdma_ring_writel(priv, index,
 				  ((size << DMA_RING_SIZE_SHIFT) |
-				   RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
+				   priv->rx_buf_len), DMA_RING_BUF_SIZE);
 	bcmgenet_rdma_ring_writel(priv, index,
 				  (DMA_FC_THRESH_LO <<
 				   DMA_XOFF_THRESHOLD_SHIFT) |
@@ -4106,6 +4161,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	/* Mii wait queue */
 	init_waitqueue_head(&priv->wq);
 	bcmgenet_hfb_init(priv);
+	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
 	INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);
 
 	priv->clk_wol = devm_clk_get_optional(&priv->pdev->dev, "enet-wol");
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 501dd1256693..6444bac168c3 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -218,6 +218,8 @@ struct bcmgenet_rx_stats64 {
 #define  RBUF_ALIGN_2B			(1 << 1)
 #define  RBUF_BAD_DIS			(1 << 2)
 
+#define RBUF_PKT_RDY_THLD		0x08
+
 #define RBUF_STATUS			0x0C
 #define  RBUF_STATUS_WOL		(1 << 0)
 #define  RBUF_STATUS_MPD_INTR_ACTIVE	(1 << 1)
@@ -248,6 +250,7 @@ struct bcmgenet_rx_stats64 {
 #define TBUF_CTRL			0x00
 #define  TBUF_64B_EN			(1 << 0)
 #define TBUF_BP_MC			0x0C
+#define TBUF_PKT_RDY_THLD		0x10
 #define TBUF_ENERGY_CTRL		0x14
 #define  TBUF_EEE_EN			(1 << 0)
 #define  TBUF_PM_EN			(1 << 1)
@@ -612,6 +615,7 @@ struct bcmgenet_priv {
 	void __iomem *rx_bds;
 	struct enet_cb *rx_cbs;
 	unsigned int num_rx_bds;
+	unsigned int rx_buf_len;
 	struct bcmgenet_rxnfc_rule rxnfc_rules[MAX_NUM_OF_FS_RULES];
 	struct list_head rxnfc_list;
 

-- 
2.53.0

Re: [PATCH net-next 4/7] net: bcmgenet: derive the receive buffer length from the MTU

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The ENET_THLD_PAGE_LEN comment added by this patch says 'A
  descriptor is one page, which also holds skb_shared_info behind the…

--

net: bcmgenet: derive the receive buffer length from the MTU

This patch replaces the fixed 2048 byte RX buffer length with one
computed from the MTU through a new RBUF packet ready threshold, plus the
64 byte status block. The patch also programs RBUF_PKT_RDY_THLD and sets
TBUF_PKT_RDY_THLD to its maximum. A new priv->rx_buf_len replaces
RX_BUF_LENGTH in the refill, sync, length check and ring setup paths.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 5cb3d25482a0d..bf889558f6ade 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> -/* RX buffer plus the skb_shared_info napi_build_skb() places behind it */
> -#define GENET_RX_BUF_SIZE	SKB_HEAD_ALIGN(RX_BUF_LENGTH)
> +/* A descriptor is one page, which also holds skb_shared_info behind the frame,
> + * so on 4K pages the page bounds the threshold before the register does.
> + */

[Severity: Low]
This isn't a bug, but is "A descriptor is one page" accurate on every page
size?

bcmgenet_rx_refill() asks page_pool_dev_alloc() for
SKB_HEAD_ALIGN(priv->rx_buf_len) bytes. page_pool returns a whole page at
offset 0 only when twice the request is larger than the page. Otherwise it
returns a fragment.

rx_buf_len is 2112 with the default MTU, so on 4K pages each descriptor
gets a whole page. On 16K or 64K pages, several descriptors share one page.

This comment also seems to contradict the existing one in
bcmgenet_rx_ring_create_pool() in the same file:

	/* Buffers share a page. bcmgenet_rx_refill() syncs each one for the
	 * device, PP_FLAG_DMA_SYNC_DEV would sync the whole page.
	 */

That one is wrong for 4K pages, and the new one is wrong for larger pages.
Neither comment changes later in the series.

The ENET_THLD_PAGE_LEN bound itself looks fine, since a buffer never
exceeds one page. Could both comments be reworded so they agree and say
that buffers may share a page depending on PAGE_SIZE?

> +#define ENET_SHINFO_LEN		SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
> +#define ENET_THLD_PAGE_LEN	round_down(PAGE_SIZE - ENET_SHINFO_LEN - \
> +					   sizeof(struct status_64), \
> +					   ENET_THLD_BURST)
> +#define ENET_THLD_MAX_LEN	min_t(unsigned int, \
> +				      ENET_THLD_MAX * ENET_THLD_UNIT, \
> +				      ENET_THLD_PAGE_LEN)

[ ... ]

> @@ -2252,7 +2269,7 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
>  			      struct enet_cb *cb)
>  {
>  	struct bcmgenet_priv *priv = ring->priv;
> -	unsigned int size = GENET_RX_BUF_SIZE;
> +	unsigned int size = SKB_HEAD_ALIGN(priv->rx_buf_len);
>  	unsigned int offset;
>  	dma_addr_t mapping;
>  	struct page *page;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de

[PATCH net-next 5/7] net: bcmgenet: allow the MTU to be changed

[Nicolai Buchwitz]

The driver never sets dev->max_mtu, so the MTU is stuck at ETH_DATA_LEN.

One descriptor reaches as far as the packet ready threshold, so derive the
maximum from it. The threshold registers are 8 bit in units of 16 bytes and
want a multiple of the 256 byte burst size. A descriptor is one page and
also holds skb_shared_info behind the frame. On 4K pages the page is the
tighter limit and leaves 3564 bytes. That includes room for a VLAN tag so a
VLAN interface can run at the parent MTU.

Resize the buffers and rewrite the registers in place. The PHY keeps
running and the link stays up.

A failed allocation retries at the previous size. If that fails too, take
the interface down. Running on rings that were never allocated is worse.

Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Link: https://github.com/raspberrypi/linux/issues/5561
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
Tested-by: Pierre-Marin Leclercq <pierremarinleclercq88@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 82 +++++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  9 ++-
 2 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index bf889558f6ad..4634b6521ab8 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -77,6 +77,12 @@
 				      ENET_THLD_MAX * ENET_THLD_UNIT, \
 				      ENET_THLD_PAGE_LEN)
 
+/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN
+ * interface can use the parent MTU.
+ */
+#define ENET_MAX_MTU		(ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \
+				 ETH_HLEN - VLAN_HLEN)
+
 /* Tx/Rx DMA register offset, skip 256 descriptors */
 #define WORDS_PER_BD(p)		(p->hw_params->words_per_bd)
 #define DMA_DESC_SIZE		(WORDS_PER_BD(priv) * sizeof(u32))
@@ -2665,7 +2671,7 @@ static void bcmgenet_set_mtu_regs(struct bcmgenet_priv *priv, unsigned int mtu)
 {
 	u32 thld = bcmgenet_pkt_rdy_thld(mtu);
 
-	bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN, UMAC_MAX_FRAME_LEN);
+	bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN);
 
 	/* GENET v1 maps other registers at these offsets */
 	if (GENET_IS_V1(priv))
@@ -2799,7 +2805,7 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 
 	/* Set flow period for ring != 0 */
 	if (index)
-		flow_period_val = ENET_MAX_FRAME_LEN << 16;
+		flow_period_val = ENET_MAX_FRAME_LEN(priv->dev->mtu) << 16;
 
 	bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_PROD_INDEX);
 	bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_CONS_INDEX);
@@ -3491,6 +3497,7 @@ static int bcmgenet_open(struct net_device *dev)
 
 	bcmgenet_netif_start(dev, true);
 
+	priv->datapath_up = true;
 	netif_tx_start_all_queues(dev);
 
 	return 0;
@@ -3549,7 +3556,11 @@ static int bcmgenet_close(struct net_device *dev)
 
 	netif_dbg(priv, ifdown, dev, "bcmgenet_close\n");
 
-	bcmgenet_netif_stop(dev, false);
+	/* A failed MTU change can have torn the datapath down already */
+	if (priv->datapath_up) {
+		bcmgenet_netif_stop(dev, false);
+		priv->datapath_up = false;
+	}
 
 	/* Really kill the PHY state machine and disconnect from it */
 	phy_disconnect(dev->phydev);
@@ -3797,6 +3808,66 @@ static int bcmgenet_change_carrier(struct net_device *dev, bool new_carrier)
 	return 0;
 }
 
+static int bcmgenet_change_mtu(struct net_device *dev, int new_mtu)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	unsigned int old_mtu = dev->mtu;
+	int ret;
+
+	if (!netif_running(dev)) {
+		WRITE_ONCE(dev->mtu, new_mtu);
+		priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
+		return 0;
+	}
+
+	/* The watchdog trips on an idle queue once the rings are gone */
+	netif_device_detach(dev);
+
+	/* Only the buffers and the MTU registers change, leave the PHY up */
+	bcmgenet_netif_stop(dev, false);
+	priv->datapath_up = false;
+
+	WRITE_ONCE(dev->mtu, new_mtu);
+	priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
+	bcmgenet_set_mtu_regs(priv, new_mtu);
+
+	ret = bcmgenet_init_dma(priv, true);
+	if (ret) {
+		/* Retry the size that was allocated a moment ago */
+		WRITE_ONCE(dev->mtu, old_mtu);
+		priv->rx_buf_len = bcmgenet_rx_buf_len(old_mtu);
+		bcmgenet_set_mtu_regs(priv, old_mtu);
+		if (bcmgenet_init_dma(priv, true)) {
+			/* Nothing left to run on. Take the interface down so
+			 * that close and suspend do not tear it down twice.
+			 */
+			netdev_err(dev, "failed to restore MTU %u, closing\n",
+				   old_mtu);
+			netif_close(dev);
+
+			/* Mark the device present again, __dev_open()
+			 * refuses a detached one. The queues stay stopped
+			 * because the interface is down by now.
+			 */
+			netif_device_attach(dev);
+			return ret;
+		}
+	}
+
+	bcmgenet_hfb_restore(priv);
+	bcmgenet_netif_start(dev, false);
+
+	/* bcmgenet_netif_start() only restores the link interrupt */
+	if (bcmgenet_has_mdio_intr(priv))
+		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_MDIO_EVENT,
+					 INTRL2_CPU_MASK_CLEAR);
+
+	priv->datapath_up = true;
+	netif_device_attach(dev);
+
+	return ret;
+}
+
 static const struct net_device_ops bcmgenet_netdev_ops = {
 	.ndo_open		= bcmgenet_open,
 	.ndo_stop		= bcmgenet_close,
@@ -3808,6 +3879,7 @@ static const struct net_device_ops bcmgenet_netdev_ops = {
 	.ndo_set_features	= bcmgenet_set_features,
 	.ndo_get_stats64	= bcmgenet_get_stats64,
 	.ndo_change_carrier	= bcmgenet_change_carrier,
+	.ndo_change_mtu		= bcmgenet_change_mtu,
 };
 
 /* GENET hardware parameters/characteristics */
@@ -4161,7 +4233,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	/* Mii wait queue */
 	init_waitqueue_head(&priv->wq);
 	bcmgenet_hfb_init(priv);
+
+	/* v1 cannot program the thresholds, so it stays at the default MTU */
 	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
+	if (!GENET_IS_V1(priv))
+		dev->max_mtu = ENET_MAX_MTU;
 	INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);
 
 	priv->clk_wol = devm_clk_get_optional(&priv->pdev->dev, "enet-wol");
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 6444bac168c3..f535490547cf 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -31,9 +31,11 @@
 #define ENET_BRCM_TAG_LEN	6
 #define ENET_PAD		8
 
-/* Longest frame the MAC must accept for the default MTU */
-#define ENET_MAX_FRAME_LEN	(ETH_DATA_LEN + ETH_HLEN + VLAN_HLEN + \
-				 ENET_BRCM_TAG_LEN + ETH_FCS_LEN + ENET_PAD)
+/* Longest frame the MAC must accept for a given MTU */
+#define ENET_FRAME_OVERHEAD	(ETH_HLEN + VLAN_HLEN + ENET_BRCM_TAG_LEN + \
+				 ETH_FCS_LEN + ENET_PAD)
+#define ENET_MAX_FRAME_LEN(mtu)	((mtu) + ENET_FRAME_OVERHEAD)
+
 #define DMA_MAX_BURST_LENGTH    0x10
 
 /* misc. configuration */
@@ -627,6 +629,7 @@ struct bcmgenet_priv {
 	unsigned autoneg_pause:1;
 	unsigned tx_pause:1;
 	unsigned rx_pause:1;
+	unsigned datapath_up:1;
 
 	/* MDIO bus variables */
 	wait_queue_head_t wq;

-- 
2.53.0

Re: [PATCH net-next 5/7] net: bcmgenet: allow the MTU to be changed

[netdev-bot+sashiko]

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

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

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 this patch, a single RX descriptor is sized for
  GENET_RBUF_ALIGN + MTU + ETH_HLEN + VLAN_HLEN; this applies to…
- [Medium] bcmgenet_change_mtu() leaves the PHY state machine running but
  reuses bcmgenet_netif_stop(dev, false).
- [Low] The commit message does not describe all of the behaviour this
  patch changes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  datapath_up flag does not cover it.

--

net: bcmgenet: allow the MTU to be changed

This commit sets dev->max_mtu to the largest MTU that fits in one RX
descriptor and adds an ndo_change_mtu handler. On a running interface the
handler resizes the rings and rewrites the MTU registers while leaving the
PHY up. If the allocation fails, it retries at the old size and closes the
interface when that also fails.

Link: https://github.com/raspberrypi/linux/issues/5561

> The driver never sets dev->max_mtu, so the MTU is stuck at ETH_DATA_LEN.

[Severity: Low]
Is this accurate? ether_setup() leaves min_mtu at 68. Before this patch
there was no ndo_change_mtu, so userspace could already lower the MTU.
That changed only dev->mtu and never touched the hardware.

With this patch, lowering the MTU has effects the commit message does not
mention.

bcmgenet_set_mtu_regs() now writes ENET_MAX_FRAME_LEN(mtu) to
UMAC_MAX_FRAME_LEN. The MAC receive limit therefore drops below the old
fixed 1536 bytes, for example to 1436 for an MTU of 1400. This also
happens on v1, because the write comes before the GENET_IS_V1() return.

On a running interface, bcmgenet_change_mtu() always runs the full
netif_device_detach() -> bcmgenet_netif_stop() -> bcmgenet_init_dma() ->
bcmgenet_netif_start() sequence. It does this even when
bcmgenet_rx_buf_len() returns the same size. That is the case for every
MTU up to about 2028, because bcmgenet_pkt_rdy_thld() clamps to
ENET_THLD_DEFAULT.

So a plain decrease interrupts traffic. It can also end in netif_close()
if both init_dma attempts fail.

GENET v1 gets the same ndo_change_mtu and the same rebuild on a decrease.
The new probe comment "v1 cannot program the thresholds, so it stays at
the default MTU" only holds for the maximum.

Could the commit message and the probe comment describe this? Could the
rebuild be skipped when rx_buf_len does not change?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index bf889558f6ade..4634b6521ab8e 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -77,6 +77,12 @@
>  				      ENET_THLD_MAX * ENET_THLD_UNIT, \
>  				      ENET_THLD_PAGE_LEN)
>  
> +/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN
> + * interface can use the parent MTU.
> + */
> +#define ENET_MAX_MTU		(ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \
> +				 ETH_HLEN - VLAN_HLEN)

[Severity: Medium]
Does one descriptor really hold the largest frame the MAC accepts at this
MTU?

bcmgenet_pkt_rdy_thld() and bcmgenet_rx_buf_len() size a descriptor for
GENET_RBUF_ALIGN + mtu + ETH_HLEN + VLAN_HLEN. However,
bcmgenet_set_mtu_regs() programs UMAC_MAX_FRAME_LEN to
mtu + ENET_FRAME_OVERHEAD (mtu + 36). That also counts the FCS, the
Broadcom tag and the padding.

With 4K pages, ENET_MAX_MTU is 3564 and the threshold is exactly 3584, so
there is no slack. Two kinds of frame are accepted by the MAC but end up
spanning two descriptors:

- a QinQ frame at full MTU (2 + 3564 + 22 = 3588 bytes)
- a VLAN tagged frame with a forwarded FCS (crc_fwd_en)

bcmgenet_desc_rx() then drops them:

	if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
		netif_err(priv, rx_status, dev,
			  "dropping fragmented packet!\n");

The same happens just below each 256 byte threshold step, for example at
MTU 2025-2028.

Later in the series, "net: bcmgenet: reassemble jumbo frames from status
block fragments" seems to fix this. It removes ENET_MAX_MTU, uses
ENET_MAX_JUMBO_MTU and reassembles frames that span several descriptors.

As of this patch, though, are these frames silently dropped and counted
as fragmented_errors?

> +
>  /* Tx/Rx DMA register offset, skip 256 descriptors */
>  #define WORDS_PER_BD(p)		(p->hw_params->words_per_bd)
>  #define DMA_DESC_SIZE		(WORDS_PER_BD(priv) * sizeof(u32))

[ ... ]

> @@ -3549,7 +3556,11 @@ static int bcmgenet_close(struct net_device *dev)
>  
>  	netif_dbg(priv, ifdown, dev, "bcmgenet_close\n");
>  
> -	bcmgenet_netif_stop(dev, false);
> +	/* A failed MTU change can have torn the datapath down already */
> +	if (priv->datapath_up) {
> +		bcmgenet_netif_stop(dev, false);
> +		priv->datapath_up = false;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but the new flag doesn't cover
it. datapath_up is only updated in open, close and change_mtu. The
suspend and resume paths never touch it.

A non-WoL bcmgenet_suspend() calls bcmgenet_netif_stop(dev, true). That
disables every TX and RX NAPI and then runs bcmgenet_fini_dma(), which
calls netif_napi_del() and frees priv->rx_cbs and priv->tx_cbs without
setting them to NULL. datapath_up stays set.

The WoL branch of resume that finds the MAC was reset also calls
bcmgenet_fini_dma() directly.

bcmgenet_resume() can then fail in bcmgenet_init_dma(), from a DMA halt
timeout or an allocation failure:

	ret = bcmgenet_init_dma(priv, false);
	if (ret) {
		netdev_err(dev, "failed to initialize DMA\n");
		goto out_clk_disable;
	}

In that case it returns with the clock off, netif_running() still true
and datapath_up still set.

A later bcmgenet_close() takes the new branch and stops the datapath a
second time:

bcmgenet_close()
  bcmgenet_netif_stop()
    bcmgenet_disable_tx_napi()
      napi_disable()
        napi_disable_locked()
          while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC))
                  usleep_range(20, 200);

Can this loop forever while holding RTNL and the netdev instance lock? If
it ever got past the loop, bcmgenet_fini_dma() would free the stale
rx_cbs and tx_cbs a second time.

With this patch, bcmgenet_change_mtu() on such a device reaches the same
loop through its own bcmgenet_netif_stop() call.

Should suspend and the resume error paths also keep datapath_up up to
date?

>  
>  	/* Really kill the PHY state machine and disconnect from it */
>  	phy_disconnect(dev->phydev);

[ ... ]

> @@ -3797,6 +3808,66 @@ static int bcmgenet_change_carrier(struct net_device *dev, bool new_carrier)
>  	return 0;
>  }
>  
> +static int bcmgenet_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +	struct bcmgenet_priv *priv = netdev_priv(dev);
> +	unsigned int old_mtu = dev->mtu;
> +	int ret;
> +
> +	if (!netif_running(dev)) {
> +		WRITE_ONCE(dev->mtu, new_mtu);
> +		priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
> +		return 0;
> +	}
> +
> +	/* The watchdog trips on an idle queue once the rings are gone */
> +	netif_device_detach(dev);
> +
> +	/* Only the buffers and the MTU registers change, leave the PHY up */
> +	bcmgenet_netif_stop(dev, false);

[Severity: Medium]
The commit message says "The PHY keeps running and the link stays up".
Can a link event be lost here, though?

bcmgenet_netif_stop() calls bcmgenet_intr_disable(), which does:

	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_MASK_SET);
	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_CLEAR);

It then calls cancel_work_sync(&priv->bcmgenet_irq_work).

A UMAC_IRQ_LINK_EVENT or UMAC_IRQ_PHY_DET_R that has latched but not been
handled can be lost in two ways:

- the CLEAR wipes it before bcmgenet_isr0() reads it
- bcmgenet_isr0() has already saved it in priv->irq0_stat, but the work
  is cancelled before bcmgenet_irq_task() runs

For internal PHYs on GENET v1-v4, bcmgenet_mii_probe() does:

	dev->phydev->irq = PHY_MAC_INTERRUPT;

So phylib never polls. The only notification path is
bcmgenet_irq_task() -> phy_mac_interrupt().

Nothing on the restart path re-triggers the PHY state machine. That path
is bcmgenet_netif_start(dev, false) plus re-enabling MDIO_EVENT.

Would phylib's link state and the carrier then stay stale until the next
link event? One example is carrier still up after the cable is pulled.

A lost PHY_DET_R on v1-v3 with forced speed would also skip the
phy_init_hw() and genphy_config_aneg() calls in bcmgenet_irq_task().

Before this patch, bcmgenet_netif_stop() was only used when the PHY was
also being stopped or disconnected. Is a phy_mac_interrupt() or
phy_trigger_machine() call needed after the restart?

> +	priv->datapath_up = false;
> +
> +	WRITE_ONCE(dev->mtu, new_mtu);
> +	priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
> +	bcmgenet_set_mtu_regs(priv, new_mtu);
> +
> +	ret = bcmgenet_init_dma(priv, true);

[ ... ]

> +	bcmgenet_hfb_restore(priv);
> +	bcmgenet_netif_start(dev, false);
> +
> +	/* bcmgenet_netif_start() only restores the link interrupt */
> +	if (bcmgenet_has_mdio_intr(priv))
> +		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_MDIO_EVENT,
> +					 INTRL2_CPU_MASK_CLEAR);
> +
> +	priv->datapath_up = true;
> +	netif_device_attach(dev);
> +
> +	return ret;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de

[PATCH net-next 6/7] net: bcmgenet: pad transmit frames out of the packet ready window

[Nicolai Buchwitz]

A frame that ends a few bytes past the transmit packet ready threshold
stops the transmitter as soon as a shorter frame follows. Tx DMA then
refuses to halt, so every later bcmgenet_init_dma() fails and the interface
cannot be opened again. IP fragmentation generates that pattern on every
datagram, full frames and a short tail.

The window starts one byte past the threshold and widens with it. On a CM4
it ends 28, 32 and 46 bytes past thresholds of 2560, 3584 and 3840. Link
speed makes no difference. Pad frames landing in it to 64 bytes past the
threshold.

Padding must not push a frame past what the peer accepts. Linux does not
bound how many VLAN tags a frame carries, so measure against the longest
frame the MAC has to accept rather than a tag count. For the MTUs where
such a frame would land in the window, lower the threshold instead. All
other MTUs keep the register maximum.

Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
Tested-by: Pierre-Marin Leclercq <pierremarinleclercq88@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 39 ++++++++++++++++++++++++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 4634b6521ab8..0cd56ec1c5cc 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -59,6 +59,11 @@
 #define ENET_THLD_DEFAULT	0x80
 #define ENET_THLD_MAX		0xf0
 
+/* A frame ending just past the transmit threshold stops the transmitter once
+ * a shorter frame follows, so pad frames that land there this far past it.
+ */
+#define ENET_TX_SAFE_MARGIN	64
+
 /* Page pool RX buffer layout:
  * RSB(64) + pad(2) | frame data | skb_shared_info
  * The HW writes the 64B RSB + 2B alignment padding before the frame.
@@ -2171,6 +2176,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
+	/* Keep the frame out of the window just past the threshold */
+	if (unlikely(skb->len > priv->tx_thld_len &&
+		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
+		if (skb_put_padto(skb, priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
+			BCMGENET_STATS64_INC((&ring->stats64), dropped);
+			ret = NETDEV_TX_OK;
+			goto out;
+		}
+	}
+
+	nr_frags = skb_shinfo(skb)->nr_frags;
+
 	/* Retain how many bytes will be sent on the wire, without TSB inserted
 	 * by transmit checksum offload
 	 */
@@ -2659,6 +2676,23 @@ static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
 		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
 }
 
+/* Transmit threshold in register units. Frames landing in the window just
+ * past it are padded clear of it, so pick a threshold that leaves room for
+ * that padding inside the frame the MTU allows. Size the window against the
+ * longest frame the MAC has to accept, since the tag count is not bounded.
+ */
+static unsigned int bcmgenet_tx_pkt_rdy_thld(unsigned int mtu)
+{
+	unsigned int thld = ENET_THLD_MAX;
+
+	while (thld > ENET_THLD_DEFAULT &&
+	       ENET_MAX_FRAME_LEN(mtu) - ETH_FCS_LEN > thld * ENET_THLD_UNIT &&
+	       thld * ENET_THLD_UNIT + ENET_TX_SAFE_MARGIN > mtu + ETH_HLEN)
+		thld -= ENET_THLD_BURST / ENET_THLD_UNIT;
+
+	return thld;
+}
+
 /* A buffer has to hold everything the threshold lets the hardware deliver */
 static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
 {
@@ -2669,8 +2703,10 @@ static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
 /* Program the MTU dependent registers. Call with the MAC disabled. */
 static void bcmgenet_set_mtu_regs(struct bcmgenet_priv *priv, unsigned int mtu)
 {
+	u32 tx_thld = bcmgenet_tx_pkt_rdy_thld(mtu);
 	u32 thld = bcmgenet_pkt_rdy_thld(mtu);
 
+	priv->tx_thld_len = tx_thld * ENET_THLD_UNIT;
 	bcmgenet_umac_writel(priv, ENET_MAX_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN);
 
 	/* GENET v1 maps other registers at these offsets */
@@ -2678,8 +2714,7 @@ static void bcmgenet_set_mtu_regs(struct bcmgenet_priv *priv, unsigned int mtu)
 		return;
 
 	bcmgenet_rbuf_writel(priv, thld, RBUF_PKT_RDY_THLD);
-	bcmgenet_writel(ENET_THLD_MAX,
-			priv->base + priv->hw_params->tbuf_offset +
+	bcmgenet_writel(tx_thld, priv->base + priv->hw_params->tbuf_offset +
 			TBUF_PKT_RDY_THLD);
 }
 
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index f535490547cf..eb00cff0e732 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -618,6 +618,7 @@ struct bcmgenet_priv {
 	struct enet_cb *rx_cbs;
 	unsigned int num_rx_bds;
 	unsigned int rx_buf_len;
+	unsigned int tx_thld_len;
 	struct bcmgenet_rxnfc_rule rxnfc_rules[MAX_NUM_OF_FS_RULES];
 	struct list_head rxnfc_list;
 

-- 
2.53.0

Re: [PATCH net-next 6/7] net: bcmgenet: pad transmit frames out of the packet ready window

[netdev-bot+sashiko]

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

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

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] bcmgenet_xmit() pads every frame whose length falls in
  (tx_thld_len, tx_thld_len + 64) with up to 63 zero bytes via…
- [Medium] (Baseline: resolved later in the series.) At this commit,
  bcmgenet_tx_pkt_rdy_thld() lowers TBUF_PKT_RDY_THLD from 3840 to 3584…
- [Low] When skb_put_padto() fails, the new error path in bcmgenet_xmit()
  jumps to 'out' and skips the TDMA_PROD_INDEX doorbell write.

--

net: bcmgenet: pad transmit frames out of the packet ready window

This patch pads transmit frames that end just past the TX packet ready
threshold, so they land 64 bytes past it. The window causes the
transmitter to stop and Tx DMA to refuse to halt. For MTUs where a
maximum-length frame would fall inside that window, the patch lowers
the TX threshold instead.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 4634b6521ab8e..0cd56ec1c5ccf 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2171,6 +2176,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  	}
>  
> +	/* Keep the frame out of the window just past the threshold */
> +	if (unlikely(skb->len > priv->tx_thld_len &&
> +		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
> +		if (skb_put_padto(skb, priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {

[Severity: Medium]
Can this padding corrupt frames that end in a trailer?

skb_put_padto() appends up to 63 zero bytes after whatever is already at
the end of the frame. It does not check what the frame carries. Some
encapsulations put data at the very end of the frame:

DSA tail taggers (tag_ksz, tag_trailer, tag_hellcreek, tag_xrs700x,
rtl8_4t): the switch would read the zeros as the tag. That is why
dsa_user_xmit() pads before the tail tagger runs:

net/dsa/user.c:dsa_user_xmit() {
    ...
	if (dev->needed_tailroom)
		eth_skb_pad(skb);
    ...
}

PRP: the redundancy control trailer would no longer be the last 6
bytes. The receiver would then treat the frame as non-PRP, and duplicate
discard would fail.

Software MACsec: macsec_decrypt() finds the ICV from the received
length:

	aead_request_set_ad(req, skb->len - icv_len);

so the trailing zeros would make authentication fail.

At this commit, on kernels with 16K or larger pages and an MTU of
3809..3820, the threshold drops to 3584. Frames of 3585..3647 bytes then
get padded.

Later in the series, "net: bcmgenet: reassemble jumbo frames from status
block fragments" raises max_mtu to ENET_MAX_JUMBO_MTU on all page sizes.
After that, any MTU of 3890 or more pads every frame of 3841..3903
bytes. MTUs of 3809..3889 pad frames of 3585..3647 bytes. Nothing later
in the series appears to fix this.

Should frames that carry a trailer be kept out of this window some other
way, rather than by appending bytes?

> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
> +			ret = NETDEV_TX_OK;
> +			goto out;

[Severity: Low]
If skb_put_padto() fails, this jumps to out. That skips the
TDMA_PROD_INDEX write at the end of bcmgenet_xmit():

	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
		/* Packets are ready, update producer index */
		bcmgenet_tdma_ring_writel(priv, ring->index,
					  ring->prod_index, TDMA_PROD_INDEX);

Suppose earlier skbs in the same xmit_more batch already advanced
ring->prod_index, and the dropped skb was the last one in the batch.
Could those descriptors sit unsubmitted until a later transmit
succeeds? The queue is not stopped, so the TX watchdog would not
notice.

skb_put_padto() can fail when the skb is short on tailroom or is
cloned, and the GFP_ATOMIC expansion fails. The existing
bcmgenet_add_tsb() and DMA mapping failure paths already behave this
way. This patch adds another way to reach that state.

> +		}
> +	}
> +
> +	nr_frags = skb_shinfo(skb)->nr_frags;
> +

[ ... ]

> @@ -2659,6 +2676,23 @@ static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
>  		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
>  }
>  
> +/* Transmit threshold in register units. Frames landing in the window just
> + * past it are padded clear of it, so pick a threshold that leaves room for
> + * that padding inside the frame the MTU allows. Size the window against the
> + * longest frame the MAC has to accept, since the tag count is not bounded.
> + */
> +static unsigned int bcmgenet_tx_pkt_rdy_thld(unsigned int mtu)
> +{
> +	unsigned int thld = ENET_THLD_MAX;
> +
> +	while (thld > ENET_THLD_DEFAULT &&
> +	       ENET_MAX_FRAME_LEN(mtu) - ETH_FCS_LEN > thld * ENET_THLD_UNIT &&
> +	       thld * ENET_THLD_UNIT + ENET_TX_SAFE_MARGIN > mtu + ETH_HLEN)
> +		thld -= ENET_THLD_BURST / ENET_THLD_UNIT;

[Severity: Medium]
For an MTU of 3809..3820, this loop lowers TBUF_PKT_RDY_THLD from 3840
to 3584 bytes. At this commit that range is only reachable on kernels
with 16K or larger pages, where ENET_MAX_MTU is 3820.

bcmgenet_xmit() still sets DMA_TX_DO_CSUM for every CHECKSUM_PARTIAL
frame:

		if (!i) {
			len_stat |= DMA_SOP;
			if (skb->ip_summed == CHECKSUM_PARTIAL)
				len_stat |= DMA_TX_DO_CSUM;
		}

Full-size TCP/UDP frames of 3585..3834 bytes now exceed the transmit
threshold. Before this commit, no frame could.

The last patch in the series, "net: bcmgenet: reassemble jumbo frames
from status block fragments", says the MAC only holds threshold bytes
for checksum insertion and silently drops longer frames. It adds this
fallback:

	if (unlikely(skb->len > priv->tx_thld_len) &&
	    skb->ip_summed == CHECKSUM_PARTIAL) {
		if (skb_checksum_help(skb)) {

Between this patch and that one, are CHECKSUM_PARTIAL frames longer
than 3584 bytes dropped when NETIF_F_HW_CSUM is enabled, which is the
default?

The end of the series fixes this, but bisection can still land in
between. Could the skb_checksum_help() fallback move into this patch?

> +
> +	return thld;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de

Re: [PATCH net-next 6/7] net: bcmgenet: pad transmit frames out of the packet ready window

[Nicolai Buchwitz]

On 5.10.2026 17:04, netdev-bot+sashiko@kernel.org wrote:

> [...]

> At this commit, on kernels with 16K or larger pages and an MTU of
> 3809..3820, the threshold drops to 3584. Frames of 3585..3647 bytes 
> then
> get padded.
> 
> Later in the series, "net: bcmgenet: reassemble jumbo frames from 
> status
> block fragments" raises max_mtu to ENET_MAX_JUMBO_MTU on all page 
> sizes.
> After that, any MTU of 3890 or more pads every frame of 3841..3903
> bytes. MTUs of 3809..3889 pad frames of 3585..3647 bytes. Nothing later
> in the series appears to fix this.
> 
> Should frames that carry a trailer be kept out of this window some 
> other
> way, rather than by appending bytes?

I do not see a way. Any frame length can fall in the window, so moving 
the
threshold only moves the window. Keeping every frame below the threshold
caps the MTU at 3808 and removes the point of the series.

Worth noting the window is unreachable below MTU 3809. At the default 
MTU
the longest frame is 1532 against a threshold of 3840, so nothing is 
ever
padded. This only affects jumbo configurations!

The alternatives are dropping those frames or leaving the transmitter
stalled until the interface is reopened. Padding seemed the least bad, 
but
I will note the trailer limitation in the commit message.

> [...]

For the other findings I will respin anyway, so:

---
pw-bot: cr

[PATCH net-next 7/7] net: bcmgenet: reassemble jumbo frames from status block fragments

[Nicolai Buchwitz]

The hardware does not truncate a frame longer than the packet ready
threshold. It splits the frame across descriptors and writes a status block
at the start of each one. The first fragment then arrives with SOP and no
EOP and is dropped as fragmented. This caps the MTU.

Strip the status blocks and reassemble the fragments. Only the last block
holds the checksum of the whole frame. Broadcom confirmed from the RTL that
every GENET revision splits long frames this way, not just the v5 this was
tested on.

The MAC only checksums a frame it holds in full, so anything longer than
the threshold falls back to software. At jumbo sizes the larger frame saves
more per packet overhead than the checksum costs.

UMAC_MAX_FRAME_LEN is 14 bit and counts the FCS. That puts the maximum MTU
at 16347.

Suggested-by: Justin Chen <justin.chen@broadcom.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
Tested-by: Pierre-Marin Leclercq <pierremarinleclercq88@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 106 ++++++++++++++++++++++---
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   2 +
 2 files changed, 98 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0cd56ec1c5cc..62edbe51fe07 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -82,11 +82,8 @@
 				      ENET_THLD_MAX * ENET_THLD_UNIT, \
 				      ENET_THLD_PAGE_LEN)
 
-/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN
- * interface can use the parent MTU.
- */
-#define ENET_MAX_MTU		(ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \
-				 ETH_HLEN - VLAN_HLEN)
+/* UMAC_MAX_FRAME_LEN is 14 bits wide and counts the FCS */
+#define ENET_MAX_JUMBO_MTU	(GENMASK(13, 0) - ENET_FRAME_OVERHEAD)
 
 /* Tx/Rx DMA register offset, skip 256 descriptors */
 #define WORDS_PER_BD(p)		(p->hw_params->words_per_bd)
@@ -2176,6 +2173,19 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto out;
 	}
 
+	/* The MAC holds a frame to insert its checksum, but only as much as
+	 * its FIFO takes. Longer frames are dropped silently.
+	 */
+	if (unlikely(skb->len > priv->tx_thld_len) &&
+	    skb->ip_summed == CHECKSUM_PARTIAL) {
+		if (skb_checksum_help(skb)) {
+			BCMGENET_STATS64_INC((&ring->stats64), dropped);
+			dev_kfree_skb_any(skb);
+			ret = NETDEV_TX_OK;
+			goto out;
+		}
+	}
+
 	/* Keep the frame out of the window just past the threshold */
 	if (unlikely(skb->len > priv->tx_thld_len &&
 		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
@@ -2318,6 +2328,54 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
 	return 0;
 }
 
+/* Drop the frame being collected. Its remaining descriptors carry no SOP,
+ * so they are dropped quietly until the next one does.
+ */
+static void bcmgenet_discard_frags(struct bcmgenet_rx_ring *ring)
+{
+	ring->frag_drop = true;
+
+	if (!ring->frag_head)
+		return;
+
+	dev_kfree_skb_any(ring->frag_head);
+	ring->frag_head = NULL;
+}
+
+/* A frame longer than the threshold arrives in several descriptors, each with
+ * its own status block. Only the first one carries a header, so hand the page
+ * of every later one to the frame already being collected. Returns the frame
+ * once EOP is in, NULL while more descriptors are expected or once the frame
+ * had to be dropped.
+ */
+static struct sk_buff *bcmgenet_add_frag(struct bcmgenet_rx_ring *ring,
+					 struct page *page,
+					 unsigned int offset,
+					 unsigned int size,
+					 unsigned int dma_flag,
+					 unsigned int len)
+{
+	struct sk_buff *head = ring->frag_head;
+
+	if (unlikely(skb_shinfo(head)->nr_frags >= MAX_SKB_FRAGS)) {
+		BCMGENET_STATS64_INC((&ring->stats64), fragmented_errors);
+		bcmgenet_discard_frags(ring);
+		page_pool_put_full_page(ring->page_pool, page, true);
+		return NULL;
+	}
+
+	skb_add_rx_frag(head, skb_shinfo(head)->nr_frags, page,
+			offset + sizeof(struct status_64),
+			len - sizeof(struct status_64), size);
+
+	if (!(dma_flag & DMA_EOP))
+		return NULL;
+
+	ring->frag_head = NULL;
+
+	return head;
+}
+
 /* bcmgenet_desc_rx - descriptor based rx process.
  * this could be called from bottom half, or from NAPI polling method.
  */
@@ -2382,6 +2440,7 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 
 		if (bcmgenet_rx_refill(ring, cb)) {
 			BCMGENET_STATS64_INC(stats, dropped);
+			bcmgenet_discard_frags(ring);
 			goto next;
 		}
 
@@ -2415,15 +2474,23 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 			netif_err(priv, rx_status, dev,
 				  "invalid packet length %d\n", len);
 			BCMGENET_STATS64_INC(stats, length_errors);
+			bcmgenet_discard_frags(ring);
 			page_pool_put_full_page(ring->page_pool, rx_page,
 						true);
 			goto next;
 		}
 
-		if (unlikely(!(dma_flag & DMA_EOP) || !(dma_flag & DMA_SOP))) {
-			netif_err(priv, rx_status, dev,
-				  "dropping fragmented packet!\n");
-			BCMGENET_STATS64_INC(stats, fragmented_errors);
+		/* A new SOP resynchronizes after an incomplete frame */
+		if (dma_flag & DMA_SOP) {
+			if (ring->frag_head) {
+				BCMGENET_STATS64_INC(stats, fragmented_errors);
+				bcmgenet_discard_frags(ring);
+			}
+			ring->frag_drop = false;
+		} else if (unlikely(!ring->frag_head)) {
+			/* Rest of a dropped frame, or no SOP seen yet */
+			if (!ring->frag_drop)
+				BCMGENET_STATS64_INC(stats, fragmented_errors);
 			page_pool_put_full_page(ring->page_pool, rx_page,
 						true);
 			goto next;
@@ -2453,17 +2520,27 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 						DMA_RX_RXER)) == DMA_RX_RXER)
 				u64_stats_inc(&stats->errors);
 			u64_stats_update_end(&stats->syncp);
+			bcmgenet_discard_frags(ring);
 			page_pool_put_full_page(ring->page_pool, rx_page,
 						true);
 			goto next;
 		} /* error packet */
 
+		if (!(dma_flag & DMA_SOP)) {
+			skb = bcmgenet_add_frag(ring, rx_page, rx_offset,
+						rx_size, dma_flag, len);
+			if (!skb)
+				goto next;
+			goto deliver;
+		}
+
 		/* Build SKB from the page - data starts at hard_start,
 		 * frame begins after RSB(64) + pad(2) = 66 bytes.
 		 */
 		skb = napi_build_skb(hard_start, rx_size);
 		if (unlikely(!skb)) {
 			BCMGENET_STATS64_INC(stats, dropped);
+			bcmgenet_discard_frags(ring);
 			page_pool_put_full_page(ring->page_pool, rx_page,
 						true);
 			goto next;
@@ -2475,6 +2552,13 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 		skb_reserve(skb, GENET_RSB_PAD);
 		__skb_put(skb, len - GENET_RSB_PAD);
 
+		if (unlikely(!(dma_flag & DMA_EOP))) {
+			ring->frag_head = skb;
+			goto next;
+		}
+
+deliver:
+
 		if (priv->crc_fwd_en) {
 			skb_trim(skb, skb->len - ETH_FCS_LEN);
 		}
@@ -2593,6 +2677,8 @@ static void bcmgenet_free_rx_buffers(struct bcmgenet_priv *priv)
 			cb = ring->cbs + i;
 			bcmgenet_free_rx_cb(cb, ring->page_pool);
 		}
+		/* a partial frame still holds pages of this pool */
+		bcmgenet_discard_frags(ring);
 	}
 }
 
@@ -4272,7 +4358,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	/* v1 cannot program the thresholds, so it stays at the default MTU */
 	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
 	if (!GENET_IS_V1(priv))
-		dev->max_mtu = ENET_MAX_MTU;
+		dev->max_mtu = ENET_MAX_JUMBO_MTU;
 	INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);
 
 	priv->clk_wol = devm_clk_get_optional(&priv->pdev->dev, "enet-wol");
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index eb00cff0e732..0f31ad2f280d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -579,6 +579,8 @@ struct bcmgenet_rx_ring {
 	unsigned int	cb_ptr;		/* Rx ring initial CB ptr */
 	unsigned int	end_ptr;	/* Rx ring end CB ptr */
 	unsigned int	old_discards;
+	struct sk_buff	*frag_head;	/* frame being reassembled */
+	bool		frag_drop;	/* discarding until the next SOP */
 	struct bcmgenet_net_dim dim;
 	u32		rx_max_coalesced_frames;
 	u32		rx_coalesce_usecs;

-- 
2.53.0

Re: [PATCH net-next 7/7] net: bcmgenet: reassemble jumbo frames from status block fragments

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 2 · 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).

- [High] bcmgenet: skb_trim() panics on non-linear jumbo frame SKBs
- [Medium] This patch adds a TX checksum fallback in bcmgenet_xmit().
- [Medium] The patch raises dev->max_mtu from the old ENET_MAX_MTU (3564
  on 4K pages, 3820 otherwise) to ENET_MAX_JUMBO_MTU = GENMASK(13,0) -…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  bcmgenet_timeout() updates ring->stats64 without holding ring->lock.

--

net: bcmgenet: reassemble jumbo frames from status block fragments

This commit strips the per-descriptor status blocks from RX frames longer
than the packet ready threshold and reassembles them into one skb. It also
adds a software checksum fallback in bcmgenet_xmit() for CHECKSUM_PARTIAL
frames longer than tx_thld_len, and raises max_mtu to ENET_MAX_JUMBO_MTU
(16347).

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 0cd56ec1c5ccf..62edbe51fe076 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2176,6 +2173,19 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  	}
>  
> +	/* The MAC holds a frame to insert its checksum, but only as much as
> +	 * its FIFO takes. Longer frames are dropped silently.
> +	 */
> +	if (unlikely(skb->len > priv->tx_thld_len) &&
> +	    skb->ip_summed == CHECKSUM_PARTIAL) {
> +		if (skb_checksum_help(skb)) {
> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
> +			dev_kfree_skb_any(skb);
> +			ret = NETDEV_TX_OK;
> +			goto out;
> +		}
> +	}
> +

[Severity: Medium]
The new comment says the MAC silently drops CHECKSUM_PARTIAL frames longer
than tx_thld_len. Can that already happen before this patch?

An earlier patch in this series, "net: bcmgenet: pad transmit frames out
of the packet ready window", added bcmgenet_tx_pkt_rdy_thld():

	while (thld > ENET_THLD_DEFAULT &&
	       ENET_MAX_FRAME_LEN(mtu) - ETH_FCS_LEN > thld * ENET_THLD_UNIT &&
	       thld * ENET_THLD_UNIT + ENET_TX_SAFE_MARGIN > mtu + ETH_HLEN)
		thld -= ENET_THLD_BURST / ENET_THLD_UNIT;

On 16K and 64K page kernels ENET_THLD_MAX_LEN is 3840, which makes the old
ENET_MAX_MTU 3820. For MTUs from 3809 to 3820:

    mtu + 32 > 3840
    3840 + 64 > mtu + 14

So the loop lowers the threshold to 0xe0 and tx_thld_len becomes 3584.
Untagged frames can still be up to mtu + 14 bytes long.

In that earlier commit, CHECKSUM_PARTIAL frames longer than 3584 bytes go
out with DMA_TX_DO_CSUM and have no software fallback. Going by the comment
above, the MAC loses them.

Could this fallback move into, or ahead of, the patch that first lets
tx_thld_len drop below the largest frame? Bisects and backports that stop
at that patch would then not see silent TX loss on large-page kernels.

The commit message only mentions the fallback in passing:

    The MAC only checksums a frame it holds in full, so anything longer than
    the threshold falls back to software.

Could it also say that this fixes TX for the earlier patch?

>  	/* Keep the frame out of the window just past the threshold */
>  	if (unlikely(skb->len > priv->tx_thld_len &&
>  		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {

[ ... ]

> @@ -4272,7 +4358,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>  	/* v1 cannot program the thresholds, so it stays at the default MTU */
>  	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
>  	if (!GENET_IS_V1(priv))
> -		dev->max_mtu = ENET_MAX_MTU;
> +		dev->max_mtu = ENET_MAX_JUMBO_MTU;

[Severity: Medium]
Now that max_mtu is 16347, can the TX side describe frames this large?
This patch leaves bcmgenet_xmit() unchanged. It still puts each skb head or
frag in one descriptor and shifts the size in without a mask:

	len_stat = (size << DMA_BUFLENGTH_SHIFT) |
		   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);

The header defines that field as 12 bits:

/* Tx/Rx Dma Descriptor common bits*/
#define DMA_BUFLENGTH_MASK		0x0fff
#define DMA_BUFLENGTH_SHIFT		16

Buffers over 4095 bytes are common at jumbo MTUs. Near MTU 9000, a UDP or
ICMP datagram gets a single linear head from __ip_append_data(). A frag
from an order-3 page_frag page in TCP or soft-GSO can be almost as large as
the MSS.

For a 9014 byte head the field would hold 822, and the upper bits would
spill above the field.

If the hardware field really is 12 bits, would these frames go out
truncated or corrupt while BQL and bytes_sent count the full length?

I couldn't confirm the real field width from the tree. The RX path reads
the length as dma_length_status >> DMA_BUFLENGTH_SHIFT without a mask. The
Tested-by may also have covered jumbo TX.

If the field is 12 bits, should max_mtu be capped at the TX descriptor
limit, or should large TX buffers be split across descriptors?

>  	INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
bcmgenet_timeout() updates the TX ring stats without holding ring->lock:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_timeout() {
    ...
	bcmgenet_tx_reclaim(dev, ring, true);

	/* Re-enable the TX interrupt for this ring */
	bcmgenet_intrl2_1_writel(priv, 1 << txqueue, INTRL2_CPU_MASK_CLEAR);

	txq_trans_cond_update(txq);

	BCMGENET_STATS64_INC((&ring->stats64), errors);
    ...
}

bcmgenet_tx_reclaim() takes ring->lock and releases it again, so the
increment runs without the lock. Meanwhile, on another CPU,
bcmgenet_tx_poll()->__bcmgenet_tx_reclaim() holds ring->lock and writes the
same syncp:

	u64_stats_update_begin(&stats->syncp);
	u64_stats_add(&stats->packets, pkts_compl);
	u64_stats_add(&stats->bytes, bytes_compl);
	u64_stats_update_end(&stats->syncp);

dev_watchdog() holds the TX queue locks. Those only exclude ndo_start_xmit,
not TX NAPI. Also, the interrupt is unmasked just before the increment, so
a pending TX-done interrupt can schedule TX NAPI at that moment.

On 32-bit SMP, can the two writers overlap and lose a seqcount increment?
That would leave the sequence odd. Readers in bcmgenet_get_stats64() and
bcmgenet_get_ethtool_stats() would then spin forever in
u64_stats_fetch_begin(). A shorter overlap could give them torn counters.

Should the increment in bcmgenet_timeout() be done while holding
ring->lock?

[ ... ]

> @@ -2475,6 +2552,13 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  		skb_reserve(skb, GENET_RSB_PAD);
>  		__skb_put(skb, len - GENET_RSB_PAD);
>  
> +		if (unlikely(!(dma_flag & DMA_EOP))) {
> +			ring->frag_head = skb;
> +			goto next;
> +		}
> +
> +deliver:
> +
>  		if (priv->crc_fwd_en) {
>  			skb_trim(skb, skb->len - ETH_FCS_LEN);
>  		}

[Severity: High]
When priv->crc_fwd_en is set, a reassembled jumbo frame now reaches the
skb_trim() just below the new deliver label. Can skb_trim() handle that
skb?

Before this patch only linear skbs from napi_build_skb() got here, so
skb_trim() was fine. bcmgenet_add_frag() now attaches every non-SOP
descriptor with skb_add_rx_frag(). That makes the skb returned on EOP
non-linear. skb_trim() calls __skb_trim(), which starts with:

	if (WARN_ON(skb_is_nonlinear(skb)))
		return;

With CRC forwarding on, every jumbo frame would then:

  - trigger a WARN, once per received frame, which a remote sender can
    drive as fast as it likes. With panic_on_warn that is a remote
    crash.
  - keep its 4 FCS bytes in the payload handed to napi_gro_receive().
  - have those 4 bytes counted in stats->bytes.

The FCS may also span two frags. min_len for a non-SOP descriptor is
only sizeof(struct status_64), so the last descriptor can carry fewer
than ETH_FCS_LEN bytes past its status block. Any trim therefore has to
be able to reach back into the previous frag.

Should this use pskb_trim(), which walks the frags, with its return
value checked? Since ip_summed is still CHECKSUM_NONE at that point,
there is no csum to adjust.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee%40tipi-net.de

Re: [PATCH net-next 7/7] net: bcmgenet: reassemble jumbo frames from status block fragments

[Nicolai Buchwitz]

On 5.10.2026 17:04, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 2 · 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).
> 
> - [High] bcmgenet: skb_trim() panics on non-linear jumbo frame SKBs
> - [Medium] This patch adds a TX checksum fallback in bcmgenet_xmit().
> - [Medium] The patch raises dev->max_mtu from the old ENET_MAX_MTU 
> (3564
>   on 4K pages, 3820 otherwise) to ENET_MAX_JUMBO_MTU = GENMASK(13,0) -…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   bcmgenet_timeout() updates ring->stats64 without holding ring->lock.
> 
> --
> 
> net: bcmgenet: reassemble jumbo frames from status block fragments
> 
> This commit strips the per-descriptor status blocks from RX frames 
> longer
> than the packet ready threshold and reassembles them into one skb. It 
> also
> adds a software checksum fallback in bcmgenet_xmit() for 
> CHECKSUM_PARTIAL
> frames longer than tx_thld_len, and raises max_mtu to 
> ENET_MAX_JUMBO_MTU
> (16347).
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c 
>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index 0cd56ec1c5ccf..62edbe51fe076 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> 
> [ ... ]
> 
>> @@ -2176,6 +2173,19 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff 
>> *skb, struct net_device *dev)
>>  		goto out;
>>  	}
>> 
>> +	/* The MAC holds a frame to insert its checksum, but only as much as
>> +	 * its FIFO takes. Longer frames are dropped silently.
>> +	 */
>> +	if (unlikely(skb->len > priv->tx_thld_len) &&
>> +	    skb->ip_summed == CHECKSUM_PARTIAL) {
>> +		if (skb_checksum_help(skb)) {
>> +			BCMGENET_STATS64_INC((&ring->stats64), dropped);
>> +			dev_kfree_skb_any(skb);
>> +			ret = NETDEV_TX_OK;
>> +			goto out;
>> +		}
>> +	}
>> +
> 
> [Severity: Medium]
> The new comment says the MAC silently drops CHECKSUM_PARTIAL frames 
> longer
> than tx_thld_len. Can that already happen before this patch?
> 
> An earlier patch in this series, "net: bcmgenet: pad transmit frames 
> out
> of the packet ready window", added bcmgenet_tx_pkt_rdy_thld():
> 
> 	while (thld > ENET_THLD_DEFAULT &&
> 	       ENET_MAX_FRAME_LEN(mtu) - ETH_FCS_LEN > thld * ENET_THLD_UNIT 
> &&
> 	       thld * ENET_THLD_UNIT + ENET_TX_SAFE_MARGIN > mtu + ETH_HLEN)
> 		thld -= ENET_THLD_BURST / ENET_THLD_UNIT;
> 
> On 16K and 64K page kernels ENET_THLD_MAX_LEN is 3840, which makes the 
> old
> ENET_MAX_MTU 3820. For MTUs from 3809 to 3820:
> 
>     mtu + 32 > 3840
>     3840 + 64 > mtu + 14
> 
> So the loop lowers the threshold to 0xe0 and tx_thld_len becomes 3584.
> Untagged frames can still be up to mtu + 14 bytes long.
> 
> In that earlier commit, CHECKSUM_PARTIAL frames longer than 3584 bytes 
> go
> out with DMA_TX_DO_CSUM and have no software fallback. Going by the 
> comment
> above, the MAC loses them.
> 
> Could this fallback move into, or ahead of, the patch that first lets
> tx_thld_len drop below the largest frame? Bisects and backports that 
> stop
> at that patch would then not see silent TX loss on large-page kernels.
> 
> The commit message only mentions the fallback in passing:
> 
>     The MAC only checksums a frame it holds in full, so anything longer 
> than
>     the threshold falls back to software.
> 
> Could it also say that this fixes TX for the earlier patch?
> 
>>  	/* Keep the frame out of the window just past the threshold */
>>  	if (unlikely(skb->len > priv->tx_thld_len &&
>>  		     skb->len < priv->tx_thld_len + ENET_TX_SAFE_MARGIN)) {
> 
> [ ... ]
> 
>> @@ -4272,7 +4358,7 @@ static int bcmgenet_probe(struct platform_device 
>> *pdev)
>>  	/* v1 cannot program the thresholds, so it stays at the default MTU 
>> */
>>  	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
>>  	if (!GENET_IS_V1(priv))
>> -		dev->max_mtu = ENET_MAX_MTU;
>> +		dev->max_mtu = ENET_MAX_JUMBO_MTU;
> 
> [Severity: Medium]
> Now that max_mtu is 16347, can the TX side describe frames this large?
> This patch leaves bcmgenet_xmit() unchanged. It still puts each skb 
> head or
> frag in one descriptor and shifts the size in without a mask:
> 
> 	len_stat = (size << DMA_BUFLENGTH_SHIFT) |
> 		   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
> 
> The header defines that field as 12 bits:
> 
> /* Tx/Rx Dma Descriptor common bits*/
> #define DMA_BUFLENGTH_MASK		0x0fff
> #define DMA_BUFLENGTH_SHIFT		16
> 
> Buffers over 4095 bytes are common at jumbo MTUs. Near MTU 9000, a UDP 
> or
> ICMP datagram gets a single linear head from __ip_append_data(). A frag
> from an order-3 page_frag page in TCP or soft-GSO can be almost as 
> large as
> the MSS.
> 
> For a 9014 byte head the field would hold 822, and the upper bits would
> spill above the field.
> 
> If the hardware field really is 12 bits, would these frames go out
> truncated or corrupt while BQL and bytes_sent count the full length?

It is not 12 bits. On a CM4 at MTU 9000 I sent raw single descriptor 
frames
of 1498 to 8998 bytes (20 each). All frames arrived at full length with 
every
payload byte intact. With a 12 bit field 4098 would have gone out as 2 
bytes.

DMA_BUFLENGTH_MASK masks the length out of the RX status word, it is not 
the
hardware field width, so max_mtu needs no cap.

> [...]