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

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

[Kordian Chmielewski]

These fix the BCM4908 Ethernet driver. Patch 1 fixes a TX stall seen
on a TP-Link Archer AX6000 under sustained load. Patches 2 to 6 come
from reading the code and have no reproducer.

Tested on an Archer AX6000 with OpenWrt and Linux 6.18.55, whose driver
differs from net only in one tree-wide change, with the LAN going
through this driver: TX and RX at line rate (941 Mbit/s TCP), and
ifdown/ifup of the conduit while it was sending. I ran the same again,
plus UDP at 1 Gbit/s, with the TX ring shrunk to 16 descriptors for the
test, so that the ring was full much of the time and the queue was
stopped and woken over and over (about a million qdisc requeues in a
minute). No stall, no errors in the counters, no DMA timeout warnings.

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

Changes in v2:
- patch 1: stop and wake the queue with the netdev_queues.h helpers,
  also when start_xmit() finds the ring full on entry; the changelog
  also names the slot that poll_tx() completed without a frame
- patch 3: wait for the TX DMA channel to stop before freeing its
  buffers; free only between read_idx and write_idx instead of
  clearing slot->skb in poll_tx(); drop the OWN check that could stop
  the queue for good; fix the RX timeout warning; the changelog
  describes leaks and stale frames, not a use-after-free
- patch 6: the comment and changelog no longer say where the DMA waits
- patches 1 and 2: more exact changelogs (the measured rate and what
  stopped; what driver.rst says about NETDEV_TX_BUSY)
- drop patch 7: its trigger is not reachable with the driver's own
  settings
- wrap lines over 80 columns; author name without the nickname

v1: https://lore.kernel.org/netdev/20261109235700.2586821-1-kudzo51@gmail.com/

Kordian Chmielewski (6):
  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

 drivers/net/ethernet/broadcom/bcm4908_enet.c | 124 ++++++++++++++-----
 1 file changed, 91 insertions(+), 33 deletions(-)


base-commit: 86706ffa990ecd46a4326f8eecbccbbd60a6cb8d
-- 
2.56.0

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

[Kordian Chmielewski]

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 everything the CPU sent to the
switch under sustained TX load (about 1.3 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.

The same mismatch also lets poll_tx() step onto slot length - 1, which
start_xmit() never fills: once per lap it completes that slot,
unmapping address 0 with length 0 and counting a packet that was never
sent. That one is from reading the code.

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 and wake the queue with the
netdev_queues.h helpers: start_xmit() stops it when the ring is full,
also when it finds the ring full on entry, and poll_tx() wakes it once
there is room again.

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

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index fd2715f84..60f711e8c 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/string.h>
+#include <net/netdev_queues.h>
 
 #include "bcm4908_enet.h"
 #include "unimac.h"
@@ -518,30 +519,34 @@ 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 netdev_queue *txq = netdev_get_tx_queue(netdev, 0);
 	struct bcm4908_enet *enet = netdev_priv(netdev);
 	struct bcm4908_enet_dma_ring *ring = &enet->tx_ring;
 	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) {
-		netif_stop_queue(netdev);
+	if (!netif_txq_maybe_stop(txq, bcm4908_enet_tx_free_descs(ring), 1, 1))
 		return NETDEV_TX_BUSY;
-	}
 
 	/* Hardware removes OWN bit after sending data */
 	buf_desc = &ring->buf_desc[ring->write_idx];
@@ -562,7 +567,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 +577,13 @@ 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;
+	if (ring->write_idx == ring->length - 1)
+		WRITE_ONCE(ring->write_idx, 0);
+	else
+		WRITE_ONCE(ring->write_idx, ring->write_idx + 1);
+
+	/* poll_tx() wakes the queue once there is room again */
+	netif_txq_maybe_stop(txq, bcm4908_enet_tx_free_descs(ring), 1, 1);
 
 	return NETDEV_TX_OK;
 }
@@ -652,17 +662,19 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 {
 	struct bcm4908_enet_dma_ring *tx_ring = container_of(napi, struct bcm4908_enet_dma_ring, napi);
 	struct bcm4908_enet *enet = container_of(tx_ring, struct bcm4908_enet, tx_ring);
+	struct netdev_queue *txq = netdev_get_tx_queue(enet->netdev, 0);
 	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,22 +682,23 @@ 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;
 	enet->netdev->stats.tx_bytes += bytes;
 
+	/* Reports to BQL, and wakes the queue if start_xmit() stopped it */
+	netif_txq_completed_wake(txq, handled, bytes,
+				 bcm4908_enet_tx_free_descs(tx_ring), 1);
+
 	if (handled < weight) {
 		napi_complete_done(napi, handled);
 		bcm4908_enet_dma_ring_intrs_on(enet, tx_ring);
 	}
 
-	if (netif_queue_stopped(enet->netdev))
-		netif_wake_queue(enet->netdev);
-
 	return handled;
 }
 
-- 
2.56.0

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

[Florian Fainelli]

On 11/12/26 11:50, Kordian Chmielewski wrote:
> 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 everything the CPU sent to the
> switch under sustained TX load (about 1.3 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.
> 
> The same mismatch also lets poll_tx() step onto slot length - 1, which
> start_xmit() never fills: once per lap it completes that slot,
> unmapping address 0 with length 0 and counting a packet that was never
> sent. That one is from reading the code.
> 
> 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 and wake the queue with the
> netdev_queues.h helpers: start_xmit() stops it when the ring is full,
> also when it finds the ring full on entry, and poll_tx() wakes it once
> there is room again.
> 
> Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
> Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 1 · 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] net: bcm4908_enet: missing barriers and late write_idx update
  on TX ring

Pre-existing issues:
- [High] When the device tree has no "tx" interrupt, TX completions never
  run, so the queue stop/wake design this patch adds cannot work.
- [High] bcm4908_enet_poll_tx() reads the producer index without a lock
  or ordering, then reads descriptor and slot contents with no read…

--

net: bcm4908_enet: fix the TX ring wrap

This patch makes start_xmit() and poll_tx() wrap the TX ring indexes at
the same point. It sets WRAP on the last descriptor and keeps one
descriptor free, so read_idx == write_idx always means an empty ring. It
also switches the queue stop/wake logic to the netif_txq_* helpers from
netdev_queues.h.

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

[ ... ]

> @@ -518,30 +519,34 @@ static int bcm4908_enet_stop(struct net_device *netdev)

[ ... ]

>  	/* 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);

[ ... ]

> @@ -572,8 +577,13 @@ 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;
> +	if (ring->write_idx == ring->length - 1)
> +		WRITE_ONCE(ring->write_idx, 0);
> +	else
> +		WRITE_ONCE(ring->write_idx, ring->write_idx + 1);
> +
> +	/* poll_tx() wakes the queue once there is room again */
> +	netif_txq_maybe_stop(txq, bcm4908_enet_tx_free_descs(ring), 1, 1);

[Severity: Medium]
This isn't introduced by this patch either, but separately from the
missing read barrier on write_idx in bcm4908_enet_poll_tx(), are there
two more ordering problems in this lockless ring?

bcm4908_enet_start_xmit() hands the descriptor to the DMA by setting
OWN in buf_desc->ctl, and kicks the channel with
bcm4908_enet_dma_tx_ring_enable(), before it publishes the new
ring->write_idx here. Suppose the frame completes and the TX interrupt
is taken on another CPU inside that window. bcm4908_enet_irq_handler()
masks and acks the TX interrupt and schedules the TX NAPI.
bcm4908_enet_poll_tx() then still sees read_idx == write_idx, handles
nothing, calls napi_complete_done() and unmasks the interrupt again.
The status for that frame has already been acked, so nothing brings
poll_tx() back for it.

If that frame is the one that made netdev_sent_queue() hit the BQL
limit, or made netif_txq_maybe_stop() stop the queue, nothing is left
to wake it and TX stalls much like the bug this patch fixes. If not,
the skb and its BQL bytes still sit there until some later frame's
completion. The window is small, but a hard interrupt on the
transmitting CPU between the enable and the WRITE_ONCE() is enough to
widen it.

Simply publishing write_idx before the enable doesn't look sufficient
if the channel is still running from an earlier frame and picks up
the descriptor as soon as OWN is set. Would it make sense for
poll_tx() to re-check the ring after re-enabling the interrupt and
reschedule itself if a completed descriptor is pending?

Second, the WRITE_ONCE(tx_ring->read_idx, read_idx) at the end of
bcm4908_enet_poll_tx() is not a release. On arm64 that store can
become visible before the earlier loads of buf_desc->ctl,
slot->dma_addr, slot->len and slot->skb in the loop are done. When
poll_tx() reclaims several slots in one pass from a full ring,
bcm4908_enet_tx_free_descs() in start_xmit() on another CPU can
already count them as free, and start_xmit() refills slots[] and
buf_desc[] for them. Could poll_tx() then unmap the new buffer, free
the new skb while the DMA is reading it, or report the wrong length
to BQL?

Would smp_store_release(&tx_ring->read_idx, read_idx) here, paired
with smp_load_acquire() of read_idx in bcm4908_enet_tx_free_descs(),
be enough? The barrier in netif_txq_completed_wake() comes after
this store, so it does not order the loads in the loop before it.

[Severity: High]
This is a pre-existing issue, but does this comment still hold when the
device tree has no "tx" interrupt?

The binding (brcm,bcm4908-enet.yaml) has minItems: 1 for interrupts and
interrupt-names, so a DT with only "rx" is valid. In that case
bcm4908_enet_probe() stores a negative value in enet->irq_tx, and
bcm4908_enet_open() never enables the TX NAPI:

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

netif_napi_add() leaves NAPI_STATE_SCHED set until napi_enable(). So the
napi_schedule() fallback in bcm4908_enet_start_xmit() never does
anything: napi_schedule_prep() sees SCHED already set and returns false,
and bcm4908_enet_poll_tx() never runs.

After this patch, poll_tx()->netif_txq_completed_wake() is the only path
that wakes the queue. Once length - 1 frames are queued, or the BQL limit
is reached, netif_txq_maybe_stop() stops the queue. start_xmit() is then
never called again, so TX stays stopped for good.

On ifdown, bcm4908_enet_stop() calls napi_disable(&tx_ring->napi)
unconditionally. Does napi_disable_locked() then spin forever with RTNL
held, waiting for SCHED/NPSVC to clear on a NAPI that was never enabled?
stop() also calls free_irq(enet->irq_tx, enet) with a negative IRQ.

This dates back to commit 12bb508bfe5a ("net: broadcom: bcm4908_enet:
support TX interrupt"). The later patches in this series don't seem to
change the open/stop gating or the irq_tx < 0 path.

Would it make sense to always enable the TX NAPI and only free the TX IRQ
when irq_tx > 0? Another option is to make the TX interrupt mandatory.

>  
>  	return NETDEV_TX_OK;
>  }
> @@ -652,17 +662,19 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)

[ ... ]

> +	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);

[Severity: High]
This isn't a bug introduced by this patch, but is a read barrier missing
between the write_idx load and the descriptor and slot loads here?

write_idx is not read with smp_load_acquire(), and there is no
smp_rmb()/dma_rmb() before the reads of buf_desc->ctl and slot->dma_addr,
slot->len and slot->skb. Only a control dependency links them, and on
arm64 that doesn't order a load against a later load.

Nothing clears the ctl word of a free descriptor, so a slot completed on
the previous lap still has OWN=0. That looks the same as a frame that
just completed. Suppose poll_tx() has caught up, so read_idx == write_idx
== i:

CPU1 (start_xmit, under the TX queue lock)
    fill slots[i], set buf_desc[i].ctl = OWN
    bcm4908_enet_dma_tx_ring_enable()
    WRITE_ONCE(ring->write_idx, i + 1)

CPU2 (poll_tx, NAPI, does not take the TX queue lock)
    load buf_desc[i].ctl early -> stale OWN=0
    READ_ONCE(tx_ring->write_idx) -> i + 1
    dma_unmap_single() / dev_kfree_skb() on slot i

Could this unmap and free the skb that the DMA is still reading? And if
the slot loads return the previous lap's values, could it double free
the old skb and leak the new one? BQL and the statistics would also be
updated too early.

The patch turns this loop into an explicit lockless producer/consumer
scheme, but the netif_txq_* helpers only order the stop/wake decision.
The later patch in this series that publishes the buffer address before
OWN adds dma_wmb()/dma_rmb() only to the producer and RX paths. poll_tx()
is the same at the end of the series.

Would it be enough to use smp_load_acquire(&tx_ring->write_idx) paired
with smp_store_release() in start_xmit()? Adding an smp_rmb() after the
index check would be another option.

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

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

[Kordian Chmielewski]

When dma_map_single() fails, start_xmit() returns NETDEV_TX_BUSY with
the queue still running, after it has already stored the skb in the
slot. Documentation/networking/driver.rst treats NETDEV_TX_BUSY under
normal circumstances as a hard error, and a driver that returns it must
not keep any reference to the skb. Here the stack requeues the frame
and retries it while the slot still points at it. Drop the frame
instead: free the skb, count it as dropped and return NETDEV_TX_OK.
Map the buffer before the slot is touched.

Found by reading the code.

Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
Signed-off-by: Kordian Chmielewski <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 60f711e8c..b1738d954 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -537,6 +537,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 */
@@ -555,12 +556,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

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

[Florian Fainelli]

On 11/12/26 11:50, Kordian Chmielewski wrote:
> When dma_map_single() fails, start_xmit() returns NETDEV_TX_BUSY with
> the queue still running, after it has already stored the skb in the
> slot. Documentation/networking/driver.rst treats NETDEV_TX_BUSY under
> normal circumstances as a hard error, and a driver that returns it must
> not keep any reference to the skb. Here the stack requeues the frame
> and retries it while the slot still points at it. Drop the frame
> instead: free the skb, count it as dropped and return NETDEV_TX_OK.
> Map the buffer before the slot is touched.
> 
> Found by reading the code.
> 
> Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
> Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>

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

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

[Kordian Chmielewski]

bcm4908_enet_stop() disables the TX DMA but leaves the ring as it is:
the skbs that poll_tx() has not freed yet stay mapped and are never
freed, and the descriptors the DMA has not sent yet keep
DMA_CTL_STATUS_OWN. The skbs, the socket memory they hold and their
mappings leak. The next open starts the indexes from 0 again over
those descriptors, so the DMA may send stale frames from before the
stop, and if the first descriptor still has OWN, start_xmit() stops
the queue before it enables the DMA, with nothing left to wake it.

Wait for the TX DMA channel to stop, as is already done for RX, then
unmap and free the skbs between read_idx and write_idx, and clear the
TX descriptors when the ring is initialised. A descriptor that
start_xmit() is about to use then never carries OWN, so drop the check
that stopped the queue on one. The RX helper's timeout warning said
TX; make it say RX.

Found by reading the code; I have not seen these on the hardware.

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

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index b1738d954..a28adef03 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -313,6 +313,11 @@ 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->buf_desc, 0,
+		       ring->length * sizeof(*ring->buf_desc));
+
 	ring->read_idx = 0;
 	ring->write_idx = 0;
 }
@@ -365,7 +370,22 @@ static void bcm4908_enet_dma_tx_ring_enable(struct bcm4908_enet *enet,
 static void bcm4908_enet_dma_tx_ring_disable(struct bcm4908_enet *enet,
 					     struct bcm4908_enet_dma_ring *ring)
 {
+	unsigned long deadline;
+	u32 tmp;
+
 	enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, 0);
+
+	/* Wait for the channel to stop, as for RX, before freeing buffers */
+	deadline = jiffies + usecs_to_jiffies(2000);
+	do {
+		tmp = enet_read(enet, ring->cfg_block + ENET_DMA_CH_CFG);
+		if (!(tmp & ENET_DMA_CH_CFG_ENABLE))
+			return;
+		enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, 0);
+		usleep_range(10, 30);
+	} while (!time_after_eq(jiffies, deadline));
+
+	dev_warn(enet->dev, "Timeout waiting for DMA TX stop\n");
 }
 
 static void bcm4908_enet_dma_rx_ring_enable(struct bcm4908_enet *enet,
@@ -391,7 +411,7 @@ static void bcm4908_enet_dma_rx_ring_disable(struct bcm4908_enet *enet,
 		usleep_range(10, 30);
 	} while (!time_after_eq(jiffies, deadline));
 
-	dev_warn(enet->dev, "Timeout waiting for DMA TX stop\n");
+	dev_warn(enet->dev, "Timeout waiting for DMA RX stop\n");
 }
 
 /***
@@ -496,6 +516,21 @@ 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 = tx_ring->read_idx; i != tx_ring->write_idx;
+	     i = i == tx_ring->length - 1 ? 0 : i + 1) {
+		struct bcm4908_enet_dma_ring_slot *slot = &tx_ring->slots[i];
+
+		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
+		dev_kfree_skb_any(slot->skb);
+	}
+}
+
 static int bcm4908_enet_stop(struct net_device *netdev)
 {
 	struct bcm4908_enet *enet = netdev_priv(netdev);
@@ -511,6 +546,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);
@@ -549,13 +585,6 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 	if (!netif_txq_maybe_stop(txq, bcm4908_enet_tx_free_descs(ring), 1, 1))
 		return NETDEV_TX_BUSY;
 
-	/* Hardware removes OWN bit after sending data */
-	buf_desc = &ring->buf_desc[ring->write_idx];
-	if (unlikely(le32_to_cpu(buf_desc->ctl) & DMA_CTL_STATUS_OWN)) {
-		netif_stop_queue(netdev);
-		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);
@@ -563,6 +592,7 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 		return NETDEV_TX_OK;
 	}
 
+	buf_desc = &ring->buf_desc[ring->write_idx];
 	slot = &ring->slots[ring->write_idx];
 	slot->skb = skb;
 	slot->len = skb->len;
-- 
2.56.0

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

[Florian Fainelli]

On 11/12/26 11:50, Kordian Chmielewski wrote:
> bcm4908_enet_stop() disables the TX DMA but leaves the ring as it is:
> the skbs that poll_tx() has not freed yet stay mapped and are never
> freed, and the descriptors the DMA has not sent yet keep
> DMA_CTL_STATUS_OWN. The skbs, the socket memory they hold and their
> mappings leak. The next open starts the indexes from 0 again over
> those descriptors, so the DMA may send stale frames from before the
> stop, and if the first descriptor still has OWN, start_xmit() stops
> the queue before it enables the DMA, with nothing left to wake it.
> 
> Wait for the TX DMA channel to stop, as is already done for RX, then
> unmap and free the skbs between read_idx and write_idx, and clear the
> TX descriptors when the ring is initialised. A descriptor that
> start_xmit() is about to use then never carries OWN, so drop the check
> that stopped the queue on one. The RX helper's timeout warning said
> TX; make it say RX.
> 
> Found by reading the code; I have not seen these on the hardware.
> 
> Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
> Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>

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

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

[netdev-bot+sashiko]

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

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

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

- [High] Double free and double DMA unmap of TX skbs, introduced by the
  new bcm4908_enet_tx_ring_clean().
- [Medium] bcm4908_enet_stop() now frees in-flight TX buffers even when
  the TX DMA channel was never confirmed stopped.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is High
  severity and it stops the patch's new cleanup from ever running in…

--

net: bcm4908_enet: release the TX ring on stop

This patch makes bcm4908_enet_stop() wait for the TX DMA channel to stop,
then unmap and free the skbs still left between read_idx and write_idx. It
also clears the TX descriptors when the ring is initialised, and drops the
OWN check in bcm4908_enet_start_xmit().

> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
> index b1738d954b91a..a28adef03e0e3 100644
> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c

[ ... ]

> @@ -365,7 +370,22 @@ static void bcm4908_enet_dma_tx_ring_enable(struct bcm4908_enet *enet,
>  static void bcm4908_enet_dma_tx_ring_disable(struct bcm4908_enet *enet,
>  					     struct bcm4908_enet_dma_ring *ring)
>  {
> +	unsigned long deadline;
> +	u32 tmp;
> +
>  	enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, 0);
> +
> +	/* Wait for the channel to stop, as for RX, before freeing buffers */
> +	deadline = jiffies + usecs_to_jiffies(2000);
> +	do {
> +		tmp = enet_read(enet, ring->cfg_block + ENET_DMA_CH_CFG);
> +		if (!(tmp & ENET_DMA_CH_CFG_ENABLE))
> +			return;
> +		enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, 0);
> +		usleep_range(10, 30);
> +	} while (!time_after_eq(jiffies, deadline));
> +
> +	dev_warn(enet->dev, "Timeout waiting for DMA TX stop\n");
>  }

[Severity: Medium]
When this poll times out, the helper only warns and returns void.
bcm4908_enet_stop() then calls bcm4908_enet_tx_ring_clean() anyway, which
unmaps and frees the skbs behind descriptors that may still have
DMA_CTL_STATUS_OWN set.

After stop, ENET_DMA_CTRL_CFG_MASTER_EN and the UMAC CMD_TX_EN bit also stay
set. The channel reset and the descriptor clear only happen in the next
open().

If the TX engine has not really stopped, can it keep fetching those buffers
and send the contents of freed, possibly reused, pages onto the wire? Before
this patch those skbs were leaked, so they were never freed while the DMA
could still reach them. I could not confirm from the code whether this
engine keeps fetching after ENABLE is cleared.

The commit message says the wait is there so the skbs are freed only after
the channel has stopped, but on the timeout path they are freed anyway.

Also, at HZ=100 or HZ=250, usecs_to_jiffies(2000) rounds up to one jiffy. If
a tick lands during the first usleep_range(10, 30), the loop gives up after
roughly 10-30us. Is the 2ms timeout actually honoured here?

Could the helper report the timeout, so that stop() can force a channel
reset through ENET_DMA_CTRL_CHANNEL_RESET (or clear MASTER_EN and confirm
the stop), or else leak the buffers deliberately instead of freeing them?

[ ... ]

> @@ -496,6 +516,21 @@ 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 = tx_ring->read_idx; i != tx_ring->write_idx;
> +	     i = i == tx_ring->length - 1 ? 0 : i + 1) {
> +		struct bcm4908_enet_dma_ring_slot *slot = &tx_ring->slots[i];
> +
> +		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
> +		dev_kfree_skb_any(slot->skb);
> +	}
> +}

[Severity: High]
This frees every skb in slots[read_idx..write_idx) but leaves read_idx,
write_idx and slot->skb as they were. Emptying the ring is left to
bcm4908_enet_dma_ring_init() in the next open. Is that call always reached?

bcm4908_enet_dma_init() fills the RX ring first and returns early if that
fails:

	for (i = 0; i < rx_ring->length; i++) {
		err = bcm4908_enet_dma_alloc_rx_buf(enet, i);
		if (err) {
			dev_err(dev, "Failed to alloc RX buffer: %d\n", err);
			bcm4908_enet_dma_uninit(enet);
			return err;
		}
	}

	bcm4908_enet_dma_ring_init(enet, &enet->tx_ring);

bcm4908_enet_open() ignores that return value and returns 0:

	bcm4908_enet_dma_reset(enet);
	bcm4908_enet_dma_init(enet);

Suppose napi_alloc_frag() or dma_map_single() fails in
bcm4908_enet_dma_alloc_rx_buf() during ifup. The interface then comes up
with the old TX read_idx/write_idx range. Those slots still point at skbs
that were already freed, and the new TX descriptor memset did not run
either.

On the next ifdown, won't bcm4908_enet_tx_ring_clean() walk the same range
and call dma_unmap_single() and dev_kfree_skb_any() on the same skbs a
second time?

bcm4908_enet_poll_tx() can also free them again before that, for any stale
descriptor in the range with OWN clear:

		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_TO_DEVICE);
		dev_kfree_skb(slot->skb);

The ignored return value in open() predates this patch. However, before this
patch those skbs were leaked rather than freed, so the stale pointers were
still valid.

Would it be safer to make the ring empty inside
bcm4908_enet_tx_ring_clean() itself, for example by resetting read_idx and
write_idx and/or setting slot->skb to NULL?

>  static int bcm4908_enet_stop(struct net_device *netdev)
>  {
>  	struct bcm4908_enet *enet = netdev_priv(netdev);
> @@ -511,6 +546,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);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. It does
mean the new cleanup is never reached when the device tree has no "tx"
interrupt.

The brcm,bcm4908-enet binding allows a single interrupt (interrupt-names has
minItems: 1). Probe accepts a negative value from
platform_get_irq_byname(pdev, "tx"). In that case bcm4908_enet_open() only
enables the TX NAPI conditionally:

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

but bcm4908_enet_stop() disables it unconditionally, before the
bcm4908_enet_tx_ring_clean() call above:

	napi_disable(&rx_ring->napi);
	napi_disable(&tx_ring->napi);

A TX NAPI that was never enabled still has NAPI_STATE_SCHED and
NAPI_STATE_NPSVC set from netif_napi_add, and only napi_enable() clears
them. So napi_disable_locked() sits in:

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

Does ndo_stop then hang forever with rtnl and the netdev lock held?

The same mode has a second problem. In bcm4908_enet_start_xmit(),
napi_schedule(&enet->tx_ring.napi) does nothing, because napi_schedule_prep()
sees SCHED already set, so bcm4908_enet_poll_tx() never runs. Doesn't the
TX ring then fill until netif_txq_maybe_stop() stops the queue, with nothing
left to wake it?

The conditional napi_enable() and the unconditional napi_disable() appear
to come from commit 12bb508bfe5a ("net: broadcom: bcm4908_enet: support TX
interrupt").

[ ... ]

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

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

[Kordian Chmielewski]

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.

Found by reading the code.

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

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index a28adef03..7f80e4e24 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -658,6 +658,8 @@ 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

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

[Florian Fainelli]

On 11/12/26 11:50, Kordian Chmielewski wrote:
> 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.
> 
> Found by reading the code.
> 
> Fixes: 4dc7f09b8bec ("net: broadcom: bcm4908_enet: fix RX path possible mem leak")
> Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>

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

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

[Kordian Chmielewski]

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.

Found by reading the code.

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

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 7f80e4e24..8ac749c44 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -333,7 +333,8 @@ 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

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

[Florian Fainelli]

On 11/12/26 11:50, Kordian Chmielewski wrote:
> 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.
> 
> Found by reading the code.
> 
> Fixes: 3a1cc23a75ab ("net: broadcom: bcm4908_enet: use build_skb()")
> Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>

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

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

[Kordian Chmielewski]

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. If the DMA read the descriptor between the two
stores, it would write 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()), as Documentation/memory-barriers.txt
describes for descriptors shared with a device.

Found by reading the code; I have no test that shows the DMA reading a
descriptor while it is being refilled.

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

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 8ac749c44..3d44b3452 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -290,8 +290,10 @@ 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);
 	buf_desc->addr = cpu_to_le32(slot->dma_addr);
+	/* Hand the descriptor to the DMA only once it holds the new buffer */
+	dma_wmb();
+	buf_desc->ctl = cpu_to_le32(tmp);
 
 	return 0;
 }
@@ -610,6 +612,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);
@@ -644,6 +648,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;
+		/* Read the rest only after the DMA gave the descriptor back */
+		dma_rmb();
 
 		slot = enet->rx_ring.slots[enet->rx_ring.read_idx];
 
-- 
2.56.0

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

[Florian Fainelli]

On 11/12/26 11:50, Kordian Chmielewski wrote:
> 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. If the DMA read the descriptor between the two
> stores, it would write 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()), as Documentation/memory-barriers.txt
> describes for descriptors shared with a device.
> 
> Found by reading the code; I have no test that shows the DMA reading a
> descriptor while it is being refilled.
> 
> Fixes: 4feffeadbcb2 ("net: broadcom: bcm4908enet: add BCM4908 controller driver")
> Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>

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