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

[PATCH net v3 0/7] 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 7 come
from reading the code and from review, 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 four
ifdown/ifup cycles of the conduit while it was sending. I ran the same
again, plus a UDP stream at 1 Gbit/s (944 Mbit/s received), with the TX
ring shrunk to 16 descriptors for the test, so that the queue was
stopped and woken over and over (about a million qdisc requeues in a
minute, with the ring full or nearly full in a third of the one-second
samples). No stall, no errors in the counters, no DMA timeout warnings.

Sashiko's review of v2 also pointed at older problems that this series
doesn't fix: open() ignores a failure to fill the RX ring, and fills
it with napi_alloc_frag() from process context; a failed RX refill
loses the old buffer; and the driver does not work without its
optional TX interrupt. I'll send fixes for those after this series.
It also found that a TX completion reported before start_xmit()
publishes write_idx can be missed; patch 7 narrows that window, and
I'll look into closing it.

Changes in v3:
- patch 3: leave the TX ring empty after freeing it, so that a failed
  open() cannot lead to the same skbs being freed again; wait for the
  TX channel with read_poll_timeout(), as a 2 ms deadline in jiffies
  can expire after one poll; leave the skbs alone if the channel does
  not stop (Sashiko)
- new patch 7: order the TX ring index accesses between start_xmit()
  and poll_tx(), and publish write_idx before the DMA kick (Sashiko)
- add Florian's Reviewed-by to patches 1, 2 and 4 to 6, not to patch 3,
  which changed

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 v1 patch 7 (RX frames longer than the buffer): its trigger is
  not reachable with the driver's own settings
- wrap lines over 80 columns; author name without the nickname

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

Kordian Chmielewski (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: order the TX ring index accesses

 drivers/net/ethernet/broadcom/bcm4908_enet.c | 143 ++++++++++++++-----
 1 file changed, 107 insertions(+), 36 deletions(-)


base-commit: 86706ffa990ecd46a4326f8eecbccbbd60a6cb8d
-- 
2.56.1

[PATCH net v3 1/7] 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")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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.1

Re: [PATCH net v3 1/7] 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 (1) · Medium: 1 (3) · 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] bcm4908_enet_start_xmit()
  (drivers/net/ethernet/broadcom/bcm4908_enet.c:553-555) still has a…

Pre-existing issues:
- [High] The DT binding makes the 'tx' interrupt optional (interrupts
  minItems: 1), and bcm4908_enet_probe() keeps a negative enet->irq_tx…
- [Medium] At this commit, bcm4908_enet_stop() never reaps the TX ring.
- [Medium] At this commit, the lockless TX ring protocol between
  bcm4908_enet_start_xmit() and bcm4908_enet_poll_tx() publishes and…
- [Medium] At this commit, bcm4908_enet_start_xmit() writes
  buf_desc->addr and then buf_desc->ctl (with OWN) to…

--

net: bcm4908_enet: fix the TX ring wrap

The patch fixes the mismatch between how start_xmit() and poll_tx() wrap
the TX ring indexes. Before the fix, a full ring could look empty, and
the queue could stall for good. All descriptors are now used, with WRAP
on the last one, both indexes wrap at length and one descriptor stays
free. Stopping and waking the queue now goes through the netdev_queues.h
helpers.

> 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)
>  	return 0;
>  }

[Severity: Medium]
This is a pre-existing issue, but bcm4908_enet_stop() never reaps the TX
ring:

bcm4908_enet_stop() {
    ...
	napi_disable(&tx_ring->napi);
	netdev_reset_queue(netdev);

	bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
	bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);

	bcm4908_enet_dma_uninit(enet);
    ...
}

bcm4908_enet_dma_uninit() only walks rx_ring->slots. Do the skbs between
read_idx and write_idx stay DMA mapped and leak on ifdown?

On the next open, bcm4908_enet_dma_ring_init() sets read_idx and
write_idx back to 0 but does not clear the TX descriptors, so stale OWN
bits can stay set.

The later commit "net: bcm4908_enet: release the TX ring on stop" in this
series fixes this. It adds bcm4908_enet_tx_ring_clean(), calls it from
bcm4908_enet_stop() after the TX channel has stopped, and clears the TX
descriptors in bcm4908_enet_dma_ring_init().

>  
> +/* 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)
>  {

[ ... ]

>  	/* 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 isn't a bug introduced by this patch, but what happens here when the
DT leaves out the optional "tx" interrupt?

The binding allows a single interrupt (interrupts: minItems: 1).
bcm4908_enet_probe() keeps the negative enet->irq_tx and carries on, and
bcm4908_enet_open() then skips enabling the TX NAPI:

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

A NAPI added with netif_napi_add_tx() starts with NAPI_STATE_SCHED and
NAPI_STATE_NPSVC set. So this napi_schedule() only sets MISSED, and
bcm4908_enet_poll_tx() never runs.

Does that mean no TX skb is ever reclaimed? After length - 1 frames,
netif_txq_maybe_stop() would stop the queue with nothing to wake it.
That is the "queue stops for good with nothing left to wake it" symptom
from the commit message. The new "poll_tx() wakes the queue once there
is room again" comment also doesn't hold in this mode.

bcm4908_enet_stop() also calls napi_disable(&tx_ring->napi)
unconditionally. SCHED and NPSVC never clear on a NAPI that was never
enabled. Can napi_disable_locked() then loop forever while holding RTNL
and the netdev lock?

stop() then also calls free_irq() with the negative enet->irq_tx.

This is still present at the end of the series. open() still enables the
TX NAPI only when irq_tx > 0, and stop() still calls napi_disable() on it
every time.

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

[Severity: Medium]
The OWN check just after this line still stops the queue directly,
without the netdev_queues.h helpers:

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

This patch removes the unconditional netif_wake_queue() from
bcm4908_enet_poll_tx(). The only waker left is
netif_txq_completed_wake(), and it only wakes when packets were
completed:

__netif_txq_completed_wake() {
    ...
	if (pkts && likely(get_desc >= start_thrs)) {
    ...
}

Suppose start_xmit() takes this path while the indexes say the ring is
empty. One way is a stale OWN descriptor at index 0 after ifdown/ifup:
bcm4908_enet_dma_ring_init() resets read_idx and write_idx to 0 but
leaves the descriptors alone. poll_tx() then has handled == 0.

Is the queue ever woken in that case?

The old code had no way out of this case either, because poll_tx() is not
scheduled when no DMA was kicked. Still, the new "wakes the queue if
start_xmit() stopped it" comment in poll_tx() does not cover this stop.

The later commit "net: bcm4908_enet: release the TX ring on stop" in this
series removes this OWN check and clears the TX descriptors in
bcm4908_enet_dma_ring_init().

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

[Severity: Medium]
This isn't a bug introduced by this patch, but right after this the
descriptor goes to the DMA with no barrier between the two stores:

	buf_desc->addr = cpu_to_le32((uint32_t)slot->dma_addr);
	buf_desc->ctl = cpu_to_le32(tmp);

With no dma_wmb() between them, can a DMA engine that is already running
see OWN before the new address? It would then transmit from a stale,
unmapped buffer.

bcm4908_enet_dma_alloc_rx_buf() uses the opposite order and writes ctl,
with OWN, before addr:

	buf_desc->ctl = cpu_to_le32(tmp);
	buf_desc->addr = cpu_to_le32(slot->dma_addr);

The later commit "net: bcm4908_enet: publish the buffer address before
OWN" in this series adds dma_wmb() between the stores on both paths,
writes addr first on RX, and adds dma_rmb() in poll_rx().

[ ... ]

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

[ ... ]

> -	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: Medium]
This is a pre-existing issue, since the old loop used plain loads here.
Is READ_ONCE() enough for this lockless index protocol, though?

start_xmit() publishes write_idx with WRITE_ONCE() after it writes the
slot and the descriptor. This loop reads write_idx with READ_ONCE() and
then loads buf_desc->ctl and slots[read_idx].

On arm64, could those loads return the previous lap's values, with OWN
clear and an skb that was already freed? That would call
dma_unmap_single() and dev_kfree_skb() a second time on that skb, and the
new frame would be lost.

The other direction has the same gap. The slot loads here are not
release-ordered before WRITE_ONCE(tx_ring->read_idx, read_idx). Also,
bcm4908_enet_tx_free_descs() reads read_idx with READ_ONCE() before
start_xmit() reuses the slot.

The later commit "net: bcm4908_enet: order the TX ring index accesses" in
this series switches all of these to smp_store_release() and
smp_load_acquire(), for both write_idx and read_idx, including in
bcm4908_enet_tx_free_descs().

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

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

[Kordian Chmielewski]

Sashiko's review of this patch [1] attributes one Medium point to it:
start_xmit() still stops the queue with netif_stop_queue() when the
descriptor at write_idx has OWN set, and poll_tx() no longer wakes the
queue unconditionally, so a stale OWN descriptor after ifdown/ifup could
stop the queue for good.

In the case the review describes, the ring is empty: start_xmit()
returns before anything has kicked the DMA, so no TX interrupt comes
and poll_tx() never runs. That stop could not be undone before this
patch either; patch 3's changelog describes it. With stale OWN
descriptors further in the ring and frames in flight, the old
unconditional wake could undo such a stop and the new wake does not,
until patch 3 removes the check and clears the TX descriptors when the
ring is initialised. I left that change in patch 3 so that this one
stays the ring wrap fix.

The other points in that review are older than this series. Patches 3,
6 and 7 fix three of them, and the missing TX interrupt is the question
I asked under v2.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113235847.517308-1-kudzo51%40gmail.com

[PATCH net v3 2/7] 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")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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.1

[PATCH net v3 3/7] 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 leave the
ring empty, so that nothing can free them a second time. If the
channel does not stop in time, leave the skbs alone rather than free
them while the DMA may still read them. 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 | 55 ++++++++++++++++----
 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index b1738d954..bb42b6ecd 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -7,6 +7,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_vlan.h>
 #include <linux/interrupt.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
@@ -313,6 +314,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;
 }
@@ -362,10 +368,22 @@ static void bcm4908_enet_dma_tx_ring_enable(struct bcm4908_enet *enet,
 	enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, ENET_DMA_CH_CFG_ENABLE);
 }
 
-static void bcm4908_enet_dma_tx_ring_disable(struct bcm4908_enet *enet,
-					     struct bcm4908_enet_dma_ring *ring)
+static int bcm4908_enet_dma_tx_ring_disable(struct bcm4908_enet *enet,
+					    struct bcm4908_enet_dma_ring *ring)
 {
+	u32 tmp;
+	int err;
+
 	enet_write(enet, ring->cfg_block + ENET_DMA_CH_CFG, 0);
+
+	/* Wait for the channel to stop, as for RX, before freeing buffers */
+	err = read_poll_timeout(enet_read, tmp, !(tmp & ENET_DMA_CH_CFG_ENABLE),
+				10, 2000, false, enet,
+				ring->cfg_block + ENET_DMA_CH_CFG);
+	if (err)
+		dev_warn(enet->dev, "Timeout waiting for DMA TX stop\n");
+
+	return err;
 }
 
 static void bcm4908_enet_dma_rx_ring_enable(struct bcm4908_enet *enet,
@@ -391,7 +409,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 +514,25 @@ 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);
+	}
+
+	/* Leave the ring empty, so that nothing frees these skbs again */
+	tx_ring->read_idx = 0;
+	tx_ring->write_idx = 0;
+}
+
 static int bcm4908_enet_stop(struct net_device *netdev)
 {
 	struct bcm4908_enet *enet = netdev_priv(netdev);
@@ -509,8 +546,10 @@ static int bcm4908_enet_stop(struct net_device *netdev)
 	netdev_reset_queue(netdev);
 
 	bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
-	bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
 
+	/* Leave the TX buffers alone if the channel may still read them */
+	if (!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 +588,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 +595,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.1

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · 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] The patch does not fix the TX skb leak when the TX channel
  times out, and on that path it leaves the hardware running.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it makes the
  new TX-ring release in bcm4908_enet_stop() unreachable when the DT has…
- [High] This problem wasn't introduced by this patch, but the patch
  applies the 'do not free buffers the DMA may still use' reasoning only…
- [High] This problem wasn't introduced by this patch, but the patch now
  depends on the code it bypasses.

--

net: bcm4908_enet: release the TX ring on stop

This patch makes bcm4908_enet_stop() wait for the TX DMA channel to stop
and then unmap and free the skbs left in the TX ring. If the channel does
not stop in time, the free is skipped. It also clears the TX descriptors
in bcm4908_enet_dma_ring_init(), drops the OWN check from
bcm4908_enet_start_xmit(), and fixes the RX timeout warning text.

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

[ ... ]

> @@ -313,6 +314,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;
>  }

[Severity: High]
This is a pre-existing issue, but the TX descriptor clear and index reset
added here only run if bcm4908_enet_dma_init() gets through the RX buffer
allocation loop:

bcm4908_enet_dma_init() {
	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_dma_ring_init(enet, &enet->rx_ring);
	...
}

bcm4908_enet_open() ignores that return value:

	bcm4908_enet_dma_reset(enet);
	bcm4908_enet_dma_init(enet);

	enet_umac_set(enet, UMAC_CMD, CMD_TX_EN | CMD_RX_EN);

	enet_set(enet, ENET_DMA_CONTROLLER_CFG, ENET_DMA_CTRL_CFG_MASTER_EN);

If napi_alloc_frag() or dma_map_single() fails for any RX slot, the
interface still comes up with MASTER_EN set and the RX channel enabled.
bcm4908_enet_dma_reset() has just set ENET_DMA_CH_STATE_RAM_BASE_DESC_PTR
to 0, and nothing programs it again. ndo_open still returns 0.

The OWN check is now removed from bcm4908_enet_start_xmit(), so this
memset is the only thing that stops start_xmit() from reusing a
descriptor that still has OWN set. After an earlier timed-out stop,
wouldn't the stale read_idx/write_idx and OWN descriptors survive this
path?

Should bcm4908_enet_open() check the result of bcm4908_enet_dma_init()
and unwind on failure?

[ ... ]

> @@ -509,8 +546,10 @@ static int bcm4908_enet_stop(struct net_device *netdev)
>  	netdev_reset_queue(netdev);

[Severity: High]
This isn't a bug introduced by this patch, but can the new TX clean be
reached when the DT has no "tx" interrupt?

The binding allows a single interrupt (minItems: 1). In that case
enet->irq_tx is negative and bcm4908_enet_open() skips the TX
napi_enable():

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

bcm4908_enet_stop() calls napi_disable(&tx_ring->napi) unconditionally,
just above this hunk. netif_napi_add_weight_locked() sets
NAPI_STATE_SCHED and NAPI_STATE_NPSVC, and only napi_enable() clears
them. So napi_disable_locked() would loop forever:

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

That hangs ndo_stop with RTNL held, before bcm4908_enet_tx_ring_clean()
is reached.

The same configuration also breaks TX. The irq_tx < 0 branch in
bcm4908_enet_start_xmit() calls napi_schedule() on a NAPI that still has
SCHED set. napi_schedule_prep() then fails, so bcm4908_enet_poll_tx()
never runs. After 199 frames the queue stops and nothing wakes it.

>  
>  	bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
> -	bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
>  
> +	/* Leave the TX buffers alone if the channel may still read them */
> +	if (!bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring))
> +		bcm4908_enet_tx_ring_clean(enet);

[Severity: Medium]
What happens to the skbs between read_idx and write_idx when
bcm4908_enet_dma_tx_ring_disable() times out?

On that path bcm4908_enet_tx_ring_clean() is skipped and nothing else is
done to the hardware. The TX subchannel is not reset through
ENET_DMA_CTRL_CHANNEL_RESET, and both ENET_DMA_CTRL_CFG_MASTER_EN and
UMAC CMD_TX_EN stay set.

Nothing frees those slots later either. On the next open,
bcm4908_enet_dma_reset() clears MASTER_EN, and
bcm4908_enet_dma_ring_init() resets the channel, clears the descriptors
and zeroes the indexes. None of that unmaps or frees what is left in
tx_ring->slots[].

Doesn't that leak the skbs, their DMA_TO_DEVICE mappings and the socket
memory they hold? The commit message says this leak is fixed.

If the device is unbound instead, the path is bcm4908_enet_remove() ->
unregister_netdev() -> bcm4908_enet_stop(), then
bcm4908_enet_dma_free(). That kfree()s tx_ring->slots without freeing
the skbs. It also calls dma_free_coherent() on the TX descriptor ring,
although the TX channel was never confirmed stopped or reset.

The comment reads as if the free is only postponed. Could the timeout
path force the channel idle first (reset the TX subchannel and/or clear
MASTER_EN, as ring_init() and dma_reset() do) and then clean the ring?

>  	bcm4908_enet_dma_uninit(enet);

[Severity: High]
This is a pre-existing issue, but doesn't the same reasoning apply to RX?
There the device writes into the buffers.

bcm4908_enet_dma_rx_ring_disable() returns void and only warns on
timeout. bcm4908_enet_dma_uninit() is then called unconditionally:

bcm4908_enet_dma_uninit() {
	...
		dma_unmap_single(dev, slot->dma_addr, slot->len, DMA_FROM_DEVICE);
		skb_free_frag(slot->buf);
		slot->buf = NULL;
	...
}

The RX descriptors still have DMA_CTL_STATUS_OWN and the buffer bus
address set by bcm4908_enet_dma_alloc_rx_buf(), and MASTER_EN is not
cleared.

If the RX channel has not stopped, can received frames be DMA-written
into pages that have already gone back to the page allocator?

Later patches in this series only change the unmap size in
bcm4908_enet_dma_uninit() and do not add a guard here.

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

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

[Kordian Chmielewski]

Sashiko's review of this patch [1] attributes one Medium point to it:
when the TX channel does not stop in time, the skbs left in the ring
leak, and on unbind bcm4908_enet_dma_free() frees the slots and the
descriptor ring while the channel may still be running. It suggests
forcing the channel idle with ENET_DMA_CTRL_CHANNEL_RESET or by clearing
MASTER_EN, and then cleaning the ring.

The leak after a timeout is on purpose, and the changelog says so. The
review of v2 suggested either forcing a reset or leaking the buffers.
I don't know what a channel reset does to a transfer in flight, so I
chose the leak: it costs memory, but the DMA never reads a freed skb.
The timeout never fired in my tests; every stop, all made while the
router was sending at line rate, finished in time. The unbind path is
older than this patch: before it, stop() did not wait for the TX
channel at all, and bcm4908_enet_dma_free() kfree()d the slot array
with the skbs still in it.

The review also points out that RX has the same problem the other way
round: bcm4908_enet_dma_rx_ring_disable() only warns on a timeout, and
stop() frees the RX buffers anyway, while the DMA may still write into
them. That one is older too, and I'll add it to the follow-ups, next to
the ignored dma_init() error in open().

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113235847.517308-1-kudzo51%40gmail.com

[PATCH net v3 4/7] 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")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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 bb42b6ecd..03de65d9b 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -661,6 +661,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.1

[PATCH net v3 5/7] 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()")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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 03de65d9b..4065769da 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -334,7 +334,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.1

[PATCH net v3 6/7] 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")
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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 4065769da..4139dad3a 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -291,8 +291,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;
 }
@@ -613,6 +615,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);
@@ -647,6 +651,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.1

[PATCH net v3 7/7] net: bcm4908_enet: order the TX ring index accesses

[Kordian Chmielewski]

Since the TX interrupt was added, poll_tx() runs in its own NAPI and
does not take the TX queue lock, so it and start_xmit() share the TX
ring with only the two indexes between them. Nothing orders the
accesses around those indexes:

- poll_tx() reads write_idx, then the descriptor and the slot. On arm64
  the later loads may be satisfied first, so poll_tx() may see the new
  write_idx together with the previous lap's descriptor, which has no
  OWN, and the previous lap's slot. It would then unmap and free a
  frame the DMA has not sent yet, or free the old skb a second time.
- poll_tx() publishes read_idx once it is done with the slots it
  freed, but that store is not ordered after its loads from those
  slots, so start_xmit() may refill a slot before poll_tx() has read
  it.

Publish each index with smp_store_release() and read the other side's
index with smp_load_acquire() before touching the slots. start_xmit()
now publishes write_idx right after the descriptor, before the MMIO
write that kicks the DMA rather than after it, which narrows the window
in which the DMA can complete a frame that poll_tx() cannot see yet.

Sashiko's review of the previous version of this series pointed this
out; I have not seen it on the hardware.

Fixes: 12bb508bfe5a ("net: broadcom: bcm4908_enet: support TX interrupt")
Signed-off-by: Kordian Chmielewski <kudzo51@gmail.com>
---
 drivers/net/ethernet/broadcom/bcm4908_enet.c | 26 ++++++++++++++------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c b/drivers/net/ethernet/broadcom/bcm4908_enet.c
index 4139dad3a..8bc620de2 100644
--- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
@@ -566,7 +566,9 @@ static int bcm4908_enet_stop(struct net_device *netdev)
  */
 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;
+	/* Pairs with smp_store_release() in bcm4908_enet_poll_tx() */
+	int free = smp_load_acquire(&ring->read_idx) -
+		   READ_ONCE(ring->write_idx) - 1;
 
 	return free < 0 ? free + ring->length : free;
 }
@@ -619,12 +621,14 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct sk_buff *skb, struct net_devic
 	dma_wmb();
 	buf_desc->ctl = cpu_to_le32(tmp);
 
-	bcm4908_enet_dma_tx_ring_enable(enet, &enet->tx_ring);
+	/* Publish the slot and the descriptor before the index; pairs with
+	 * smp_load_acquire() in bcm4908_enet_poll_tx()
+	 */
+	smp_store_release(&ring->write_idx,
+			  ring->write_idx < ring->length - 1 ?
+			  ring->write_idx + 1 : 0);
 
-	if (ring->write_idx == ring->length - 1)
-		WRITE_ONCE(ring->write_idx, 0);
-	else
-		WRITE_ONCE(ring->write_idx, ring->write_idx + 1);
+	bcm4908_enet_dma_tx_ring_enable(enet, &enet->tx_ring);
 
 	/* poll_tx() wakes the queue once there is room again */
 	netif_txq_maybe_stop(txq, bcm4908_enet_tx_free_descs(ring), 1, 1);
@@ -718,7 +722,10 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 	unsigned int bytes = 0;
 	int handled = 0;
 
-	while (handled < weight && read_idx != READ_ONCE(tx_ring->write_idx)) {
+	while (handled < weight) {
+		/* Pairs with the release in bcm4908_enet_start_xmit() */
+		if (read_idx == smp_load_acquire(&tx_ring->write_idx))
+			break;
 		buf_desc = &tx_ring->buf_desc[read_idx];
 		if (le32_to_cpu(buf_desc->ctl) & DMA_CTL_STATUS_OWN)
 			break;
@@ -733,7 +740,10 @@ static int bcm4908_enet_poll_tx(struct napi_struct *napi, int weight)
 		if (++read_idx == tx_ring->length)
 			read_idx = 0;
 	}
-	WRITE_ONCE(tx_ring->read_idx, read_idx);
+	/* Be done with the slots before start_xmit() may reuse them; pairs
+	 * with smp_load_acquire() in bcm4908_enet_tx_free_descs()
+	 */
+	smp_store_release(&tx_ring->read_idx, read_idx);
 
 	enet->netdev->stats.tx_packets += handled;
 	enet->netdev->stats.tx_bytes += bytes;
-- 
2.56.1