[PATCH net 0/7] net: bcm4908_enet: TX ring and DMA fixes

[PATCH net 0/7] net: bcm4908_enet: TX ring and DMA fixes

[Kordian Chmielewski (mlody1212)]

These fix the BCM4908 Ethernet driver; the bugs were found on a
TP-Link Archer AX6000 running OpenWrt. The first patch is the
important one: the TX ring indexes wrapped at different points, so a
full ring could look empty, and under sustained load the queue stopped
for good with nothing logged.

The others drop a TX frame whose DMA mapping failed instead of
retrying it, free what stop leaves in the TX ring, unmap RX buffers
that were freed while still mapped or unmapped with the wrong size,
order the descriptor writes before OWN, and drop RX frames longer than
the buffer instead of panicking in skb_put().

The last patch guards against something seen only with a non-default
RX burst length; I am fine with it going to net-next instead.

The same changes have run on the 6.12 and 6.18 kernels of two Archer
AX6000 units and an AX11000 since the end of September. Here they are
on top of net; each patch builds with W=1.

Kordian Chmielewski (mlody1212) (7):
  net: bcm4908_enet: fix the TX ring wrap
  net: bcm4908_enet: drop a TX frame whose DMA mapping failed
  net: bcm4908_enet: release the TX ring on stop
  net: bcm4908_enet: unmap the buffer of a dropped RX frame
  net: bcm4908_enet: unmap RX buffers on stop with their mapped size
  net: bcm4908_enet: publish the buffer address before OWN
  net: bcm4908_enet: drop RX frames longer than the buffer

 drivers/net/ethernet/broadcom/bcm4908_enet.c | 106 +++++++++++++++----
 1 file changed, 83 insertions(+), 23 deletions(-)


base-commit: 86706ffa990ecd46a4326f8eecbccbbd60a6cb8d
-- 
2.56.0

[PATCH net 1/7] net: bcm4908_enet: fix the TX ring wrap

[Kordian Chmielewski (mlody1212)]

start_xmit() wraps write_idx at length - 1 and sets WRAP on the
descriptor at length - 2, while poll_tx() wraps read_idx at length,
and the free descriptor count is taken from length. With read_idx at 0
and write_idx at length - 2 the count comes out as 2, the frame is
queued and write_idx wraps to 0 == read_idx: a full ring looks empty.
The following frames overwrite slots whose skbs were never unmapped
or freed, their bytes stay in BQL, and the queue stops for good with
nothing left to wake it.

On a TP-Link Archer AX6000 this stopped all wired traffic under
sustained TX load (about 1.4 Gbit/s through the switch's CPU port):
tx_packets froze, nothing was logged, and BQL showed about 185 KB in
flight against a limit of about 10 KB.

Use every descriptor, with WRAP on the last one, wrap both indexes at
length and keep one descriptor free, so that read_idx == write_idx can
only mean an empty ring. Stop the queue as soon as the ring is full
and wake it from poll_tx() once there is room again, with the usual
barrier pairing.

Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 52 +++++++++++++-------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index fd2715f84..01a3e1d01 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -518,6 +518,16 @@ static int bcm4908_enet_stop(struct net_device *netdev)
 	return 0;
 }
 
+/* One descriptor always stays unused, so read_idx == write_idx can only
+ * mean an empty ring
+ */
+static int bcm4908_enet_tx_free_descs(struct bcm4908_enet_dma_ring *ring)
+{
+	int free = READ_ONCE(ring->read_idx) - READ_ONCE(ring->write_idx) - 1;
+
+	return free < 0 ? free + ring->length : free;
+}
+
 static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct bcm4908_enet *enet = netdev_priv(netdev);
@@ -525,20 +535,14 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 	struct bcm4908_enet_dma_ring_slot *slot;
 	struct device *dev = enet->dev;
 	struct bcm4908_enet_dma_ring_bd *buf_desc;
-	int free_buf_descs;
 	u32 tmp;
 
 	/* Free transmitted skbs */
 	if (enet->irq_tx < 0 &&
-	    !(le32_to_cpu(ring->buf_desc[ring->read_idx].ctl) & DMA_CTL_STATUS_OWN))
+	    !(le32_to_cpu(ring->buf_desc[READ_ONCE(ring->read_idx)].ctl) & DMA_CTL_STATUS_OWN))
 		napi_schedule(&enet->tx_ring.napi);
 
-	/* Don't use the last empty buf descriptor */
-	if (ring->read_idx <= ring->write_idx)
-		free_buf_descs = ring->read_idx - ring->write_idx + ring->length;
-	else
-		free_buf_descs = ring->read_idx - ring->write_idx;
-	if (free_buf_descs < 2) {
+	if (unlikely(bcm4908_enet_tx_free_descs(ring) < 1)) {
 		netif_stop_queue(netdev);
 		return NETDEV_TX_BUSY;
 	}
@@ -562,7 +566,7 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 	tmp |= DMA_CTL_STATUS_SOP;
 	tmp |= DMA_CTL_STATUS_EOP;
 	tmp |= DMA_CTL_STATUS_APPEND_CRC;
-	if (ring->write_idx + 1 == ring->length - 1)
+	if (ring->write_idx == ring->length - 1)
 		tmp |= DMA_CTL_STATUS_WRAP;
 
 	netdev_sent_queue(enet->netdev, skb->len);
@@ -572,8 +576,17 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 
 	bcm4908_enet_dma_tx_ring_enable(enet, &enet->tx_ring);
 
-	if (++ring->write_idx == ring->length - 1)
-		ring->write_idx = 0;
+	WRITE_ONCE(ring->write_idx,
+		   ring->write_idx == ring->length - 1 ? 0 : ring->write_idx + 1);
+
+	/* Stop while the ring is full; poll_tx() wakes the queue once there is room */
+	if (unlikely(bcm4908_enet_tx_free_descs(ring) < 1)) {
+		netif_stop_queue(netdev);
+		/* Pairs with smp_mb() in poll_tx(), which may have freed descriptors meanwhile */
+		smp_mb();
+		if (bcm4908_enet_tx_free_descs(ring) >= 1)
+			netif_start_queue(netdev);
+	}
 
 	return NETDEV_TX_OK;
 }
@@ -655,14 +668,15 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 	struct bcm4908_enet_dma_ring_bd *buf_desc;
 	struct bcm4908_enet_dma_ring_slot *slot;
 	struct device *dev = enet->dev;
+	int read_idx = tx_ring->read_idx;
 	unsigned int bytes = 0;
 	int handled = 0;
 
-	while (handled < weight && tx_ring->read_idx != tx_ring->write_idx) {
-		buf_desc = &tx_ring->buf_desc[tx_ring->read_idx];
+	while (handled < weight && read_idx != READ_ONCE(tx_ring->write_idx)) {
+		buf_desc = &tx_ring->buf_desc[read_idx];
 		if (le32_to_cpu(buf_desc->ctl) & DMA_CTL_STATUS_OWN)
 			break;
-		slot = &tx_ring->slots[tx_ring->read_idx];
+		slot = &tx_ring->slots[read_idx];
 
 		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
 		dev_kfree_skb(slot->skb);
@@ -670,9 +684,10 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 		handled++;
 		bytes += slot->len;
 
-		if (++tx_ring->read_idx == tx_ring->length)
-			tx_ring->read_idx = 0;
+		if (++read_idx == tx_ring->length)
+			read_idx = 0;
 	}
+	WRITE_ONCE(tx_ring->read_idx, read_idx);
 
 	netdev_completed_queue(enet->netdev, handled, bytes);
 	enet->netdev->stats.tx_packets += handled;
@@ -683,7 +698,10 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 		bcm4908_enet_dma_ring_intrs_on(enet, tx_ring);
 	}
 
-	if (netif_queue_stopped(enet->netdev))
+	/* Pairs with smp_mb() in start_xmit() after it stopped the queue */
+	smp_mb();
+	if (netif_queue_stopped(enet->netdev) &&
+	    bcm4908_enet_tx_free_descs(tx_ring) >= 1)
 		netif_wake_queue(enet->netdev);
 
 	return handled;
-- 
2.56.0

[PATCH net 2/7] net: bcm4908_enet: drop a TX frame whose DMA mapping failed

[Kordian Chmielewski (mlody1212)]

When dma_map_single() fails, start_xmit() returns NETDEV_TX_BUSY after
it has already stored the skb in the slot. The stack requeues the same
frame and retries it at once, and the skb is left in a slot that never
gets a descriptor. A failed mapping is not something a retry fixes:
free the skb, count it as dropped and return NETDEV_TX_OK. Map the
buffer before the slot is touched.

Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 01a3e1d01..727ad1e96 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -535,6 +535,7 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 	struct bcm4908_enet_dma_ring_slot *slot;
 	struct device *dev = enet->dev;
 	struct bcm4908_enet_dma_ring_bd *buf_desc;
+	dma_addr_t dma_addr;
 	u32 tmp;
 
 	/* Free transmitted skbs */
@@ -554,12 +555,17 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 		return NETDEV_TX_BUSY;
 	}
 
+	dma_addr = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
+	if (unlikely(dma_mapping_error(dev, dma_addr))) {
+		dev_kfree_skb_any(skb);
+		netdev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
+	}
+
 	slot = &ring->slots[ring->write_idx];
 	slot->skb = skb;
 	slot->len = skb->len;
-	slot->dma_addr = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
-	if (unlikely(dma_mapping_error(dev, slot->dma_addr)))
-		return NETDEV_TX_BUSY;
+	slot->dma_addr = dma_addr;
 
 	tmp = skb->len << DMA_CTL_LEN_DESC_BUFLENGTH_SHIFT;
 	tmp |= DMA_CTL_STATUS_OWN;
-- 
2.56.0

[PATCH net 3/7] net: bcm4908_enet: release the TX ring on stop

[Kordian Chmielewski (mlody1212)]

bcm4908_enet_stop() disables the TX DMA but leaves the ring as it is:
the skbs of frames not completed yet stay mapped and are never freed,
and their descriptors keep DMA_CTL_STATUS_OWN. The next open starts
the indexes from 0 again over those descriptors, so the DMA can send
from a buffer that has been freed, and start_xmit() finds OWN set on a
slot it is about to use.

Unmap and free the skbs left in the TX ring on stop, clear slot->skb
once poll_tx() has freed it, and clear the TX descriptors when the ring
is initialised.

Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 23 ++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 727ad1e96..8008b44a4 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -312,6 +312,10 @@ static void bcm4908_enet_dma_ring_init(struct bcm4908_enet *enet,
 	enet_write(enet, ring->st_ram_block + ENET_DMA_CH_STATE_RAM_BASE_DESC_PTR,
 		   (uint32_t)ring->dma_addr);
 
+	/* A descriptor the stopped DMA never completed still carries OWN */
+	if (ring->is_tx)
+		memset(ring->cpu_addr, 0, ring->length * sizeof(*ring->buf_desc));
+
 	ring->read_idx = 0;
 	ring->write_idx = 0;
 }
@@ -495,6 +499,23 @@ static int bcm4908_enet_open(struct net_device *netdev)
 	return 0;
 }
 
+static void bcm4908_enet_tx_ring_clean(struct bcm4908_enet *enet)
+{
+	struct bcm4908_enet_dma_ring *tx_ring = &enet->tx_ring;
+	struct device *dev = enet->dev;
+	int i;
+
+	for (i = 0; i < tx_ring->length; i++) {
+		struct bcm4908_enet_dma_ring_slot *slot = &tx_ring->slots[i];
+
+		if (!slot->skb)
+			continue;
+		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
+		dev_kfree_skb_any(slot->skb);
+		slot->skb = NULL;
+	}
+}
+
 static int bcm4908_enet_stop(struct net_device *netdev)
 {
 	struct bcm4908_enet *enet = netdev_priv(netdev);
@@ -510,6 +531,7 @@ static int bcm4908_enet_stop(struct net_device *netdev)
 	bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
 	bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
 
+	bcm4908_enet_tx_ring_clean(enet);
 	bcm4908_enet_dma_uninit(enet);
 
 	free_irq(enet->irq_tx, enet);
@@ -686,6 +708,7 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 
 		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
 		dev_kfree_skb(slot->skb);
+		slot->skb = NULL;
 
 		handled++;
 		bytes += slot->len;
-- 
2.56.0

[PATCH net 4/7] net: bcm4908_enet: unmap the buffer of a dropped RX frame

[Kordian Chmielewski (mlody1212)]

When poll_rx() drops a frame (too short, or not in a single buffer),
it frees the buffer without unmapping it first, so the memory goes
back to the page allocator while still mapped for the device. Unmap it
as the path for good frames does.

Fixes: 4dc7f09b8bec ("net: broadcom: bcm4908_enet: fix RX path possible mem leak")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 8008b44a4..778920e37 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -653,6 +653,7 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
 
 		if (len < ETH_ZLEN ||
 		    (ctl & (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) != (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) {
+			dma_unmap_single(dev, slot.dma_addr, ENET_RX_BUF_DMA_SIZE, DMA_FROM_DEVICE);
 			skb_free_frag(slot.buf);
 			enet->netdev->stats.rx_dropped++;
 			break;
-- 
2.56.0

[PATCH net 5/7] net: bcm4908_enet: unmap RX buffers on stop with their mapped size

[Kordian Chmielewski (mlody1212)]

Since the switch to build_skb() the RX buffers are mapped with
ENET_RX_BUF_DMA_SIZE and slot->len is no longer set for them, but
bcm4908_enet_dma_uninit() still unmaps them with slot->len. Use the
size they were mapped with.

Fixes: 3a1cc23a75ab ("net: broadcom: bcm4908_enet: use build_skb()")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 778920e37..c0aca6801 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -331,7 +331,7 @@ static void bcm4908_enet_dma_uninit(struct bcm4908_enet *enet)
 		slot = &rx_ring->slots[i];
 		if (!slot->buf)
 			continue;
-		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_FROM_DEVICE);
+		dma_unmap_single(dev, slot->dma_addr, ENET_RX_BUF_DMA_SIZE, DMA_FROM_DEVICE);
 		skb_free_frag(slot->buf);
 		slot->buf = NULL;
 	}
-- 
2.56.0

[PATCH net 6/7] net: bcm4908_enet: publish the buffer address before OWN

[Kordian Chmielewski (mlody1212)]

bcm4908_enet_dma_alloc_rx_buf() hands a descriptor to the DMA by
writing its control word, with DMA_CTL_STATUS_OWN, first and the new
buffer address second. The refill runs on the descriptor that has just
been completed, which is where the DMA waits once the ring is full: if
it picks the descriptor up between the two stores, it writes the next
frame into the old buffer, which poll_rx() is turning into an skb.
start_xmit() writes the address first, but nothing orders the two
stores, and the descriptors live in coherent memory, where arm64 may
make them visible in either order.

Write the address first on both paths and order it before the control
word with dma_wmb(). In poll_rx(), read what the DMA wrote only after
seeing OWN clear (dma_rmb()).

Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index c0aca6801..ea4a9eb98 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -289,8 +289,13 @@ static int bcm4908_enet_dma_alloc_rx_buf(struct bcm4908_enet *enet, unsigned int
 	tmp |= DMA_CTL_STATUS_OWN;
 	if (idx == enet->rx_ring.length - 1)
 		tmp |= DMA_CTL_STATUS_WRAP;
-	buf_desc->ctl = cpu_to_le32(tmp);
+	/* The DMA must not see OWN before the new buffer address: with a full
+	 * ring it waits on this very descriptor and would fill the old buffer,
+	 * which is on its way up the stack as an skb.
+	 */
 	buf_desc->addr = cpu_to_le32(slot->dma_addr);
+	dma_wmb();
+	buf_desc->ctl = cpu_to_le32(tmp);
 
 	return 0;
 }
@@ -600,6 +605,8 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 	netdev_sent_queue(enet->netdev, skb->len);
 
 	buf_desc->addr = cpu_to_le32((uint32_t)slot->dma_addr);
+	/* The DMA must not see OWN before the buffer address */
+	dma_wmb();
 	buf_desc->ctl = cpu_to_le32(tmp);
 
 	bcm4908_enet_dma_tx_ring_enable(enet, &enet->tx_ring);
@@ -638,6 +645,8 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
 		ctl = le32_to_cpu(buf_desc->ctl);
 		if (ctl & DMA_CTL_STATUS_OWN)
 			break;
+		/* Nothing the DMA wrote may be read before it gave the descriptor back */
+		dma_rmb();
 
 		slot = enet->rx_ring.slots[enet->rx_ring.read_idx];
 
-- 
2.56.0

[PATCH net 7/7] net: bcm4908_enet: drop RX frames longer than the buffer

[Kordian Chmielewski (mlody1212)]

poll_rx() takes the frame length from the DMA descriptor and passes it
to skb_put() unchecked, so a length larger than the buffer ends in
skb_over_panic() from the NAPI poll. It was seen on a BCM4908 with the
RX channel's MAX_BURST raised to 32, where the DMA reported a length of
3388 bytes, more than twice the size of the buffer; with the default
burst length it has not been seen. Drop such a frame like any other bad
one and count it as a length error.

Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Kordian Chmielewski (mlody1212) <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index ea4a9eb98..b2debdfa0 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -660,10 +660,13 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
 
 		len = (ctl & DMA_CTL_LEN_DESC_BUFLENGTH) >> DMA_CTL_LEN_DESC_BUFLENGTH_SHIFT;
 
-		if (len < ETH_ZLEN ||
+		/* A length past the buffer would make skb_put() panic the kernel */
+		if (len < ETH_ZLEN || len > ENET_RX_BUF_DMA_SIZE ||
 		    (ctl & (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) != (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) {
 			dma_unmap_single(dev, slot.dma_addr, ENET_RX_BUF_DMA_SIZE, DMA_FROM_DEVICE);
 			skb_free_frag(slot.buf);
+			if (len > ENET_RX_BUF_DMA_SIZE)
+				enet->netdev->stats.rx_length_errors++;
 			enet->netdev->stats.rx_dropped++;
 			break;
 		}
-- 
2.56.0

Re: [PATCH net 0/7] net: bcm4908_enet: TX ring and DMA fixes

[Kordian Chmielewski (mlody1212)]

A correction to the cover letter and to two changelogs: I mixed up what
I had seen fail on the hardware with what I had found reading the code.

Only patches 1 and 7 come from failures on the hardware. Patch 1 fixes
the TX stall under load (seen twice, none in 8 runs with the fix).
Patches 2 to 6 come from reading the code and have no reproducer.

Patch 7: the skb_over_panic was seen only after I raised the RX
channel's MAX_BURST to 32 by hand on a running channel. I tried that
again today on my 6.18 tree with the patch: within seconds one frame
was longer than the buffer, and it was dropped and counted in
rx_length_errors instead of a panic, but the DMA then stalled and the
board needed a reboot. With the driver's own value, 8, there were none
in about 18 million frames. So patch 7 is hardening and belongs in
net-next rather than net.

Two changelogs say too much. In patch 3, the TX skbs left on stop are
leaked, not freed: the stale descriptors could send buffers nobody
freed. In patch 6, I have no test of what the DMA does at a descriptor
being refilled. With the patch, a 16-entry RX ring under more traffic
than the CPU can take gave no RX or checksum errors in about 14 million
frames, but I have not run that without the patch. Also, the panic in
patch 7 passed 3388 to skb_put(), so the descriptor said 3392 with the
FCS.

In my earlier plan mail I wrote that RX buffers were not freed on stop
and that RX frames longer than the buffer were passed up. Both were
wrong: the RX buffers are freed, only unmapped with a size of 0, and
such frames end in skb_over_panic, seen only with the raised burst.

I will send a v2 with these fixed.

Kordian

Re: [PATCH net 0/7] net: bcm4908_enet: TX ring and DMA fixes

[Jakub Kicinski]

On Thu, 12 Nov 2026 14:23:08 +0100 Kordian Chmielewski (mlody1212)
wrote:
> I will send a v2 with these fixed.

The AI review just got posted, please make sure you look thru it before
sending v2.

Re: [PATCH net 1/7] net: bcm4908_enet: fix the TX ring wrap

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · 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 early ring-full path at the top of
  bcm4908_enet_start_xmit() (`if…
- [Low] The commit message leaves out a second symptom of the same index
  mismatch, which this patch also fixes.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch's
  new stop/wake logic still depends on it working.

--

net: bcm4908_enet: fix the TX ring wrap

The TX ring now uses all descriptors, with WRAP on the last one. Both
read_idx and write_idx wrap at length, and one descriptor is always left
free, so read_idx == write_idx can only mean an empty ring.
bcm4908_enet_start_xmit() now stops the queue once the ring is full.
bcm4908_enet_poll_tx() wakes it again, with smp_mb() on both sides.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index fd2715f84af97..01a3e1d0192f8 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c

[ ... ]

> @@ -525,20 +535,14 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic

[ ... ]

>  	/* Free transmitted skbs */
>  	if (enet->irq_tx < 0 &&
> -	    !(le32_to_cpu(ring->buf_desc[ring->read_idx].ctl) & DMA_CTL_STATUS_OWN))
> +	    !(le32_to_cpu(ring->buf_desc[READ_ONCE(ring->read_idx)].ctl) & DMA_CTL_STATUS_OWN))
>  		napi_schedule(&enet->tx_ring.napi);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
does the new stop/wake logic work when the optional TX interrupt is
missing?

In Documentation/devicetree/bindings/net/brcm,bcm4908-enet.yaml, both
interrupts and interrupt-names have minItems: 1. A DT node that lists
only "rx" is therefore valid, and probe carries on with
enet->irq_tx < 0.

In that case bcm4908_enet_open() never enables the TX NAPI:

	if (enet->irq_tx > 0) {
		napi_enable(&tx_ring->napi);

The TX NAPI still has NAPI_STATE_SCHED set from netif_napi_add, so
napi_schedule() here always fails in napi_schedule_prep(), and
bcm4908_enet_poll_tx() never runs.

Nothing then advances read_idx, unmaps or frees the TX skbs, or wakes
the queue. After length - 1 frames, the new stop-on-full branch at the
end of bcm4908_enet_start_xmit() stops the queue. Nothing wakes it
again, although the new comment there says "poll_tx() wakes the queue
once there is room".

On ifdown, bcm4908_enet_stop() calls napi_disable(&tx_ring->napi) on
the NAPI that was never enabled. Wouldn't napi_disable_locked() then
spin here forever while holding RTNL and the netdev instance lock?

		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}

bcm4908_enet_stop() also calls free_irq(enet->irq_tx, enet) with a
negative IRQ.

This goes back to 12bb508bfe5a ("net: broadcom: bcm4908_enet: support
TX interrupt"). The in-tree bcm4908.dtsi provides both "rx" and "tx",
so only DTs that use the binding's optional-TX form are affected.

>  
> -	/* Don't use the last empty buf descriptor */
> -	if (ring->read_idx <= ring->write_idx)
> -		free_buf_descs = ring->read_idx - ring->write_idx + ring->length;
> -	else
> -		free_buf_descs = ring->read_idx - ring->write_idx;
> -	if (free_buf_descs < 2) {
> +	if (unlikely(bcm4908_enet_tx_free_descs(ring) < 1)) {
>  		netif_stop_queue(netdev);
>  		return NETDEV_TX_BUSY;
>  	}

[Severity: Medium]
Can this early ring-full path in bcm4908_enet_start_xmit() lose a
wakeup?

It stops the queue without the smp_mb() and re-check that the patch
adds at the end of bcm4908_enet_start_xmit(). include/net/netdev_queues.h
says the standard stop/wake pairing does not prevent false wake ups
when the ring is full, and that drivers should check for ring full at
the start of the xmit handler. So this path can be reached.

Here is one way to get a false wake:

  - bcm4908_enet_poll_tx() finds netif_queue_stopped() true and free
    descriptors >= 1.
  - Meanwhile the stop/smp_mb/re-check at the end of start_xmit
    restarts the queue.
  - poll_tx is delayed, for example by a hardirq, before it calls
    netif_wake_queue().
  - start_xmit fills the last free descriptor and stops the queue
    again.
  - poll_tx's late netif_wake_queue() then clears XOFF on a full ring.

There is no read barrier in poll_tx between the XOFF test and the
write_idx load, so load-load reordering is another way to get there.

On the next xmit, the early check reads read_idx and sees a full ring.
Suppose the poll_tx() runs that reap the remaining descriptors finish
inside the window before netif_stop_queue() here:

CPU0 (start_xmit)                    CPU1 (poll_tx)
bcm4908_enet_tx_free_descs() == 0
                                     reap all outstanding descriptors
                                     WRITE_ONCE(tx_ring->read_idx, ...)
                                     smp_mb()
                                     netif_queue_stopped() == false
netif_stop_queue()
return NETDEV_TX_BUSY

No descriptors are outstanding at that point, so no TX interrupt or
NAPI poll follows, and the driver has no ndo_tx_timeout. Wouldn't the
queue then stay stopped until ifdown/ifup? That is the "queue stops for
good with nothing left to wake it" symptom the commit message describes.

The window is narrow. It needs the xmit CPU to be preempted, as on
PREEMPT_RT, or held by a long hardirq.

Could this path use netif_txq_try_stop() instead, or the same
stop/smp_mb/re-check/restart sequence used at the end of
bcm4908_enet_start_xmit()?

[ ... ]

> @@ -572,8 +576,17 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
>  
>  	bcm4908_enet_dma_tx_ring_enable(enet, &enet->tx_ring);
>  
> -	if (++ring->write_idx == ring->length - 1)
> -		ring->write_idx = 0;
> +	WRITE_ONCE(ring->write_idx,
> +		   ring->write_idx == ring->length - 1 ? 0 : ring->write_idx + 1);

[Severity: Low]
This isn't a bug, but the commit message leaves out a second symptom of
the same index mismatch, and this change fixes it too.

With the old wrap at length - 1, slot length - 1 was never filled.
bcm4908_enet_poll_tx() wrapped read_idx at length. After it reaped
slot length - 2, read_idx became length - 1, which write_idx could
never equal.

The loop then ran on the unused slot, whose descriptor ctl is still
zero, so OWN is clear. Once per ring lap it called dma_unmap_single()
with slot->dma_addr 0 and slot->len 0, then dev_kfree_skb(NULL) and
handled++. This over-counted tx_packets and caused CONFIG_DMA_API_DEBUG
warnings about unmapping memory that was never mapped.

The changelog only says:

    while poll_tx() wraps read_idx at length,

Could it also name this symptom, so stable backporters can see that it
is covered?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235700.2586821-1-kudzo51%40gmail.com

Re: [PATCH net 3/7] net: bcm4908_enet: release the TX ring on stop

[netdev-bot+sashiko]

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

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

- [High] In bcm4908_enet_stop()
  (drivers/net/ethernet/broadcom/bcm4908_enet.c), the new…
- [Medium] bcm4908_enet_poll_tx() now writes slot->skb = NULL (line 711)
  and then publishes the freed slots with a plain…
- [Low] The commit message says that without the patch, the next open
  means 'the DMA can send from a buffer that has been freed'.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the open and
  stop paths disagree about the optional TX interrupt, and that keeps…

--

net: bcm4908_enet: release the TX ring on stop

This patch unmaps and frees the skbs still left in the TX ring when
bcm4908_enet_stop() runs, and clears slot->skb in bcm4908_enet_poll_tx()
after the skb is freed. It also zeroes the TX descriptors in
bcm4908_enet_dma_ring_init(), so stale DMA_CTL_STATUS_OWN bits are not
carried into the next open.

> bcm4908_enet_stop() disables the TX DMA but leaves the ring as it is:
> the skbs of frames not completed yet stay mapped and are never freed,
> and their descriptors keep DMA_CTL_STATUS_OWN. The next open starts
> the indexes from 0 again over those descriptors, so the DMA can send
> from a buffer that has been freed, and start_xmit() finds OWN set on a
> slot it is about to use.

[Severity: Low]
Does "the DMA can send from a buffer that has been freed" describe the
code before this patch?

bcm4908_enet_poll_tx() is the only place TX skbs are freed. It frees a
slot only after the hardware has cleared OWN:

	if (le32_to_cpu(buf_desc->ctl) & DMA_CTL_STATUS_OWN)
		break;

bcm4908_enet_start_xmit() also will not reuse a slot that still has OWN
set:

	if (unlikely(le32_to_cpu(buf_desc->ctl) & DMA_CTL_STATUS_OWN)) {
		netif_stop_queue(netdev);
		return NETDEV_TX_BUSY;
	}

The descriptor ring and slots are allocated at probe time and kept
across stop/open. An skb behind a stale OWN descriptor was therefore
leaked and left mapped, not freed.

Before this patch the effects seem to be these:

- skbs, socket wmem charges and DMA mappings leak.
- Stale frames from the previous session go out after reopen.
- TX can stall.

The stall happens when slot 0 still has OWN after reopen. start_xmit()
then stops the queue before it calls bcm4908_enet_dma_tx_ring_enable().
No TX completion fires after that, so poll_tx() never wakes the queue,
and the driver has no ndo_tx_timeout.

Could the commit message list these effects instead of a use-after-free?
That would help stable/backport triage.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index 727ad1e96db35..8008b44a4635f 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c

[ ... ]

> @@ -510,6 +531,7 @@ static int bcm4908_enet_stop(struct net_device *netdev)
>  	bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
>  	bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
>  
> +	bcm4908_enet_tx_ring_clean(enet);

[Severity: High]
Can the TX DMA still be reading these buffers when
bcm4908_enet_tx_ring_clean() unmaps and frees them?

bcm4908_enet_dma_tx_ring_disable() is a single posted write. It does
not wait for the channel to stop:

	enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, 0);

The RX helper, bcm4908_enet_dma_rx_ring_disable(), polls for up to 2 ms
until the hardware drops ENABLE:

	tmp = enet_read(enet, ring->cfg_block + ENET_DMA_CH_CFG);
	if (!(tmp & ENET_DMA_CH_CFG_ENABLE))
		return;

That suggests the channel stops asynchronously. bcm63xx_enet drives the
same DMA block. Its bcm_enet_disable_dma() polls
ENETDMAC_CHANCFG_EN_MASK for both TX and RX before
bcm_enet_tx_reclaim() runs. ENET_DMA_CTRL_CFG_MASTER_EN and UMAC
CMD_TX_EN are also still set at this point.

Suppose the interface goes down while frames are in flight. The DMA
could still be fetching the current descriptor's buffer here:

bcm4908_enet_stop()
  bcm4908_enet_dma_tx_ring_disable()   <- CFG = 0, no wait
  bcm4908_enet_tx_ring_clean()
    dma_unmap_single()
    dev_kfree_skb_any()

Before this patch those skbs leaked but stayed mapped, so a late DMA
read still hit valid memory. With this change the DMA could read freed
and possibly reallocated memory. That data could go out on the wire, or
the read could fault on IOMMU systems.

bcm4908_enet_dma_ring_init() clears the TX descriptors under the
comment "A descriptor the stopped DMA never completed still carries
OWN". That memset only runs at the next open. Until then, the OWN
descriptors that point at the freed buffers stay in the ring.

Could the TX disable path poll for ENET_DMA_CH_CFG_ENABLE to clear, as
the RX path does, before the buffers are freed?

>  	bcm4908_enet_dma_uninit(enet);
>  
>  	free_irq(enet->irq_tx, enet);

[Severity: High]
This isn't a bug introduced by this patch, but if the TX interrupt is
missing, does bcm4908_enet_stop() ever reach the new
bcm4908_enet_tx_ring_clean()?

The binding (brcm,bcm4908-enet.yaml) allows only the "rx" interrupt
(minItems: 1). bcm4908_enet_probe() accepts a negative value here:

	enet->irq_tx = platform_get_irq_byname(pdev, "tx");

bcm4908_enet_open() enables the TX NAPI only when that IRQ exists:

	if (enet->irq_tx > 0) {
		napi_enable(&tx_ring->napi);

bcm4908_enet_stop() still calls napi_disable(&tx_ring->napi)
unconditionally. netif_napi_add_tx() leaves the NAPI with SCHED and
NPSVC set. napi_disable_locked() would then loop in usleep_range()
forever, holding RTNL and the netdev instance lock.

In the same setup, this fallback in bcm4908_enet_start_xmit() always
fails in napi_schedule_prep(), because SCHED is already set:

	napi_schedule(&enet->tx_ring.napi);

So poll_tx() never runs, and TX stops once the ring fills.

The free_irq(enet->irq_tx, enet) here is also unconditional. open() only
requests that IRQ when irq_tx > 0.

[ ... ]

> @@ -686,6 +708,7 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
>  
>  		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
>  		dev_kfree_skb(slot->skb);
> +		slot->skb = NULL;

[Severity: Medium]
Can this NULL store race with bcm4908_enet_start_xmit() reusing the slot
on another CPU?

Later in bcm4908_enet_poll_tx(), the freed slots are published with a
plain store:

	WRITE_ONCE(tx_ring->read_idx, read_idx);

Nothing orders the slot->skb = NULL stores before it. The smp_mb() in
poll_tx() comes after it.

start_xmit() reads read_idx without a lock, through
bcm4908_enet_tx_free_descs(), and then writes:

	slot->skb = skb;

On arm64 the late NULL store from poll_tx() could land after that
write. One possible sequence:

- start_xmit() fills the ring and stops the queue.
- It rechecks after smp_mb() and sees that poll_tx() freed two or more
  slots.
- It restarts the queue and fills old_read-1, then old_read.

That slot would then hold skb == NULL while its descriptor belongs to
the new frame. The next poll_tx() unmaps it and calls
dev_kfree_skb(NULL), so the skb and its socket sk_wmem_alloc charge
leak. bcm4908_enet_tx_ring_clean() would skip that slot as well.

The window looks very narrow in practice, since poll_tx() runs an
smp_mb() shortly after the WRITE_ONCE(). Still, before this patch only
start_xmit() wrote slot->skb.

Would smp_store_release(&tx_ring->read_idx, read_idx), or an smp_wmb()
before the WRITE_ONCE(), be enough? Another option is for
bcm4908_enet_tx_ring_clean() to walk read_idx..write_idx, so poll_tx()
would not need this store at all.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235700.2586821-1-kudzo51%40gmail.com

Re: [PATCH net 6/7] net: bcm4908_enet: publish the buffer address before OWN

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The comment this patch adds in bcm4908_enet_dma_alloc_rx_buf()
  (drivers/net/ethernet/broadcom/bcm4908_enet.c, lines 292-295) says the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is in the
  function the patch edits and is still present at the end of the series.

--

net: bcm4908_enet: publish the buffer address before OWN

This patch changes the RX refill in bcm4908_enet_dma_alloc_rx_buf() so the
buffer address is written before the control word that carries
DMA_CTL_STATUS_OWN. It adds a dma_wmb() between the two stores on both the
RX refill path and the TX path. It also adds a dma_rmb() in
bcm4908_enet_poll_rx() after DMA_CTL_STATUS_OWN is seen clear.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index c0aca68010c3b..ea4a9eb98097c 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> @@ -289,8 +289,13 @@ static int bcm4908_enet_dma_alloc_rx_buf(struct bcm4908_enet *enet, unsigned int
>  	tmp |= DMA_CTL_STATUS_OWN;
>  	if (idx == enet->rx_ring.length - 1)
>  		tmp |= DMA_CTL_STATUS_WRAP;
> -	buf_desc->ctl = cpu_to_le32(tmp);
> +	/* The DMA must not see OWN before the new buffer address: with a full
> +	 * ring it waits on this very descriptor and would fill the old buffer,
> +	 * which is on its way up the stack as an skb.
> +	 */

[Severity: Low]
This isn't a bug, but does this comment match what the rest of the driver
assumes about a full ring? The commit message makes the same claim:

  "which is where the DMA waits once the ring is full"

The end of bcm4908_enet_poll_rx() assumes the opposite for the same case:

	/* Hardware could disable ring if it run out of descriptors */
	bcm4908_enet_dma_rx_ring_enable(enet, &enet->rx_ring);

If the channel is disabled when it runs out of descriptors, it only
restarts through the writel() in bcm4908_enet_dma_rx_ring_enable(). That
write comes after the descriptor stores, so the window described here
would not open.

The new addr -> dma_wmb() -> ctl ordering looks correct under either
model. Could the new comment and the commit message be brought in line
with the existing comment, or hedged, so a reader can tell which hardware
behaviour applies?

>  	buf_desc->addr = cpu_to_le32(slot->dma_addr);
> +	dma_wmb();
> +	buf_desc->ctl = cpu_to_le32(tmp);
>  
>  	return 0;
>  }

[ ... ]

> @@ -638,6 +645,8 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
>  		ctl = le32_to_cpu(buf_desc->ctl);
>  		if (ctl & DMA_CTL_STATUS_OWN)
>  			break;
> +		/* Nothing the DMA wrote may be read before it gave the descriptor back */
> +		dma_rmb();
>  
>  		slot = enet->rx_ring.slots[enet->rx_ring.read_idx];
>  

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but it is
in the refill path this patch touches. What happens to the old RX buffer
when bcm4908_enet_dma_alloc_rx_buf() fails?

The refill writes into the live slot before it knows the refill will
succeed:

bcm4908_enet_dma_alloc_rx_buf() {
	slot->buf = napi_alloc_frag(ENET_RX_SKB_BUF_ALLOC_SIZE);
	if (!slot->buf)
		return -ENOMEM;
	...
	if (err) {
		...
		skb_free_frag(slot->buf);
		slot->buf = NULL;
		return err;
	}
	...
}

bcm4908_enet_poll_rx() keeps the old buffer only in its local copy:

	slot = enet->rx_ring.slots[enet->rx_ring.read_idx];

	/* Provide new buffer before unpinning the old one */
	err = bcm4908_enet_dma_alloc_rx_buf(enet, enet->rx_ring.read_idx);
	if (err)
		break;

After that break, slots[read_idx].buf is NULL. Its dma_addr holds either
the old mapping or the mapping error cookie. The filled, still-mapped old
buffer is leaked, and read_idx does not move. The descriptor still has
DMA_CTL_STATUS_OWN clear, and ctl still holds the old length and
SOP/EOP bits.

The next poll stops on the same index, now with slot.buf == NULL. The
refill succeeds this time, and the length and SOP/EOP check passes on the
stale ctl, so we get:

bcm4908_enet_poll_rx()
  dma_unmap_single(dev, slot.dma_addr, ...)   <- stale or error dma_addr
  build_skb(NULL, ENET_RX_SKB_BUF_ALLOC_SIZE)
    __build_skb_around()
      __finalize_skb_around()                 <- writes shinfo at NULL + size

The bad-frame path calls skb_free_frag(slot.buf) on NULL instead. Could
this oops in NAPI softirq context when GFP_ATOMIC allocations fail or DMA
mappings run out?

bcm4908_enet_dma_uninit() also skips slots with buf == NULL:

	if (!slot->buf)
		continue;

so the leaked frag and its mapping are never freed. The code looks the
same at the end of the series.

Would it work to allocate and map into local variables, and only write
them to the slot and the descriptor once both steps succeed? On failure,
poll_rx() could give the old buffer back to the hardware, or drop the
frame and keep the slot intact.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235700.2586821-1-kudzo51%40gmail.com

Re: [PATCH net 0/7] net: bcm4908_enet: TX ring and DMA fixes

[Jakub Kicinski]

On Tue, 10 Nov 2026 00:56:53 +0100 Kordian Chmielewski (mlody1212)
wrote:
> Kordian Chmielewski (mlody1212)

Per kernel process docs the stuff in the round brackets is supposed to
be a sponsor of your work or your employer, please drop the nick name.

Re: [PATCH net 7/7] net: bcm4908_enet: drop RX frames longer than the buffer

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new check in bcm4908_enet_poll_rx()
  (drivers/net/ethernet/broadcom/bcm4908_enet.c:664-671) treats a…
- [Medium] The new branch in bcm4908_enet_poll_rx()
  (bcm4908_enet.c:668-670) increments netdev->stats.rx_length_errors and…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the RX refill
  failure path in bcm4908_enet_poll_rx() loses the old buffer and later…

--

net: bcm4908_enet: drop RX frames longer than the buffer

This patch adds a check to bcm4908_enet_poll_rx() so that RX descriptors
reporting a length larger than ENET_RX_BUF_DMA_SIZE are dropped instead
of being passed to skb_put(). Such frames are counted in rx_length_errors
and rx_dropped. The commit message says this was seen only with the RX
channel MAX_BURST raised to 32.

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index ea4a9eb98097c..b2debdfa0902e 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> @@ -660,10 +660,13 @@ static int bcm4908_enet_poll_rx(struct napi_struct *napi, int weight)
>  
>  		len = (ctl & DMA_CTL_LEN_DESC_BUFLENGTH) >> DMA_CTL_LEN_DESC_BUFLENGTH_SHIFT;
>  
> -		if (len < ETH_ZLEN ||
> +		/* A length past the buffer would make skb_put() panic the kernel */
> +		if (len < ETH_ZLEN || len > ENET_RX_BUF_DMA_SIZE ||

[Severity: Medium]
On a mainline kernel, can the device ever report a length like this?

bcm4908_enet_dma_alloc_rx_buf() maps exactly ENET_RX_BUF_DMA_SIZE (1532)
bytes and puts that size in the descriptor:

    tmp = ENET_RX_BUF_DMA_SIZE << DMA_CTL_LEN_DESC_BUFLENGTH_SHIFT;

bcm4908_enet_set_mtu() programs mtu + ENET_MAX_ETH_OVERHEAD, which is
also 1532 at ENET_MTU_MAX:

    enet_umac_write(enet, UMAC_MAX_FRAME_LEN, mtu + ENET_MAX_ETH_OVERHEAD);

bcm4908_enet_dma_ring_init() only ever writes ENET_DMA_MAX_BURST_LEN (8)
to ENET_DMA_CH_CFG_MAX_BURST.

The commit message says:

    It was seen on a BCM4908 with the RX channel's MAX_BURST raised to
    32, where the DMA reported a length of 3388 bytes, more than twice
    the size of the buffer; with the default burst length it has not
    been seen.

Given that, does the Fixes: tag (and the stable backport it implies) fit
here? The triggering configuration is not one mainline uses.

There is also the question of what the hardware actually did. If the
DMA wrote the 3388 bytes it reported into a 1532-byte mapping, then the
skb_shared_info area of the frag, and part of the next napi_alloc_frag()
allocation, were overwritten before the CPU saw the descriptor.

Before this patch, skb_over_panic() stopped the system at that point.
With this patch the frame is dropped silently. There is no WARN or log
message, the channel is not reset, and bcm4908_enet_dma_rx_ring_enable()
re-enables the ring with the same configuration.

Did the hardware really overrun the buffer, or did it only misreport the
length? If an overrun is possible, would a WARN_ONCE() or a ratelimited
netdev_err() be better than a silent drop?

>  		    (ctl & (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) != (DMA_CTL_STATUS_SOP | DMA_CTL_STATUS_EOP)) {
>  			dma_unmap_single(dev, slot.dma_addr, ENET_RX_BUF_DMA_SIZE, DMA_FROM_DEVICE);
>  			skb_free_frag(slot.buf);

[Severity: High]
This isn't a bug introduced by this patch, but slot.buf can be NULL
when this path runs, after an earlier RX refill failure.

bcm4908_enet_poll_rx() copies the ring slot by value and then refills
the same index:

    slot = enet->rx_ring.slots[enet->rx_ring.read_idx];

    /* Provide new buffer before unpinning the old one */
    err = bcm4908_enet_dma_alloc_rx_buf(enet, enet->rx_ring.read_idx);
    if (err)
        break;

bcm4908_enet_dma_alloc_rx_buf() writes straight into the ring slot
before it knows whether the allocation worked:

    slot->buf = napi_alloc_frag(ENET_RX_SKB_BUF_ALLOC_SIZE);
    if (!slot->buf)
        return -ENOMEM;

If napi_alloc_frag() fails, the ring slot now holds buf == NULL. The
only reference to the old, still-mapped frag was in the local copy,
which is discarded on the break.

On a dma_mapping_error(), slot->dma_addr is overwritten with the error
cookie and slot->buf is set to NULL, so the old streaming mapping is
leaked as well.

read_idx is not advanced, and the descriptor still has OWN clear. The
next poll therefore handles the same index with slot.buf == NULL. Once a
refill succeeds:

    bcm4908_enet_poll_rx()
      build_skb(NULL, ENET_RX_SKB_BUF_ALLOC_SIZE)
        __build_skb_around()
          __finalize_skb_around()
            memset(shinfo, ...)   <- NULL + size

Does this oops in NAPI softirq context? On the bad-frame path, including
the new len > ENET_RX_BUF_DMA_SIZE branch, the code calls
skb_free_frag(NULL) instead, and after a mapping error it calls
dma_unmap_single() on the error cookie.

bcm4908_enet_dma_uninit() skips slots with buf == NULL, so the leaked
frag and its mapping are never released at teardown either.

Could bcm4908_enet_dma_alloc_rx_buf() allocate and map into local
variables, and update the slot only on success?

> +			if (len > ENET_RX_BUF_DMA_SIZE)
> +				enet->netdev->stats.rx_length_errors++;
>  			enet->netdev->stats.rx_dropped++;

[Severity: Medium]
Should rx_errors be incremented here as well?

The struct rtnl_link_stats64 kernel-doc in include/uapi/linux/if_link.h
says:

    @rx_errors: Total number of bad packets received on this network
    device. This counter must include events counted by
    @rx_length_errors, ...

This driver never touches rx_errors, and bcm4908_enet_netdev_ops has no
ndo_get_stats64. Userspace therefore sees the raw netdev->stats values.
After one oversized frame, ip -s link would show a non-zero frame count
with zero errors.

The same frame is also counted in rx_dropped, which the uAPI describes
as packets received but not processed, for example because of a lack of
resources.

For comparison, bcmsysport.c and bgmac.c increment rx_length_errors
together with rx_errors, and do not also count the frame in rx_dropped.

>  			break;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235700.2586821-1-kudzo51%40gmail.com

Re: [PATCH net 0/7] net: bcm4908_enet: TX ring and DMA fixes

[Kordian Chmielewski]

On Thu, 12 Nov 2026 09:13:23 -0800 Jakub Kicinski wrote:
> The AI review just got posted, please make sure you look thru it before
> sending v2.

Thanks, I went through it. The points on patches 1, 3 and 6 are right,
and v2 will fix them: the early ring-full stop in start_xmit() in
patch 1, the TX DMA that is not waited on before its buffers are freed
and the slot->skb race in patch 3, and the comment in patch 6.

I'll drop patch 7 rather than move it to net-next: its trigger is not
reachable with the driver's own settings, and dropping the frame did
not keep the DMA running anyway.

The review also found two older problems, a missing TX interrupt and a
failed RX refill. I'll send fixes for them separately, after this
series.

And I'll drop the nickname, as you asked.

I'll send v2 later today or tomorrow, once I've tested the changes on
the hardware.

Kordian