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

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

[Nicolai Buchwitz]

This is v7 of my genet series which adds larger MTU / jumbo packet support.

Larger MTU aka. jumbo frames are requested often by Raspberry Pi users, but
the hardware has its quirks with larger frames. Dave Stevenson's PoC [1] had
the receive side working, and thanks to Justin, who checked the RTL for what
the hardware does with long frames, I could add the missing puzzle pieces.

GENET never sets dev->max_mtu and the packet ready thresholds stay at their
2048 byte reset default, so long frames were dropped on receive and never
sent on transmit.

Patches 1 to 5 prepare the driver, patch 8 sets the MTU, as far as one
descriptor reaches (3564 bytes with 4K pages). Patch 9 reassembles what the
hardware hands up above that, which gets the ceiling to 16347 [2]. Patch 6
splits a frame ending just past the packet ready threshold, which otherwise
stops the transmitter [3]. Patch 7 builds the new rings before dropping the
old ones, so a failed MTU change leaves the interface alone.

A split frame takes one descriptor more. Patch 6 adds it to the two limits
which 1e09af397bc7 and fbfd7cfd2019 put on the 32 descriptor rings, so the
queue stops one descriptor earlier and a frame is flattened one fragment
sooner.

Patch 8 needs bcmgenet_fini_dma() to clear rx_cbs and tx_cbs, which landed
in net as 1c1f51c3ee9c.

Tested on a Raspberry Pi CM4 (GENET v5), peer at the same MTU:
- MTU 1500: iperf3 TCP 941/941 Mbit/s, UDP 956 Mbit/s, unchanged
- MTU 9000: iperf3 TCP 991/987 Mbit/s, UDP 993 Mbit/s, no loss
- MTU 1500 to 16347 with traffic and ifdown/ifup's
- pktgen at line rate, 3858/3872/3886 byte frames mixed with 64 byte ones
- raw frames across every receive descriptor boundary, byte exact
- CONFIG_MAX_SKB_FRAGS 31, 29 to 31 fragment frames in the split window

Pierre-Marin Leclercq tested v5 on a Raspberry Pi 4.

[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>
---
Changes in v7:
- Patch 1 rings the doorbell on the skb_share_check() exit too (sashiko)
- Patch 6 wakes and stops the transmit queue at the same threshold (sashiko)
- Patch 9 clears frag_drop once the dropped frame has ended (sashiko)
- Dropped Florian's Reviewed-by on patches 1 and 9, both changed
- Dropped Pierre-Marin's Tested-by on patch 9, it changed

To: Doug Berger <opendmb@gmail.com>
To: Florian Fainelli <florian.fainelli@broadcom.com>
To: Nicolai Buchwitz <nb@tipi-net.de>
To: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
v6: https://patch.msgid.link/20261103-nb-genet-mtu-nn-v2-v6-0-61d5631676e5@tipi-net.de
v5: https://patch.msgid.link/20261013-nb-genet-mtu-nn-v2-v5-0-076b6fc240be@tipi-net.de
v4: https://patch.msgid.link/20261012-nb-genet-mtu-nn-v2-v4-0-2e2e3bfaf8cc@tipi-net.de
v3: https://patch.msgid.link/20261007-nb-genet-mtu-nn-v2-v3-0-74a796c019ce@tipi-net.de
v2: https://patch.msgid.link/20261006-nb-genet-mtu-nn-v2-v2-0-c4be21151912@tipi-net.de
v1: https://patch.msgid.link/20261002-nb-genet-mtu-nn-v2-v1-0-96dc6d54cbee@tipi-net.de

---
Nicolai Buchwitz (9):
      net: bcmgenet: ring the doorbell when the last frame of a batch is dropped
      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: split frames that end just past the transmit threshold
      net: bcmgenet: build the DMA rings before giving up the running ones
      net: bcmgenet: allow the MTU to be changed
      net: bcmgenet: reassemble jumbo frames from status block fragments

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 737 +++++++++++++++++++------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  34 +-
 2 files changed, 588 insertions(+), 183 deletions(-)
---
base-commit: c88fd6989aea9237a60a4859139a265bfb7ddc0b
change-id: 20261002-nb-genet-mtu-nn-v2-402b5e6f306f

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

[PATCH net-next v7 1/9] net: bcmgenet: ring the doorbell when the last frame of a batch is dropped

[Nicolai Buchwitz]

bcmgenet_xmit() only writes the producer index at the end of a batch. The
error exits return before that write. If the dropped frame was the last of
an xmit_more batch, the frames queued before it stay in the ring until the
next transmit. The queue is still running, so the watchdog does not fire.

It came out of a sashiko review, not a bug report. Those exits only run
when a clone, an allocation or a DMA mapping fails, and a later patch adds
one more.

Write the producer index on those exits too.

Fixes: ddd0ca5d60b3 ("net: bcmgenet: add support for xmit_more")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 31 +++++++++++++++-----------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 61f5a4c2f0cd9..1114fe46d2f73 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2122,6 +2122,15 @@ static void bcmgenet_hide_tsb(struct sk_buff *skb)
 	__skb_pull(skb, sizeof(struct status_64));
 }
 
+static void bcmgenet_tx_kick(struct bcmgenet_priv *priv,
+			     struct bcmgenet_tx_ring *ring,
+			     struct netdev_queue *txq)
+{
+	if (!netdev_xmit_more() || netif_xmit_stopped(txq))
+		bcmgenet_tdma_ring_writel(priv, ring->index,
+					  ring->prod_index, TDMA_PROD_INDEX);
+}
+
 static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
@@ -2153,8 +2162,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (!skb) {
 			spin_lock(&ring->lock);
 			BCMGENET_STATS64_INC((&ring->stats64), dropped);
-			ret = NETDEV_TX_OK;
-			goto out;
+			goto drop;
 		}
 		nr_frags = 0;
 	}
@@ -2173,8 +2181,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb = skb_share_check(skb, GFP_ATOMIC);
 	if (!skb) {
 		BCMGENET_STATS64_INC((&ring->stats64), dropped);
-		ret = NETDEV_TX_OK;
-		goto out;
+		goto drop;
 	}
 
 	/* Retain how many bytes will be sent on the wire, without TSB inserted
@@ -2184,10 +2191,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* add the Transmit Status Block */
 	skb = bcmgenet_add_tsb(dev, skb, ring);
-	if (!skb) {
-		ret = NETDEV_TX_OK;
-		goto out;
-	}
+	if (!skb)
+		goto drop;
 
 	for (i = 0; i <= nr_frags; i++) {
 		tx_cb_ptr = bcmgenet_get_txcb(priv, ring);
@@ -2212,7 +2217,6 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (ret) {
 			priv->mib.tx_dma_failed++;
 			netif_err(priv, tx_err, dev, "Tx DMA map failed\n");
-			ret = NETDEV_TX_OK;
 			goto out_unmap_frags;
 		}
 		dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
@@ -2254,10 +2258,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (ring->free_bds <= ring->stop_thld)
 		netif_tx_stop_queue(txq);
 
-	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);
+	bcmgenet_tx_kick(priv, ring, txq);
 out:
 	spin_unlock(&ring->lock);
 
@@ -2274,6 +2275,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	dev_kfree_skb(skb);
+drop:
+	/* The dropped frame may have been the last one of the batch */
+	bcmgenet_tx_kick(priv, ring, txq);
+	ret = NETDEV_TX_OK;
 	goto out;
 }
 

-- 
2.53.0

[PATCH net-next v7 2/9] 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>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 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 1114fe46d2f73..4c2a38eaab108 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3390,7 +3390,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);
 
@@ -3407,7 +3407,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)
@@ -3470,7 +3471,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);
 
@@ -4369,7 +4370,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

[PATCH net-next v7 3/9] 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 a byte 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.

Tested-by: Pierre-Marin Leclercq <pierremarinleclercq88@gmail.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 4c2a38eaab108..d4eac993368fe 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -53,7 +53,8 @@
 
 /* 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.
+ * The HW writes the 64B RSB before every descriptor of a frame. Only the
+ * first one also gets the 2B alignment padding.
  */
 #define GENET_RSB_PAD		(sizeof(struct status_64) + 2)
 
@@ -2363,6 +2364,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;
 
@@ -2399,8 +2401,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

[PATCH net-next v7 4/9] 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.

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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 d4eac993368fe..b0b0d9b423482 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2673,7 +2673,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);
@@ -2781,7 +2781,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 6ab3f73475550..afaccb7699336 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

[PATCH net-next v7 5/9] 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. Derive UMAC_MAX_FRAME_LEN and the flow period from the MTU too, with
a floor at ETH_DATA_LEN so a lower MTU does not stop the MAC accepting a
standard frame.

Suggested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 85 +++++++++++++++++++++-----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h | 18 +++++-
 2 files changed, 85 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index b0b0d9b423482..1bb5fed4a8b4d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -48,18 +48,36 @@
 #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 before every descriptor of a frame. Only the
  * first one also gets the 2B alignment padding.
  */
-#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 never spans more than one page, which also holds
+ * skb_shared_info behind the frame, so on 4K pages the page bounds the
+ * threshold before the register does. Larger pages fit several descriptors.
+ */
+#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)
@@ -2287,7 +2305,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;
@@ -2302,7 +2320,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;
@@ -2381,10 +2399,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;
@@ -2406,7 +2424,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);
@@ -2657,6 +2675,41 @@ 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_MAC_FRAME_LEN(mtu), 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);
+}
+
 static void init_umac(struct bcmgenet_priv *priv)
 {
 	struct device *kdev = &priv->pdev->dev;
@@ -2673,7 +2726,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);
@@ -2781,7 +2834,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_MAC_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);
@@ -2792,7 +2845,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,
@@ -2811,8 +2864,9 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 static int bcmgenet_rx_ring_create_pool(struct bcmgenet_priv *priv,
 					struct bcmgenet_rx_ring *ring)
 {
-	/* Buffers share a page. bcmgenet_rx_refill() syncs each one for the
-	 * device, PP_FLAG_DMA_SYNC_DEV would sync the whole page.
+	/* Buffers may share a page, depending on PAGE_SIZE and the MTU.
+	 * bcmgenet_rx_refill() syncs each one for the device,
+	 * PP_FLAG_DMA_SYNC_DEV would sync the whole page.
 	 */
 	struct page_pool_params pp_params = {
 		.order = 0,
@@ -2875,7 +2929,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) |
@@ -4154,6 +4208,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 afaccb7699336..b40e56872132c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -31,9 +31,17 @@
 #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. ENET_PAD is slack the
+ * driver has always carried, it rounded the default up to 1536 from 1528.
+ */
+#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)
+
+/* The MAC keeps accepting a standard frame below the default MTU */
+#define ENET_MAC_FRAME_LEN(mtu)	ENET_MAX_FRAME_LEN(max_t(unsigned int, (mtu), \
+							ETH_DATA_LEN))
+
 #define DMA_MAX_BURST_LENGTH    0x10
 
 /* misc. configuration */
@@ -218,6 +226,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 +258,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)
@@ -614,6 +625,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 v7 5/9] 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] In bcmgenet_init_tx_ring()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c), the buffer-size…

--

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

This patch replaces the fixed 2048 byte receive buffer length with a value
taken from the MTU. The RBUF packet ready threshold is computed from the
MTU and programmed into RBUF, and the buffer length is that threshold
plus the status block. UMAC_MAX_FRAME_LEN and the TX flow period are
also derived from the MTU, with a floor at ETH_DATA_LEN.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index b0b0d9b423482..1bb5fed4a8b4d 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> +/* 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;
> +}

[ ... ]

> @@ -2792,7 +2845,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);

[Severity: Low]
Is this change to the TX ring intended?

In bcmgenet_init_tx_ring(), the buffer size field of the TDMA
DMA_RING_BUF_SIZE register now gets priv->rx_buf_len. That value comes
from bcmgenet_rx_buf_len(), which only uses RX quantities: the RX status
block plus the RBUF packet ready threshold.

At the default MTU this is 64 + 0x80 * 16 = 2112, so the TDMA register
value goes from 0x800 to 0x840 on every interface init.

The commit message only talks about the receive side:

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

It doesn't mention that a TX DMA register changes as well.

The TX ring already used the RX constant RX_BUF_LENGTH before this patch.
What is new here is the changed value, and the fact that the TX
programming now follows the MTU through the RX threshold and its
page-size clamp.

This line is unchanged in the later patches of the series. So once the MTU
can grow, the TDMA field keeps following bcmgenet_rx_buf_len(mtu) and not
any value picked for TX.

No functional effect has been shown. The per-descriptor TX length is in
length_status. The last patch in the series, "net: bcmgenet: reassemble
jumbo frames from status block fragments", reports 16361 byte frames
sent byte-exact over UMAC loopback while this field stays at rx_buf_len
(about 3.9K at most). That suggests the field does not limit TX.

Would it be clearer to keep a TX-specific value here? If the change is
intentional, could the commit message say so?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-nb-genet-mtu-nn-v2-v7-0-43a5d90b3256%40tipi-net.de

[PATCH net-next v7 6/9] net: bcmgenet: split frames that end just past the transmit threshold

[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 never reaches 64 bytes.
On a v5 it ends 28, 32 and 46 bytes past thresholds of 2560, 3584 and
3840, and link speed makes no difference. That is one DMA transfer wide,
which wtschueller measured and proposed to split around.

Splitting such a frame over two descriptors keeps its end out of that
transfer. Both share the one mapping, so the wire bytes do not change. The
split costs one descriptor, so the queue stops and wakes one earlier, and
a frame is flattened one fragment sooner.

The threshold kept its reset value until now. Raising it only once the MTU
asks for more than ETH_DATA_LEN leaves the common MTUs as they were.

The MAC inserts a checksum only into a frame it holds in full, so a longer
one has to fall back to software.

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

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 1bb5fed4a8b4d..d70079832dd24 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 this far past the transmit threshold stops the transmitter
+ * once a shorter frame follows. Widest window measured is 46 bytes.
+ */
+#define ENET_TX_THLD_WINDOW	64
+
 /* Page pool RX buffer layout:
  * RSB(64) + pad(2) | frame data | skb_shared_info
  * The HW writes the 64B RSB before every descriptor of a frame. Only the
@@ -1910,15 +1915,19 @@ static struct sk_buff *bcmgenet_free_tx_cb(struct device *dev,
 
 	if (skb) {
 		cb->skb = NULL;
-		if (cb == GENET_CB(skb)->first_cb)
-			dma_unmap_single(dev, dma_unmap_addr(cb, dma_addr),
-					 dma_unmap_len(cb, dma_len),
-					 DMA_TO_DEVICE);
-		else
-			dma_unmap_page(dev, dma_unmap_addr(cb, dma_addr),
-				       dma_unmap_len(cb, dma_len),
-				       DMA_TO_DEVICE);
-		dma_unmap_addr_set(cb, dma_addr, 0);
+		/* A cut descriptor left its mapping to the one after it */
+		if (cb != GENET_CB(skb)->split_cb) {
+			if (cb == GENET_CB(skb)->first_cb)
+				dma_unmap_single(dev,
+						 dma_unmap_addr(cb, dma_addr),
+						 dma_unmap_len(cb, dma_len),
+						 DMA_TO_DEVICE);
+			else
+				dma_unmap_page(dev, dma_unmap_addr(cb, dma_addr),
+					       dma_unmap_len(cb, dma_len),
+					       DMA_TO_DEVICE);
+			dma_unmap_addr_set(cb, dma_addr, 0);
+		}
 
 		if (cb == GENET_CB(skb)->last_cb)
 			return skb;
@@ -2154,7 +2163,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct device *kdev = &priv->pdev->dev;
+	unsigned int split_i = 0, split_at = 0;
 	struct bcmgenet_tx_ring *ring = NULL;
+	dma_addr_t split_mapping = 0;
+	unsigned int split_size = 0;
 	struct enet_cb *tx_cb_ptr;
 	struct netdev_queue *txq;
 	int nr_frags, index;
@@ -2162,6 +2174,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	unsigned int size;
 	skb_frag_t *frag;
 	u32 len_stat;
+	bool split;
 	int ret;
 	int i;
 
@@ -2171,8 +2184,14 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
+	/* Cut a frame ending in that window in two at the threshold, its end
+	 * then lands in a separate transfer.
+	 */
+	split = unlikely(skb->len > priv->tx_thld_len &&
+			 skb->len < priv->tx_thld_len + ENET_TX_THLD_WINDOW);
+
 	/* The check below would always refuse this frame */
-	if (unlikely(nr_frags + 1 >= ring->size)) {
+	if (unlikely(nr_frags + 1 + split >= ring->size)) {
 		skb = skb_share_check(skb, GFP_ATOMIC);
 		if (skb && skb_linearize(skb)) {
 			dev_kfree_skb_any(skb);
@@ -2187,7 +2206,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	spin_lock(&ring->lock);
-	if (ring->free_bds <= (nr_frags + 1)) {
+	if (ring->free_bds <= (nr_frags + 1 + split)) {
 		if (!netif_tx_queue_stopped(txq))
 			netif_tx_stop_queue(txq);
 		ret = NETDEV_TX_BUSY;
@@ -2203,6 +2222,19 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto drop;
 	}
 
+	/* The MAC inserts a checksum only into a frame it holds in full and
+	 * silently drops a longer one, so fall back to software.
+	 */
+	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);
+			goto drop;
+		}
+		nr_frags = skb_shinfo(skb)->nr_frags;
+	}
+
 	/* Retain how many bytes will be sent on the wire, without TSB inserted
 	 * by transmit checksum offload
 	 */
@@ -2213,33 +2245,73 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!skb)
 		goto drop;
 
-	for (i = 0; i <= nr_frags; i++) {
+	/* A threshold that already ends a buffer needs no cut */
+	if (split) {
+		unsigned int buf = skb_headlen(skb);
+		unsigned int covered = buf - sizeof(struct status_64);
+
+		while (covered < priv->tx_thld_len) {
+			frag = &skb_shinfo(skb)->frags[split_i++];
+			buf = skb_frag_size(frag);
+			covered += buf;
+		}
+
+		split_at = buf - (covered - priv->tx_thld_len);
+		split = split_at && split_at < buf;
+	}
+
+	for (i = 0; i <= nr_frags + split; i++) {
+		unsigned int f = i - (split && i > split_i);
+		bool cont = split && i == split_i + 1;
+
 		tx_cb_ptr = bcmgenet_get_txcb(priv, ring);
 
 		BUG_ON(!tx_cb_ptr);
 
-		if (!i) {
-			/* Transmit single SKB or head of fragment list */
-			GENET_CB(skb)->first_cb = tx_cb_ptr;
-			size = skb_headlen(skb);
-			mapping = dma_map_single(kdev, skb->data, size,
-						 DMA_TO_DEVICE);
+		if (cont) {
+			/* Rest of the buffer cut above. It unmaps for both,
+			 * the hardware is done with it only now.
+			 */
+			mapping = split_mapping + split_at;
+			size = split_size - split_at;
+			dma_unmap_addr_set(tx_cb_ptr, dma_addr, split_mapping);
+			dma_unmap_len_set(tx_cb_ptr, dma_len, split_size);
 		} else {
-			/* xmit fragment */
-			frag = &skb_shinfo(skb)->frags[i - 1];
-			size = skb_frag_size(frag);
-			mapping = skb_frag_dma_map(kdev, frag, 0, size,
-						   DMA_TO_DEVICE);
-		}
+			if (!f) {
+				/* Transmit single SKB or head of frag list */
+				GENET_CB(skb)->first_cb = tx_cb_ptr;
+				GENET_CB(skb)->split_cb = NULL;
+				size = skb_headlen(skb);
+				mapping = dma_map_single(kdev, skb->data,
+							 size, DMA_TO_DEVICE);
+			} else {
+				/* xmit fragment */
+				frag = &skb_shinfo(skb)->frags[f - 1];
+				size = skb_frag_size(frag);
+				mapping = skb_frag_dma_map(kdev, frag, 0, size,
+							   DMA_TO_DEVICE);
+			}
 
-		ret = dma_mapping_error(kdev, mapping);
-		if (ret) {
-			priv->mib.tx_dma_failed++;
-			netif_err(priv, tx_err, dev, "Tx DMA map failed\n");
-			goto out_unmap_frags;
+			ret = dma_mapping_error(kdev, mapping);
+			if (ret) {
+				priv->mib.tx_dma_failed++;
+				netif_err(priv, tx_err, dev,
+					  "Tx DMA map failed\n");
+				goto out_unmap_frags;
+			}
+			dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
+			dma_unmap_len_set(tx_cb_ptr, dma_len, size);
+
+			/* The cut hands buffer and mapping to the next one */
+			if (split && i == split_i) {
+				GENET_CB(skb)->split_cb = tx_cb_ptr;
+				split_mapping = mapping;
+				split_size = size;
+				size = split_at;
+				dma_unmap_addr_set(tx_cb_ptr, dma_addr, 0);
+				dma_unmap_len_set(tx_cb_ptr, dma_len, 0);
+			}
 		}
-		dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
-		dma_unmap_len_set(tx_cb_ptr, dma_len, size);
 
 		tx_cb_ptr->skb = skb;
 
@@ -2256,7 +2328,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 			if (skb->ip_summed == CHECKSUM_PARTIAL)
 				len_stat |= DMA_TX_DO_CSUM;
 		}
-		if (i == nr_frags)
+		if (i == nr_frags + split)
 			len_stat |= DMA_EOP;
 
 		dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
@@ -2268,8 +2340,8 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb_tx_timestamp(skb);
 
 	/* Decrement total BD count and advance our write pointer */
-	ring->free_bds -= nr_frags + 1;
-	ring->prod_index += nr_frags + 1;
+	ring->free_bds -= nr_frags + 1 + split;
+	ring->prod_index += nr_frags + 1 + split;
 	ring->prod_index &= DMA_P_INDEX_MASK;
 
 	netdev_tx_sent_queue(txq, GENET_CB(skb)->bytes_sent);
@@ -2689,6 +2761,17 @@ static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
 		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
 }
 
+/* The transmit threshold in register units. Keep the reset value while the
+ * MTU has no use for more, the hardware never reaches it below ETH_DATA_LEN.
+ */
+static unsigned int bcmgenet_tx_pkt_rdy_thld(unsigned int mtu)
+{
+	if (mtu <= ETH_DATA_LEN)
+		return ENET_THLD_DEFAULT;
+
+	return ENET_THLD_MAX;
+}
+
 /* A buffer has to hold everything the threshold lets the hardware deliver */
 static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
 {
@@ -2699,8 +2782,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_MAC_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN);
 
 	/* GENET v1 maps other registers at these offsets */
@@ -2708,6 +2793,8 @@ 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(tx_thld, priv->base + priv->hw_params->tbuf_offset +
+			TBUF_PKT_RDY_THLD);
 }
 
 static void init_umac(struct bcmgenet_priv *priv)
@@ -2822,8 +2909,10 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 	ring->index = index;
 	ring->cbs = priv->tx_cbs + start_ptr;
 	ring->size = size;
-	/* An empty ring must be able to wake the queue */
-	ring->stop_thld = min_t(unsigned int, MAX_SKB_FRAGS + 1, size - 1);
+	/* An empty ring must be able to wake the queue, a split frame takes
+	 * one descriptor more.
+	 */
+	ring->stop_thld = min_t(unsigned int, MAX_SKB_FRAGS + 2, size - 1);
 	ring->clean_ptr = start_ptr;
 	ring->c_index = 0;
 	ring->free_bds = size;
@@ -4209,6 +4298,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	init_waitqueue_head(&priv->wq);
 	bcmgenet_hfb_init(priv);
 	priv->rx_buf_len = bcmgenet_rx_buf_len(dev->mtu);
+	priv->tx_thld_len = bcmgenet_tx_pkt_rdy_thld(dev->mtu) * ENET_THLD_UNIT;
 	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 b40e56872132c..12a3dfa807393 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -544,6 +544,7 @@ struct bcmgenet_hw_params {
 struct bcmgenet_skb_cb {
 	struct enet_cb *first_cb;	/* First control block of SKB */
 	struct enet_cb *last_cb;	/* Last control block of SKB */
+	struct enet_cb *split_cb;	/* Cut, the next one unmaps for both */
 	unsigned int bytes_sent;	/* bytes on the wire (no TSB) */
 };
 
@@ -618,6 +619,7 @@ struct bcmgenet_priv {
 	void __iomem *tx_bds;
 	struct enet_cb *tx_cbs;
 	unsigned int num_tx_bds;
+	unsigned int tx_thld_len;
 
 	struct bcmgenet_tx_ring tx_rings[GENET_MAX_MQ_CNT + 1];
 

-- 
2.53.0

Re: [PATCH net-next v7 6/9] net: bcmgenet: split frames that end just past the transmit threshold

[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] ENET_TX_THLD_WINDOW is fixed at 64 bytes for every platform.
- [Medium] bcmgenet_tx_pkt_rdy_thld() returns ENET_THLD_MAX (0xf0, 3840
  bytes) for every GENET version once the MTU exceeds ETH_DATA_LEN, and…
- [Low] The commit message describes the transmitter stall as a live bug
  that IP fragmentation hits on every datagram, and says the interface…

--

net: bcmgenet: split frames that end just past the transmit threshold

This patch splits a frame across two descriptors that share one DMA
mapping when the frame ends within ENET_TX_THLD_WINDOW bytes past the
transmit packet ready threshold. It programs TBUF_PKT_RDY_THLD from the
MTU and falls back to a software checksum for frames longer than the
threshold. It also reserves one more descriptor in ring->stop_thld.

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

[Severity: Low]
Can this happen at this point in the series?

genet does not set dev->max_mtu and has no ndo_change_mtu, so
ether_setup() leaves max_mtu at ETH_DATA_LEN. There is no TSO, so GSO
segments down to the MTU.

That means bcmgenet_tx_pkt_rdy_thld() always returns ENET_THLD_DEFAULT
and priv->tx_thld_len is always 2048. No transmitted frame, which is
about 1518 bytes at most, can satisfy this check in bcmgenet_xmit():

	split = unlikely(skb->len > priv->tx_thld_len &&
			 skb->len < priv->tx_thld_len + ENET_TX_THLD_WINDOW);

So the split path, the skb_checksum_help() fallback and the
ENET_THLD_MAX branch cannot run until "net: bcmgenet: allow the MTU to
be changed". Today the only runtime changes appear to be two:
TBUF_PKT_RDY_THLD gets its reset value written, and stop_thld goes from
MAX_SKB_FRAGS + 1 to MAX_SKB_FRAGS + 2.

Earlier patches in this series state this directly ("The MTU cannot
produce a frame past the threshold yet, so nothing hits this today. It
is preparation for the larger MTU").

Could this message say the same, so that readers and stable backporters
don't take it for a fix for a hang that can happen today?

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1bb5fed4a8b4d..d70079832dd24 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 this far past the transmit threshold stops the transmitter
> + * once a shorter frame follows. Widest window measured is 46 bytes.
> + */
> +#define ENET_TX_THLD_WINDOW	64

[Severity: Medium]
The commit message says the window "is one DMA transfer wide". The TX
DMA burst size, though, is set per platform in bcmgenet_dma_init():

	bcmgenet_tdma_writel(priv, priv->dma_max_burst_length,
			     DMA_SCB_BURST_SIZE);

bcm2711_plat_data uses .dma_max_burst_length = 0x08. The v2, v3, v4, v5
and bcm7712 platform data use DMA_MAX_BURST_LENGTH (0x10).

Suppose the window grows with the programmed burst. On the 0x10 burst
parts, a frame could end 64 bytes or more past tx_thld_len and still
fall inside one transfer. Would bcmgenet_xmit() then skip the split for
it? That would leave the Tx DMA stall and the bcmgenet_init_dma()
failure in place on those cores.

The measurements in the message come from one v5. Once "net: bcmgenet:
allow the MTU to be changed" raises max_mtu on every non-v1 core, this
case becomes reachable.

Should the window be derived from dma_max_burst_length, or has 64 been
checked on a core with a 0x10 burst?

[ ... ]

> @@ -2689,6 +2761,17 @@ static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
>  		       ENET_THLD_MAX_LEN / ENET_THLD_UNIT);
>  }
>  
> +/* The transmit threshold in register units. Keep the reset value while the
> + * MTU has no use for more, the hardware never reaches it below ETH_DATA_LEN.
> + */
> +static unsigned int bcmgenet_tx_pkt_rdy_thld(unsigned int mtu)
> +{
> +	if (mtu <= ETH_DATA_LEN)
> +		return ENET_THLD_DEFAULT;
> +
> +	return ENET_THLD_MAX;
> +}
> +
>  /* A buffer has to hold everything the threshold lets the hardware deliver */
>  static unsigned int bcmgenet_rx_buf_len(unsigned int mtu)
>  {
> @@ -2699,8 +2782,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_MAC_FRAME_LEN(mtu), UMAC_MAX_FRAME_LEN);
>  
>  	/* GENET v1 maps other registers at these offsets */
> @@ -2708,6 +2793,8 @@ 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(tx_thld, priv->base + priv->hw_params->tbuf_offset +
> +			TBUF_PKT_RDY_THLD);
>  }

[Severity: Medium]
Does ENET_THLD_MAX (3840 bytes) fit in the TBUF of every GENET version
that gets it here? bcmgenet_tx_pkt_rdy_thld() returns the same value
for all versions. bcmgenet_set_mtu_regs() then writes it on v2 and
later, with no clamp per version.

init_umac() programs the TBUF size control only on v3 and later:

	if (!GENET_IS_V1(priv) && !GENET_IS_V2(priv))
		bcmgenet_rbuf_writel(priv, 1, RBUF_TBUF_SIZE_CTRL);

So v2 has different TBUF sizing but gets the same threshold. The
measurements in the commit message are from a v5 only.

Suppose a core's TBUF cannot hold 3840 bytes. Could a frame longer than
that TBUF then never reach the threshold and stall the transmitter?

"net: bcmgenet: allow the MTU to be changed" later sets
dev->max_mtu = ENET_MAX_MTU for every non-v1 core. That would make this
setting reachable on v2 through v4.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-nb-genet-mtu-nn-v2-v7-0-43a5d90b3256%40tipi-net.de

[PATCH net-next v7 7/9] net: bcmgenet: build the DMA rings before giving up the running ones

[Nicolai Buchwitz]

bcmgenet_init_dma() allocates the new rings only after its callers have
torn the running ones down, so a failure under memory pressure leaves the
interface with no datapath at all. The MTU change in the next patch runs
the same path on a live interface, where that takes a working link down.

Allocate into a set of its own first and install it only once it is
complete, so a failed allocation costs the running datapath nothing.

The receive buffers can only reach the hardware at install time, the
descriptors of a running ring must not be rewritten.

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

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d70079832dd24..3cbffbc333dea 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2373,16 +2373,15 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	goto out;
 }
 
-static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
-			      struct enet_cb *cb)
+static int bcmgenet_rx_alloc_cb(struct bcmgenet_priv *priv,
+				struct page_pool *pool, unsigned int buf_len,
+				struct enet_cb *cb)
 {
-	struct bcmgenet_priv *priv = ring->priv;
-	unsigned int size = SKB_HEAD_ALIGN(priv->rx_buf_len);
+	unsigned int size = SKB_HEAD_ALIGN(buf_len);
 	unsigned int offset;
-	dma_addr_t mapping;
 	struct page *page;
 
-	page = page_pool_dev_alloc(ring->page_pool, &offset, &size);
+	page = page_pool_dev_alloc(pool, &offset, &size);
 	if (!page) {
 		priv->mib.alloc_rx_buff_failed++;
 		netif_err(priv, rx_err, priv->dev,
@@ -2390,15 +2389,37 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
 		return -ENOMEM;
 	}
 
+	cb->rx_page = page;
+	cb->rx_offset = offset;
+	cb->rx_size = size;
+
+	return 0;
+}
+
+/* Hand the buffer of a control block to the hardware */
+static void bcmgenet_rx_arm_cb(struct bcmgenet_priv *priv, struct enet_cb *cb)
+{
+	dma_addr_t mapping;
+
 	/* page_pool handles DMA mapping via PP_FLAG_DMA_MAP */
-	mapping = page_pool_get_dma_addr(page) + offset;
+	mapping = page_pool_get_dma_addr(cb->rx_page) + cb->rx_offset;
 	dma_sync_single_for_device(&priv->pdev->dev, mapping, priv->rx_buf_len,
 				   DMA_FROM_DEVICE);
 
-	cb->rx_page = page;
-	cb->rx_offset = offset;
-	cb->rx_size = size;
 	dmadesc_set_addr(priv, cb->bd_addr, mapping);
+}
+
+static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
+			      struct enet_cb *cb)
+{
+	struct bcmgenet_priv *priv = ring->priv;
+	int ret;
+
+	ret = bcmgenet_rx_alloc_cb(priv, ring->page_pool, priv->rx_buf_len, cb);
+	if (ret)
+		return ret;
+
+	bcmgenet_rx_arm_cb(priv, cb);
 
 	return 0;
 }
@@ -2646,22 +2667,15 @@ static void bcmgenet_dim_work(struct work_struct *work)
 }
 
 /* Assign page_pool pages to RX DMA descriptors. */
-static int bcmgenet_alloc_rx_buffers(struct bcmgenet_priv *priv,
-				     struct bcmgenet_rx_ring *ring)
+static void bcmgenet_arm_rx_buffers(struct bcmgenet_priv *priv,
+				    struct bcmgenet_rx_ring *ring)
 {
-	struct enet_cb *cb;
 	int i;
 
 	netif_dbg(priv, hw, priv->dev, "%s\n", __func__);
 
-	/* loop here for each buffer needing assign */
-	for (i = 0; i < ring->size; i++) {
-		cb = ring->cbs + i;
-		if (bcmgenet_rx_refill(ring, cb))
-			return -ENOMEM;
-	}
-
-	return 0;
+	for (i = 0; i < ring->size; i++)
+		bcmgenet_rx_arm_cb(priv, ring->cbs + i);
 }
 
 static void bcmgenet_free_rx_buffers(struct bcmgenet_priv *priv)
@@ -2950,41 +2964,33 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
 	netif_napi_add_tx(priv->dev, &ring->napi, bcmgenet_tx_poll);
 }
 
-static int bcmgenet_rx_ring_create_pool(struct bcmgenet_priv *priv,
-					struct bcmgenet_rx_ring *ring)
+static struct page_pool *bcmgenet_create_rx_pool(struct bcmgenet_priv *priv,
+						 unsigned int size)
 {
 	/* Buffers may share a page, depending on PAGE_SIZE and the MTU.
-	 * bcmgenet_rx_refill() syncs each one for the device,
+	 * bcmgenet_rx_arm_cb() syncs each one for the device,
 	 * PP_FLAG_DMA_SYNC_DEV would sync the whole page.
 	 */
 	struct page_pool_params pp_params = {
 		.order = 0,
 		.flags = PP_FLAG_DMA_MAP,
-		.pool_size = ring->size,
+		.pool_size = size,
 		.nid = NUMA_NO_NODE,
 		.dev = &priv->pdev->dev,
 		.dma_dir = DMA_FROM_DEVICE,
 	};
-	int err;
-
-	ring->page_pool = page_pool_create(&pp_params);
-	if (IS_ERR(ring->page_pool)) {
-		err = PTR_ERR(ring->page_pool);
-		ring->page_pool = NULL;
-		return err;
-	}
 
-	return 0;
+	return page_pool_create(&pp_params);
 }
 
 /* Initialize a RDMA ring */
-static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
-				 unsigned int index, unsigned int size,
-				 unsigned int start_ptr, unsigned int end_ptr)
+static void bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
+				  unsigned int index, unsigned int size,
+				  unsigned int start_ptr, unsigned int end_ptr,
+				  struct page_pool *pool)
 {
 	struct bcmgenet_rx_ring *ring = &priv->rx_rings[index];
 	u32 words_per_bd = WORDS_PER_BD(priv);
-	int ret, i;
 
 	ring->priv = priv;
 	ring->index = index;
@@ -2994,19 +3000,9 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
 	ring->read_ptr = start_ptr;
 	ring->cb_ptr = start_ptr;
 	ring->end_ptr = end_ptr - 1;
+	ring->page_pool = pool;
 
-	ret = bcmgenet_rx_ring_create_pool(priv, ring);
-	if (ret)
-		return ret;
-
-	ret = bcmgenet_alloc_rx_buffers(priv, ring);
-	if (ret) {
-		for (i = 0; i < ring->size; i++)
-			bcmgenet_free_rx_cb(ring->cbs + i, ring->page_pool);
-		page_pool_destroy(ring->page_pool);
-		ring->page_pool = NULL;
-		return ret;
-	}
+	bcmgenet_arm_rx_buffers(priv, ring);
 
 	bcmgenet_init_dim(ring, bcmgenet_dim_work);
 	bcmgenet_init_rx_coalesce(ring);
@@ -3033,8 +3029,6 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
 				  RDMA_WRITE_PTR);
 	bcmgenet_rdma_ring_writel(priv, index, end_ptr * words_per_bd - 1,
 				  DMA_END_ADDR);
-
-	return ret;
 }
 
 static void bcmgenet_enable_tx_napi(struct bcmgenet_priv *priv)
@@ -3222,18 +3216,17 @@ static void bcmgenet_destroy_rx_page_pools(struct bcmgenet_priv *priv)
  *
  * Queue 0 is also the default Rx queue with GENET_Q0_RX_BD_CNT descriptors.
  */
-static int bcmgenet_init_rx_queues(struct net_device *dev)
+static void bcmgenet_init_rx_queues(struct net_device *dev,
+				    struct bcmgenet_dma_set *set)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	unsigned int start = 0, end = GENET_Q0_RX_BD_CNT;
 	u32 i, ring_mask;
-	int ret;
 
 	/* Initialize Rx priority queues */
 	for (i = 0; i <= priv->hw_params->rx_queues; i++) {
-		ret = bcmgenet_init_rx_ring(priv, i, end - start, start, end);
-		if (ret)
-			return ret;
+		bcmgenet_init_rx_ring(priv, i, end - start, start, end,
+				      set->page_pool[i]);
 
 		start = end;
 		end += priv->hw_params->rx_bds_per_q;
@@ -3246,8 +3239,6 @@ static int bcmgenet_init_rx_queues(struct net_device *dev)
 	/* Enable Rx rings */
 	ring_mask <<= DMA_RING_BUF_EN_SHIFT;
 	bcmgenet_rdma_writel(priv, ring_mask, DMA_CTRL);
-
-	return 0;
 }
 
 static int bcmgenet_dma_teardown(struct bcmgenet_priv *priv)
@@ -3293,11 +3284,86 @@ static void bcmgenet_fini_dma(struct bcmgenet_priv *priv)
 	priv->tx_cbs = NULL;
 }
 
-/* init_edma: Initialize DMA control register */
-static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
+/* Hand back a set that was never installed */
+static void bcmgenet_free_dma_set(struct bcmgenet_priv *priv,
+				  struct bcmgenet_dma_set *set)
+{
+	unsigned int start = 0, end = GENET_Q0_RX_BD_CNT;
+	unsigned int i, q;
+
+	for (q = 0; q <= priv->hw_params->rx_queues; q++) {
+		if (set->page_pool[q]) {
+			for (i = start; i < end; i++)
+				bcmgenet_free_rx_cb(set->rx_cbs + i,
+						    set->page_pool[q]);
+			page_pool_destroy(set->page_pool[q]);
+		}
+
+		start = end;
+		end += priv->hw_params->rx_bds_per_q;
+	}
+
+	kfree(set->rx_cbs);
+	kfree(set->tx_cbs);
+	memset(set, 0, sizeof(*set));
+}
+
+/* Build everything a datapath of this buffer length needs. The running one
+ * keeps its own until the caller installs this set, so a failure here costs
+ * nothing.
+ */
+static int bcmgenet_alloc_dma(struct bcmgenet_priv *priv, unsigned int buf_len,
+			      struct bcmgenet_dma_set *set)
+{
+	unsigned int start = 0, end = GENET_Q0_RX_BD_CNT;
+	unsigned int i, q;
+
+	memset(set, 0, sizeof(*set));
+
+	set->rx_cbs = kzalloc_objs(struct enet_cb, TOTAL_DESC);
+	set->tx_cbs = kzalloc_objs(struct enet_cb, TOTAL_DESC);
+	if (!set->rx_cbs || !set->tx_cbs)
+		goto err;
+
+	for (i = 0; i < TOTAL_DESC; i++) {
+		set->rx_cbs[i].bd_addr = priv->base +
+					 priv->hw_params->rdma_offset +
+					 i * DMA_DESC_SIZE;
+		set->tx_cbs[i].bd_addr = priv->base +
+					 priv->hw_params->tdma_offset +
+					 i * DMA_DESC_SIZE;
+	}
+
+	for (q = 0; q <= priv->hw_params->rx_queues; q++) {
+		set->page_pool[q] = bcmgenet_create_rx_pool(priv, end - start);
+		if (IS_ERR(set->page_pool[q])) {
+			set->page_pool[q] = NULL;
+			goto err;
+		}
+
+		for (i = start; i < end; i++)
+			if (bcmgenet_rx_alloc_cb(priv, set->page_pool[q],
+						 buf_len, set->rx_cbs + i))
+				goto err;
+
+		start = end;
+		end += priv->hw_params->rx_bds_per_q;
+	}
+
+	return 0;
+
+err:
+	bcmgenet_free_dma_set(priv, set);
+
+	return -ENOMEM;
+}
+
+/* Install a set built by bcmgenet_alloc_dma() and start the engines. The set
+ * is consumed either way, a failure frees it and leaves no datapath behind.
+ */
+static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx,
+			     struct bcmgenet_dma_set *set)
 {
-	struct enet_cb *cb;
-	unsigned int i;
 	int ret;
 	u32 reg;
 
@@ -3307,14 +3373,14 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
 	ret = bcmgenet_tdma_disable(priv);
 	if (ret) {
 		netdev_err(priv->dev, "failed to halt Tx DMA\n");
-		return ret;
+		goto err;
 	}
 
 	/* Disable RX DMA */
 	ret = bcmgenet_rdma_disable(priv);
 	if (ret) {
 		netdev_err(priv->dev, "failed to halt Rx DMA\n");
-		return ret;
+		goto err;
 	}
 
 	/* Flush TX queues */
@@ -3330,49 +3396,21 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
 		udelay(10);
 	}
 
-	/* Initialize common Rx ring structures */
+	/* The set belongs to the datapath from here on */
 	priv->rx_bds = priv->base + priv->hw_params->rdma_offset;
 	priv->num_rx_bds = TOTAL_DESC;
-	priv->rx_cbs = kzalloc_objs(struct enet_cb, priv->num_rx_bds);
-	if (!priv->rx_cbs)
-		return -ENOMEM;
+	priv->rx_cbs = set->rx_cbs;
 
-	for (i = 0; i < priv->num_rx_bds; i++) {
-		cb = priv->rx_cbs + i;
-		cb->bd_addr = priv->rx_bds + i * DMA_DESC_SIZE;
-	}
-
-	/* Initialize common TX ring structures */
 	priv->tx_bds = priv->base + priv->hw_params->tdma_offset;
 	priv->num_tx_bds = TOTAL_DESC;
-	priv->tx_cbs = kzalloc_objs(struct enet_cb, priv->num_tx_bds);
-	if (!priv->tx_cbs) {
-		kfree(priv->rx_cbs);
-		priv->rx_cbs = NULL;
-		return -ENOMEM;
-	}
-
-	for (i = 0; i < priv->num_tx_bds; i++) {
-		cb = priv->tx_cbs + i;
-		cb->bd_addr = priv->tx_bds + i * DMA_DESC_SIZE;
-	}
+	priv->tx_cbs = set->tx_cbs;
 
 	/* Init rDma */
 	bcmgenet_rdma_writel(priv, priv->dma_max_burst_length,
 			     DMA_SCB_BURST_SIZE);
 
 	/* Initialize Rx queues */
-	ret = bcmgenet_init_rx_queues(priv->dev);
-	if (ret) {
-		netdev_err(priv->dev, "failed to initialize Rx queues\n");
-		bcmgenet_free_rx_buffers(priv);
-		bcmgenet_destroy_rx_page_pools(priv);
-		kfree(priv->rx_cbs);
-		priv->rx_cbs = NULL;
-		kfree(priv->tx_cbs);
-		priv->tx_cbs = NULL;
-		return ret;
-	}
+	bcmgenet_init_rx_queues(priv->dev, set);
 
 	/* Init tDma */
 	bcmgenet_tdma_writel(priv, priv->dma_max_burst_length,
@@ -3391,6 +3429,11 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx)
 	bcmgenet_tdma_writel(priv, reg, DMA_CTRL);
 
 	return 0;
+
+err:
+	bcmgenet_free_dma_set(priv, set);
+
+	return ret;
 }
 
 /* Interrupt bottom half */
@@ -3563,6 +3606,7 @@ static void bcmgenet_netif_start(struct net_device *dev, bool start_phy)
 static int bcmgenet_open(struct net_device *dev)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
+	struct bcmgenet_dma_set set;
 	int ret;
 
 	netif_dbg(priv, ifup, dev, "bcmgenet_open\n");
@@ -3592,7 +3636,11 @@ static int bcmgenet_open(struct net_device *dev)
 	bcmgenet_hfb_restore(priv);
 
 	/* Reinitialize TDMA and RDMA and SW housekeeping */
-	ret = bcmgenet_init_dma(priv, true);
+	ret = bcmgenet_alloc_dma(priv, priv->rx_buf_len, &set);
+	if (ret)
+		goto err_clk_disable;
+
+	ret = bcmgenet_init_dma(priv, true, &set);
 	if (ret) {
 		netdev_err(dev, "failed to initialize DMA\n");
 		goto err_clk_disable;
@@ -4446,6 +4494,7 @@ static int bcmgenet_resume(struct device *d)
 	struct net_device *dev = dev_get_drvdata(d);
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct bcmgenet_rxnfc_rule *rule;
+	struct bcmgenet_dma_set set;
 	int ret;
 	u32 reg;
 
@@ -4511,7 +4560,9 @@ static int bcmgenet_resume(struct device *d)
 	bcmgenet_hfb_restore(priv);
 
 	/* Reinitialize TDMA and RDMA and SW housekeeping */
-	ret = bcmgenet_init_dma(priv, false);
+	ret = bcmgenet_alloc_dma(priv, priv->rx_buf_len, &set);
+	if (!ret)
+		ret = bcmgenet_init_dma(priv, false, &set);
 	if (ret) {
 		/* Leave the block clocked, close and suspend tear it down */
 		netdev_err(dev, "failed to initialize DMA\n");
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 12a3dfa807393..c623c5e9de97a 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -541,6 +541,15 @@ struct bcmgenet_hw_params {
 	u32		words_per_bd;
 };
 
+/* Everything an open datapath needs that can fail to be allocated. Built
+ * before the running one is given up, so a failure changes nothing.
+ */
+struct bcmgenet_dma_set {
+	struct enet_cb *rx_cbs;
+	struct enet_cb *tx_cbs;
+	struct page_pool *page_pool[GENET_MAX_MQ_CNT + 1];
+};
+
 struct bcmgenet_skb_cb {
 	struct enet_cb *first_cb;	/* First control block of SKB */
 	struct enet_cb *last_cb;	/* Last control block of SKB */

-- 
2.53.0

[PATCH net-next v7 8/9] 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. The MAC accepts a little more
than one descriptor holds, so a double tagged frame at that maximum is
dropped as fragmented until a later patch reassembles those.

The PHY keeps running across the change so the link stays up, with its
lock held so that a link event cannot enable the MAC while the registers
change. Two MTUs at or below ETH_DATA_LEN derive the same values, so a
change between them needs no restart at all.

Only a DMA that refuses to halt still takes the interface down, because
nothing is left to run on.

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>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 86 ++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 3cbffbc333dea..30ba77931c4f0 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -84,6 +84,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))
@@ -3981,6 +3987,79 @@ 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;
+	struct bcmgenet_dma_set set;
+	int ret;
+
+	/* Below ETH_DATA_LEN everything is floored, so there is nothing to
+	 * reprogram. Same when a failed resume left the datapath down.
+	 */
+	if (!netif_running(dev) || !priv->tx_cbs ||
+	    max_t(unsigned int, old_mtu, ETH_DATA_LEN) ==
+	    max_t(unsigned int, new_mtu, ETH_DATA_LEN)) {
+		WRITE_ONCE(dev->mtu, new_mtu);
+		priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
+		return 0;
+	}
+
+	ret = bcmgenet_alloc_dma(priv, bcmgenet_rx_buf_len(new_mtu), &set);
+	if (ret)
+		return ret;
+
+	/* The watchdog trips on an idle queue once the rings are gone */
+	netif_device_detach(dev);
+
+	/* The PHY stays up, hold its lock. A link event would otherwise take
+	 * the MAC out of reset and enable it mid rewrite.
+	 */
+	mutex_lock(&dev->phydev->lock);
+	bcmgenet_netif_stop(dev, 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, &set);
+	if (ret) {
+		/* The datapath is gone, do not leave the interface up */
+		netdev_err(dev, "failed to initialize DMA, closing\n");
+		WRITE_ONCE(dev->mtu, old_mtu);
+		priv->rx_buf_len = bcmgenet_rx_buf_len(old_mtu);
+		mutex_unlock(&dev->phydev->lock);
+		/* bcmgenet_close() attaches the device again */
+		netif_close(dev);
+		return ret;
+	}
+
+	bcmgenet_hfb_restore(priv);
+	bcmgenet_netif_start(dev, false);
+	mutex_unlock(&dev->phydev->lock);
+
+	/* 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);
+
+	/* Interrupts were off, so a latched link event is lost. Internal PHYs
+	 * on v1-v4 are not polled either.
+	 */
+	phy_mac_interrupt(dev->phydev);
+
+	netif_device_attach(dev);
+
+	/* Filter updates that came in while the device was detached got
+	 * dropped. An IPv6 join does not hold RTNL.
+	 */
+	netif_addr_lock_bh(dev);
+	bcmgenet_set_rx_mode(dev);
+	netif_addr_unlock_bh(dev);
+
+	return 0;
+}
+
 static const struct net_device_ops bcmgenet_netdev_ops = {
 	.ndo_open		= bcmgenet_open,
 	.ndo_stop		= bcmgenet_close,
@@ -3992,6 +4071,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 */
@@ -4345,8 +4425,14 @@ 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);
 	priv->tx_thld_len = bcmgenet_tx_pkt_rdy_thld(dev->mtu) * ENET_THLD_UNIT;
+
+	/* v1 cannot program the thresholds, so it stays at the default 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");

-- 
2.53.0

Re: [PATCH net-next v7 8/9] 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 · 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] The probe change raises dev->max_mtu for every version except
  v1 (bcmgenet.c:4433-4434, `if (!GENET_IS_V1(priv)) dev->max_mtu =…
- [Medium] net: bcmgenet: mismatched dma_unmap_page() in
  bcmgenet_free_tx_cb()
- [Low] bcmgenet_change_mtu()
  (drivers/net/ethernet/broadcom/genet/bcmgenet.c:3990) restarts the…

--

net: bcmgenet: allow the MTU to be changed

This commit sets dev->max_mtu for GENET v2 and later. The value comes from
the packet ready threshold and the page size. It also adds
bcmgenet_change_mtu(), which restarts the DMA datapath in place while the
PHY keeps running. MTU changes that stay at or below ETH_DATA_LEN skip the
restart.

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

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 3cbffbc333dea..30ba77931c4f0 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> +static int bcmgenet_change_mtu(struct net_device *dev, int new_mtu)
> +{

[ ... ]

> +	/* The PHY stays up, hold its lock. A link event would otherwise take
> +	 * the MAC out of reset and enable it mid rewrite.
> +	 */
> +	mutex_lock(&dev->phydev->lock);
> +	bcmgenet_netif_stop(dev, false);

[ ... ]

> +	bcmgenet_hfb_restore(priv);
> +	bcmgenet_netif_start(dev, false);
> +	mutex_unlock(&dev->phydev->lock);
> +
> +	/* 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);
> +
> +	/* Interrupts were off, so a latched link event is lost. Internal PHYs
> +	 * on v1-v4 are not polled either.
> +	 */
> +	phy_mac_interrupt(dev->phydev);

[Severity: Low]
Does this restart also lose a pending UMAC_IRQ_PHY_DET_R event?

Inside bcmgenet_netif_stop(), bcmgenet_intr_disable() clears the latched
INTRL2_0 status. After that, the bottom half is cancelled:

bcmgenet_netif_stop() {
    ...
	cancel_work_sync(&priv->bcmgenet_irq_work);
    ...
}

If the work was queued but had not run yet, its bits stay in
priv->irq0_stat and nothing queues the work again.

On v1-v3 with an internal PHY, bcmgenet_link_intr_enable() enables
UMAC_IRQ_PHY_DET_R. Only bcmgenet_irq_task() handles it:

	if (status & UMAC_IRQ_PHY_DET_R &&
	    priv->dev->phydev->autoneg != AUTONEG_ENABLE) {
		phy_init_hw(priv->dev->phydev);
		genphy_config_aneg(priv->dev->phydev);

This is the 40nm EPHY energy-detect workaround from commit 25382b991d25
("net: bcmgenet: reset 40nm EPHY on energy detect").

The link event is replayed with phy_mac_interrupt(), but the PHY_DET_R
event is not. If an energy-detect event lands in the restart window, which
includes the sleep in bcmgenet_dma_teardown(), or sits in the cancelled
work item, is the EPHY soft reset skipped until some unrelated isr0 event
queues the work again?

Before this patch, bcmgenet_netif_stop() was only followed by close/open
or suspend/resume, and both reinitialise the PHY.

Would it work to schedule bcmgenet_irq_work after bcmgenet_netif_start()
when priv->irq0_stat is non-zero? Another option is to replay the PHY_DET_R
handling the same way the link event is replayed.

The later patch "reassemble jumbo frames from status block fragments" does
not touch bcmgenet_change_mtu(), so this seems to remain at the end of the
series.

[ ... ]

> @@ -4345,8 +4425,14 @@ 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);
>  	priv->tx_thld_len = bcmgenet_tx_pkt_rdy_thld(dev->mtu) * ENET_THLD_UNIT;
> +
> +	/* v1 cannot program the thresholds, so it stays at the default MTU */
> +	if (!GENET_IS_V1(priv))
> +		dev->max_mtu = ENET_MAX_MTU;

[Severity: Medium]
Has GENET v2 been confirmed to handle the larger thresholds this enables?

On v2, any MTU above ETH_DATA_LEN makes bcmgenet_tx_pkt_rdy_thld() return
ENET_THLD_MAX (0xf0, 3840 bytes). bcmgenet_set_mtu_regs() then programs
that value, together with a larger RBUF_PKT_RDY_THLD and
UMAC_MAX_FRAME_LEN.

init_umac() only programs the buffer sizing register on v3 and later:

	if (!GENET_IS_V1(priv) && !GENET_IS_V2(priv))
		bcmgenet_rbuf_writel(priv, 1, RBUF_TBUF_SIZE_CTRL);

So v2 keeps whatever RBUF/TBUF partitioning it has at reset. Its RBUF
register layout also differs from v3 and later, for example
RBUF_OVFL_CNT_V2 versus RBUF_OVFL_CNT_V3PLUS.

The commit message only explains why v1 is excluded.

What if the v2 TBUF cannot buffer up to the 3840 byte threshold, or the v2
RBUF cannot hold the larger frames? Large-frame TX could then stall, and
the earlier patch "split frames that end just past the transmit threshold"
shows this can leave TX DMA unable to halt. RX could also overflow.

The last patch in the series raises ENET_MAX_MTU for v2 to 16347 and keeps
the 0xf0 TX threshold. Its RTL confirmation covers only RX frame splitting.

Should max_mtu be limited to the versions known to work until v2 is
confirmed on hardware?

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

[ ... ]

> @@ -3981,6 +3987,79 @@ 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;
> +	struct bcmgenet_dma_set set;
> +	int ret;
> +
> +	/* Below ETH_DATA_LEN everything is floored, so there is nothing to
> +	 * reprogram. Same when a failed resume left the datapath down.
> +	 */
> +	if (!netif_running(dev) || !priv->tx_cbs ||
> +	    max_t(unsigned int, old_mtu, ETH_DATA_LEN) ==
> +	    max_t(unsigned int, new_mtu, ETH_DATA_LEN)) {
> +		WRITE_ONCE(dev->mtu, new_mtu);
> +		priv->rx_buf_len = bcmgenet_rx_buf_len(new_mtu);
> +		return 0;
> +	}
> +
> +	ret = bcmgenet_alloc_dma(priv, bcmgenet_rx_buf_len(new_mtu), &set);
> +	if (ret)
> +		return ret;
> +
> +	/* The watchdog trips on an idle queue once the rings are gone */
> +	netif_device_detach(dev);
> +
> +	/* The PHY stays up, hold its lock. A link event would otherwise take
> +	 * the MAC out of reset and enable it mid rewrite.
> +	 */
> +	mutex_lock(&dev->phydev->lock);
> +	bcmgenet_netif_stop(dev, 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, &set);
> +	if (ret) {
> +		/* The datapath is gone, do not leave the interface up */
> +		netdev_err(dev, "failed to initialize DMA, closing\n");
> +		WRITE_ONCE(dev->mtu, old_mtu);
> +		priv->rx_buf_len = bcmgenet_rx_buf_len(old_mtu);
> +		mutex_unlock(&dev->phydev->lock);
> +		/* bcmgenet_close() attaches the device again */
> +		netif_close(dev);
> +		return ret;
> +	}
> +
> +	bcmgenet_hfb_restore(priv);
> +	bcmgenet_netif_start(dev, false);
> +	mutex_unlock(&dev->phydev->lock);
> +
> +	/* 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);
> +
> +	/* Interrupts were off, so a latched link event is lost. Internal PHYs
> +	 * on v1-v4 are not polled either.
> +	 */
> +	phy_mac_interrupt(dev->phydev);
> +
> +	netif_device_attach(dev);
> +
> +	/* Filter updates that came in while the device was detached got
> +	 * dropped. An IPv6 join does not hold RTNL.
> +	 */
> +	netif_addr_lock_bh(dev);
> +	bcmgenet_set_rx_mode(dev);
> +	netif_addr_unlock_bh(dev);
> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops bcmgenet_netdev_ops = {
>  	.ndo_open		= bcmgenet_open,
>  	.ndo_stop		= bcmgenet_close,

[Severity: Medium]
Once this lets frames grow past priv->tx_thld_len, does the threshold
split in bcmgenet_xmit() release the head buffer with the wrong DMA API
call?

For a linear skb, skb_headlen() already covers priv->tx_thld_len. The
covered < priv->tx_thld_len loop in bcmgenet_xmit() then never runs,
so split_i stays 0. On the first descriptor, GENET_CB(skb)->first_cb is
set to that control block and skb->data is mapped with
dma_map_single(). Because i == split_i, the same control block also
becomes GENET_CB(skb)->split_cb. Its dma_addr and dma_len are cleared,
and the mapping passes to the next control block through
split_mapping.

bcmgenet_free_tx_cb() then does:

	if (cb != GENET_CB(skb)->split_cb) {
		if (cb == GENET_CB(skb)->first_cb)
			dma_unmap_single(...);
		else
			dma_unmap_page(...);

The first control block is skipped because it is split_cb. The
continuation control block now owns the dma_map_single() mapping, but
it is not first_cb, so it gets dma_unmap_page(). TX completion, ring
reclaim and the out_unmap_frags unwind all take this path.

On most platforms both calls end up doing the same work. Still, the
mapping type mismatch breaks the DMA API contract, and
CONFIG_DMA_API_DEBUG will complain about it for these frames.

At the default MTU the split window does not appear to be reachable. It
looks like bcmgenet_change_mtu() and the larger ENET_MAX_MTU are what
expose it. The code itself comes from the earlier patch "split frames
that end just past the transmit threshold", so the fix probably belongs
there.

What is the preferred way to track this? One option is to record on
the control block that owns the mapping whether it came from
dma_map_single(), instead of comparing against first_cb. Another is to
move first_cb to the continuation descriptor when split_i is 0.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-nb-genet-mtu-nn-v2-v7-0-43a5d90b3256%40tipi-net.de

[PATCH net-next v7 9/9] 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.

On transmit 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.

The TX descriptor length field is wider than DMA_BUFLENGTH_MASK suggests,
that mask only extracts the length from the RX status word. A frame of
16361 bytes, the largest untagged frame that MTU allows, came back byte
exact 200 times over UMAC loopback.

Suggested-by: Justin Chen <justin.chen@broadcom.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 131 ++++++++++++++++++++-----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   2 +
 2 files changed, 111 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 30ba77931c4f0..eb6975459917b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -84,11 +84,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_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)
@@ -2430,6 +2427,52 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
 	return 0;
 }
 
+/* The rest of the frame carries no SOP. Drop it and do not count it again
+ * until the next SOP, unless this descriptor already ended the frame.
+ */
+static void bcmgenet_discard_frags(struct bcmgenet_rx_ring *ring, bool eop)
+{
+	ring->frag_drop = !eop;
+
+	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. Returns the frame once EOP is in. NULL means either
+ * more descriptors are coming or the frame is gone.
+ */
+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, dma_flag & DMA_EOP);
+		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.
  */
@@ -2447,6 +2490,7 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 	unsigned int rxpktprocessed = 0, rxpkttoprocess;
 	unsigned int bytes_processed = 0;
 	unsigned int p_index, mask;
+	unsigned int rxframes = 0;
 	unsigned int discards;
 
 	/* Clear status before servicing to reduce spurious interrupts */
@@ -2494,6 +2538,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, false);
 			goto next;
 		}
 
@@ -2518,24 +2563,42 @@ 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);
+		/* Only the first descriptor carries the alignment pad. A head
+		 * with more to come needs the Ethernet header in it, a
+		 * continuation needs at least one payload byte.
+		 */
+		if (dma_flag & DMA_SOP) {
+			min_len = GENET_RSB_PAD;
+			if (!(dma_flag & DMA_EOP))
+				min_len += ETH_HLEN;
+		} else {
+			min_len = sizeof(struct status_64) + 1;
+		}
 
 		/* Reject lengths that would underflow the SKB build path. */
 		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);
+			bcmgenet_discard_frags(ring, dma_flag & DMA_EOP);
 			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 picks up again after an incomplete frame */
+		if (dma_flag & DMA_SOP) {
+			if (ring->frag_head) {
+				BCMGENET_STATS64_INC(stats, fragmented_errors);
+				bcmgenet_discard_frags(ring, false);
+			}
+			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);
+				ring->frag_drop = true;
+			}
 			page_pool_put_full_page(ring->page_pool, rx_page,
 						true);
 			goto next;
@@ -2565,17 +2628,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, dma_flag & DMA_EOP);
 			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, dma_flag & DMA_EOP);
 			page_pool_put_full_page(ring->page_pool, rx_page,
 						true);
 			goto next;
@@ -2587,17 +2660,26 @@ 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 (priv->crc_fwd_en) {
-			skb_trim(skb, skb->len - ETH_FCS_LEN);
+		if (unlikely(!(dma_flag & DMA_EOP))) {
+			ring->frag_head = skb;
+			goto next;
+		}
+
+deliver:
+		/* The FCS trim may release the page with the last status block */
+		rx_csum = (__force __be16)(status->rx_csum & 0xffff);
+
+		if (priv->crc_fwd_en &&
+		    unlikely(pskb_trim(skb, skb->len - ETH_FCS_LEN))) {
+			BCMGENET_STATS64_INC(stats, dropped);
+			dev_kfree_skb_any(skb);
+			goto next;
 		}
 
 		/* Set up checksum offload */
-		if (dev->features & NETIF_F_RXCSUM) {
-			rx_csum = (__force __be16)(status->rx_csum & 0xffff);
-			if (rx_csum) {
-				skb->csum = (__force __wsum)ntohs(rx_csum);
-				skb->ip_summed = CHECKSUM_COMPLETE;
-			}
+		if ((dev->features & NETIF_F_RXCSUM) && rx_csum) {
+			skb->csum = (__force __wsum)ntohs(rx_csum);
+			skb->ip_summed = CHECKSUM_COMPLETE;
 		}
 
 		len = skb->len;
@@ -2605,6 +2687,7 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 
 		/*Finish setting up the received SKB and send it to the kernel*/
 		skb->protocol = eth_type_trans(skb, priv->dev);
+		rxframes++;
 
 		u64_stats_update_begin(&stats->syncp);
 		u64_stats_inc(&stats->packets);
@@ -2631,7 +2714,8 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 	}
 
 	ring->dim.bytes = bytes_processed;
-	ring->dim.packets = rxpktprocessed;
+	/* A long frame spans several descriptors but is one packet */
+	ring->dim.packets = rxframes;
 
 	return rxpktprocessed;
 }
@@ -2698,6 +2782,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, true);
 	}
 }
 
@@ -2768,7 +2854,8 @@ static void bcmgenet_link_intr_enable(struct bcmgenet_priv *priv)
 }
 
 /* Receive threshold in register units. Covers the alignment bytes and the
- * frame, but not the status block, which the hardware adds on top.
+ * frame up to the register maximum, but not the status block, which the
+ * hardware adds on top. A longer frame arrives in several descriptors.
  */
 static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
 {
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index c623c5e9de97a..cdab1205c841c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -597,6 +597,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 v7 9/9] net: bcmgenet: reassemble jumbo frames from status block fragments

[netdev-bot+sashiko]

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

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

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] This patch raises dev->max_mtu to ENET_MAX_MTU = GENMASK(13,0)
  - ENET_FRAME_OVERHEAD = 16347 on every GENET revision except v1…
- [Low] The comment this patch rewrote above bcmgenet_pkt_rdy_thld() says
  the receive threshold "covers the alignment bytes and the frame up to…
- [Low] In bcmgenet_desc_rx() the length check (`if (unlikely(len >
  priv->rx_buf_len || len < min_len))`) runs before the new…
- [Low] net: bcmgenet: frag_drop not cleared on EOP in bcmgenet_desc_rx()

--

net: bcmgenet: reassemble jumbo frames from status block fragments

When a frame is longer than the packet ready threshold, the hardware
splits it across several RX descriptors. This patch reassembles those
frames and strips the per-descriptor status blocks. It also raises
ENET_MAX_MTU to the 14 bit UMAC_MAX_FRAME_LEN limit, which allows an MTU
of up to 16347.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 30ba77931c4f0..eb6975459917b 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -84,11 +84,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_MTU		(GENMASK(13, 0) - ENET_FRAME_OVERHEAD)

[Severity: Medium]
Has the wider TX descriptor length field been confirmed on GENET v2, v3
and v4, or only on v5?

bcmgenet_probe() applies the new maximum to every revision except v1:

	if (!GENET_IS_V1(priv))
		dev->max_mtu = ENET_MAX_MTU;

Before this change, max_mtu was tied to the single-descriptor RX
threshold (about 3564 with 4K pages). So no TX buffer passed to
bcmgenet_xmit() could reach 4096 bytes, even with the 64 byte TSB added.

With an MTU near 16K, a linear head can now be 4096 bytes or more, for
example with SG off, from a packet socket, or after skb_linearize(). A
single page frag can be that large too. bcmgenet_xmit() writes the
length without clamping or splitting it:

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

bcmgenet.h still documents the field as 12 bits for both directions:

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

The commit message says the TX field is wider, but the only evidence
given is a UMAC loopback test on v5. The RTL confirmation from Broadcom
that it cites covers how RX splits long frames. It doesn't cover the
width of the TX length field.

If the TDMA on v2 to v4 decodes only 12 length bits, would TX buffers of
4096 bytes or more be sent truncated modulo 4096?

[ ... ]

> @@ -2518,24 +2563,42 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,

[ ... ]

>  		/* Reject lengths that would underflow the SKB build path. */
>  		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);
> +			bcmgenet_discard_frags(ring, dma_flag & DMA_EOP);
>  			page_pool_put_full_page(ring->page_pool, rx_page,
>  						true);
>  			goto next;
>  		}

[Severity: Low]
Should this branch clear ring->frag_drop when the descriptor carries
DMA_EOP?

bcmgenet_discard_frags() sets frag_drop to !eop. The rest of an
aborted frame is then dropped without being counted again, and
counting resumes once that frame has ended. This branch can only set
frag_drop to true. So when the last continuation of a dropped frame
lands here with DMA_EOP set, frag_drop stays true even though the
frame is over.

The same thing happens in two other cases. A lone orphan descriptor
with DMA_EOP and no SOP sets frag_drop to true here. A failure in
bcmgenet_rx_refill() calls bcmgenet_discard_frags(ring, false) before
the status word has been read, so it cannot know whether that
descriptor ended the frame.

Suppose the next frame on the ring has also lost its SOP descriptor.
Its continuations reach this branch with frag_drop already set, so
they are dropped without incrementing fragmented_errors. That lost
frame then never shows up in the error counters.

This is only a counter inaccuracy on already abnormal paths. Would
setting ring->frag_drop to !(dma_flag & DMA_EOP) here, after the
count, keep this branch consistent with bcmgenet_discard_frags()?

[Severity: Low]
Should this length check in bcmgenet_desc_rx() come after the new SOP and
frag_drop handling? Because it runs first, two cases skip the reassembly
accounting.

The first case is a frame that has already been dropped (frag_drop set,
frag_head NULL). Each later non-SOP descriptor with an out of range
length still logs "invalid packet length" and increments length_errors.

The comment on bcmgenet_discard_frags() says "do not count it again until
the next SOP". Continuations that carry DMA error flags, on the other
hand, are absorbed silently by the frag_drop branch.

The second case is a SOP descriptor with an invalid length that arrives
while ring->frag_head holds an incomplete frame.
bcmgenet_discard_frags() frees the pending frame, but fragmented_errors
is not incremented. A SOP with a valid length in the same situation does
increment it:

		if (dma_flag & DMA_SOP) {
			if (ring->frag_head) {
				BCMGENET_STATS64_INC(stats, fragmented_errors);
				bcmgenet_discard_frags(ring, false);
			}

Is the lost incomplete frame meant to go uncounted when the next SOP has
a bad length?

[ ... ]

> @@ -2768,7 +2854,8 @@ static void bcmgenet_link_intr_enable(struct bcmgenet_priv *priv)
>  }
>  
>  /* Receive threshold in register units. Covers the alignment bytes and the
> - * frame, but not the status block, which the hardware adds on top.
> + * frame up to the register maximum, but not the status block, which the
> + * hardware adds on top. A longer frame arrives in several descriptors.
>   */
>  static unsigned int bcmgenet_pkt_rdy_thld(unsigned int mtu)
>  {

[Severity: Low]
This isn't a bug, but is "up to the register maximum" accurate? The clamp
in bcmgenet_pkt_rdy_thld() uses ENET_THLD_MAX_LEN:

#define ENET_THLD_MAX_LEN	min_t(unsigned int, \
				      ENET_THLD_MAX * ENET_THLD_UNIT, \
				      ENET_THLD_PAGE_LEN)

With 4K pages, ENET_THLD_PAGE_LEN (3584) is below the register maximum
(0xf0 * 16 = 3840), so the page sets the limit.

The comment above ENET_THLD_PAGE_LEN says the same: "on 4K pages the page
bounds the threshold before the register does". The two comments now
contradict each other.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112-nb-genet-mtu-nn-v2-v7-0-43a5d90b3256%40tipi-net.de