* [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up
@ 2026-10-22 9:38 Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 1/6] net: macb: take bp->lock around NCR read-modify-writes Steffen Dirkwinkel
` (6 more replies)
0 siblings, 7 replies; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, stable, Steffen Dirkwinkel
Hello Théo, Christian, all,
I've been looking at RCU and TX stalls on macb devices (Xilinx ZynqMP on
Beckhoff CX9240) for a while and have been carrying downstream patches
similar to Christian's. A while ago I let Claude try to reproduce this on my
devices, it was able to reproduce and found some fixes but I wasn't
happy with the quality of the patches and my understanding of them. I
briefly talked to Théo in Prague and he suggested I send what I have as
either the patches or the reproducers may be useful for ideas.
I've finally gotten around to it and also ran some more testing and improved
the patches in the meantime. The series includes Christian's patches in the
original form (should also at least be reworded) and I also have Patch 1 in here
since the bench seems to have reproduced the issue on hardware (see below).
If anyone has other ideas / patches for these issues I'm also happy to
run tests on devices.
Claude output from here:
This series fixes the TX stalls, TXUBR interrupt storms and RCU stalls we
reproduce on ZynqMP (GEM rev 0x50070106, two TX queues, PREEMPT_RT).
Patch 1 is Théo's NCR locking patch [1], unchanged; patches 2 and 3 are
Christian's v1 [2], unchanged; patches 4 to 6 are ours. Patch 6 is most
likely what Théo reported in [3].
Reproducers (all on the same ZynqMP board, RT config, IRQs on CPU 0,
rcu_cpu_stall_timeout=8, iperf3 -P10 against a host on the same link):
A. the link speed is toggled between 100 and 1000 Mbit/s every ~12 s
with "ethtool -s end0 speed {100,1000} duplex full autoneg on" while
the load runs; after every link-up the transmit counter, the netdev
watchdog counters and the interrupt rate are checked. A storm is
left running so the RCU stall detector can report it.
B. a managed switch cycles the DUT's two ports (one port only, the other
port only at 10 Mbit/s, back at autoneg), ~140 s per cycle; a hit is
an RCU stall on the serial console.
Results, vanilla net/main 78731b46967e, PREEMPT_RT:
driver A: failures/toggles B: hits/cycles
net/main watchdog at link-up 2, 1 TX stall episode
RCU stall at link-up 3 in 74 cycles
(idle mode: stall at 3)
patches 2-6 0/300 (idle mode 0/50) 0/74 (with load)
Same on a v7.2.7-rt tree with the net/main driver files (same bench):
net/main 8/9 toggles failed, RCU stall after 9 link-ups
patches 2-6 1/300 (one dead transmitter), 0 stalls in ~1300
Per patch, one patch removed at a time from the series (method A):
patch 2 (TBQP) alone off 0/100; TBQP reads the stale pre-link-down
index in 176/295 link-ups without the write,
0/555 with it (trace)
patch 3 (TXUBR mask) alone off 0/100
patches 2+3 off RCU stall at toggle 85 (1/153)
patches 2+3+4 off RCU stall at toggle 42 (1/41, p = 0.03)
patch 4 (rotate) alone off 1/97: TBQP stopped at index 0 on the used
descriptor of an unreaped frame, watchdog
patch 5 (kick) alone off 8/11 toggles: watchdogs, p = 7e-13
patch 6 (queue 0) alone off 1/100 watchdog; per-stream share at
100 Mbit/s 6.9 to 11.7 Mbit/s over both
queues vs 9.3 Mbit/s x10 with it
patches 5+6 off (2-4 only) 8/16 toggles: watchdogs on queue 0
So patches 2, 3 and 4 together prevent the storm and the RCU stall, patch
5 fixes the TX stall after link-up, patch 6 fixes the strict-priority
unfairness (no starvation-induced stall reproduced on this device).
Patch 1: once in ~300 toggles the transmitter stayed dead after a link-up
with patches 2-6. Tracing every NCR writer showed the cause:
macb_mac_link_down() clears RE|TE with an unlocked read-modify-write,
and macb_tx_restart() in the TX NAPI poll, which had read NCR with TE set
just before, writes its image back with TSTART 40 us to 900 us later. TE
is set again while the link is down, the next link-up starts with TE set,
the TBQP write of patch 2 is dropped and the ring is rotated under a
running transmitter. With the read-to-write gap of the TSTART path
widened to 1 ms to amplify the race: without patch 1, 3 dead transmitters
and an RCU stall in 792 toggles, 6 lost updates in 408 traced link-downs;
with patch 1, 0 in 1196 toggles (p = 0.025) and 0 lost updates in 1206
link-downs. The
traces (link_down clear, the stale write from irq/22-eth%d, TE set at
the next link-up) are available on request.
Answering the review of [2]: with the rest of the series in place,
removing patch 2 or patch 3 alone gives 0/100 each, removing both brings
the RCU stall back (1 in 153 toggles), removing 2, 3 and 4 gives 1 in 41;
TBQP of queue 0 keeps its
value while TE is low in about half of the link-ups on this GEM, as in
Christian's trace, while the fetch still starts at the base after TSTART.
Both reproducers were run on vanilla net/main; only PREEMPT_RT was tested.
Théo's steady-state case ("iperf3 -P10" for 30 min without transitions)
gave no watchdog on this device with or without the series.
[1] https://lore.kernel.org/netdev/20260918-macb-ncr-rmw-v1-1-30c3494f93cd@bootlin.com/
[2] https://lore.kernel.org/netdev/20260706-upstreaming-macb-irq-storm-v1-1-ab3115b5a13a@weidmueller.com/
[3] https://lore.kernel.org/netdev/DLIJN4CDS11K.RVJGK3UALMJE@bootlin.com/
---
Christian Taedcke (2):
net: macb: reprogram TBQP after shuffling the TX ring on link-up
net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms
Steffen Dirkwinkel (3):
net: macb: rotate unreaped TX frames behind the ring base on link-up
net: macb: kick the transmitter on link-up
net: macb: keep best-effort traffic on TX queue 0
Théo Lebrun (1):
net: macb: take bp->lock around NCR read-modify-writes
drivers/net/ethernet/cadence/macb_main.c | 80 ++++++++++++++++++++++++++------
1 file changed, 67 insertions(+), 13 deletions(-)
---
base-commit: 78731b46967e3e7102b5cd5062bef185f3404b5c
change-id: 20261022-std-macb-v14-send-6c5fd939b4e1
Best regards,
--
Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC net 1/6] net: macb: take bp->lock around NCR read-modify-writes
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
@ 2026-10-22 9:38 ` Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up Steffen Dirkwinkel
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, stable
From: Théo Lebrun <theo.lebrun@bootlin.com>
NCR is read-modify-written from many contexts:
- macb_mac_link_down() clears RE|TE,
- macb_mac_link_up() sets RE|TE|PTPUNI,
- macb_hresp_error_task() clears then re-sets RE|TE,
- macb_start_xmit() / macb_tx_restart() / macb_tx_error_task() set
TSTART (already under bp->lock),
- macb_interrupt() might toggle RE (also under bp->lock).
The first three risk concurrent RMW with anyone from the list as they
don't grab bp->lock.
Theoretical bugfix only, it has never reproduced on hardware. Also note
it does *not* take the lock hoping to protect against full races
inbetween BH scheduling concurrently. We only want to ensure consistent
NCR RMW operations.
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
---
drivers/net/ethernet/cadence/macb_main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 261a7e87520a..4b5037867537 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -777,6 +777,7 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
struct net_device *netdev = to_net_dev(config->dev);
struct macb *bp = netdev_priv(netdev);
struct macb_queue *queue;
+ unsigned long flags;
unsigned int q;
u32 ctrl;
@@ -786,8 +787,10 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP));
/* Disable Rx and Tx */
+ spin_lock_irqsave(&bp->lock, flags);
ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
macb_writel(bp, NCR, ctrl);
+ spin_unlock_irqrestore(&bp->lock, flags);
netif_tx_stop_all_queues(netdev);
}
@@ -940,11 +943,13 @@ static void macb_mac_link_up(struct phylink_config *config,
}
/* Enable Rx and Tx; Enable PTP unicast */
+ spin_lock_irqsave(&bp->lock, flags);
ctrl = macb_readl(bp, NCR);
if (gem_has_ptp(bp))
ctrl |= MACB_BIT(PTPUNI);
macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
+ spin_unlock_irqrestore(&bp->lock, flags);
netif_tx_wake_all_queues(netdev);
}
@@ -1995,6 +2000,7 @@ static void macb_hresp_error_task(struct work_struct *work)
struct macb *bp = from_work(bp, work, hresp_err_bh_work);
struct net_device *netdev = bp->netdev;
struct macb_queue *queue;
+ unsigned long flags;
unsigned int q;
u32 ctrl;
@@ -2003,9 +2009,11 @@ static void macb_hresp_error_task(struct work_struct *work)
MACB_TX_INT_FLAGS |
MACB_BIT(HRESP));
}
+ spin_lock_irqsave(&bp->lock, flags);
ctrl = macb_readl(bp, NCR);
ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
macb_writel(bp, NCR, ctrl);
+ spin_unlock_irqrestore(&bp->lock, flags);
netif_tx_stop_all_queues(netdev);
netif_carrier_off(netdev);
@@ -2022,8 +2030,10 @@ static void macb_hresp_error_task(struct work_struct *work)
MACB_TX_INT_FLAGS |
MACB_BIT(HRESP));
- ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
- macb_writel(bp, NCR, ctrl);
+ spin_lock_irqsave(&bp->lock, flags);
+ ctrl = MACB_BIT(RE) | MACB_BIT(TE);
+ macb_writel(bp, NCR, macb_readl(bp, NCR) | ctrl);
+ spin_unlock_irqrestore(&bp->lock, flags);
netif_carrier_on(netdev);
netif_tx_start_all_queues(netdev);
--
2.56.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 1/6] net: macb: take bp->lock around NCR read-modify-writes Steffen Dirkwinkel
@ 2026-10-22 9:38 ` Steffen Dirkwinkel
2026-10-23 9:39 ` sashiko-bot
2026-10-22 9:38 ` [PATCH RFC net 3/6] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms Steffen Dirkwinkel
` (4 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, stable
From: Christian Taedcke <christian.taedcke@weidmueller.com>
gem_shuffle_tx_one_ring() rotates the software TX ring so that the
tail sits at index 0 and resets queue->tx_tail to 0, but it never
reprograms the hardware transmit buffer queue pointer (TBQP). Other
paths that reset tx_tail to the ring base (macb_init_buffers() and
macb_tx_error_task()) also reprogram TBQP to queue->tx_ring_dma; this
path does not, leaving TBQP pointing at a stale descriptor.
gem_shuffle_tx_rings() runs on every link-up from
macb_mac_link_up(). After a few link up/down flaps that leave
un-completed descriptors in the ring, the stale TBQP keeps pointing at
a descriptor whose used bit is set. When TX is re-enabled on link-up,
the GEM reads that used descriptor and raises TXUBR. macb_interrupt()
schedules the TX NAPI, macb_tx_poll() makes no progress (work_done ==
0) and macb_tx_restart() re-issues TSTART, which makes the controller
read the same used descriptor again and re-assert TXUBR. As the MAC
interrupt is level-triggered, it never deasserts and one CPU is pegged
at 100% in the threaded handler, eventually triggering "sched: RT
throttling activated" and a dead network interface.
Fix it by reprogramming TBQP to the ring base on every path of
gem_shuffle_tx_one_ring() that resets tx_tail to 0, mirroring
macb_tx_error_task(). The early return for an already-aligned tail is
left untouched as TBQP is already consistent there. This is safe
because the shuffle runs from macb_mac_link_up() while TE is still
disabled, so the transmitter is halted.
Fixes: 881a0263d502 ("net: macb: Shuffle the tx ring before enabling tx")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
---
drivers/net/ethernet/cadence/macb_main.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 4b5037867537..97494933eb1e 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -823,7 +823,7 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
if (!count) {
queue->tx_head = 0;
queue->tx_tail = 0;
- goto unlock;
+ goto reset_hw_ptr;
}
shift = tail % ring_size;
@@ -872,6 +872,13 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
/* Make descriptor updates visible to hardware */
wmb();
+reset_hw_ptr:
+ /* tx_tail was reset to the ring base, so TBQP must be reprogrammed
+ * to match; otherwise it keeps pointing at a stale descriptor. Safe
+ * to write directly here as TX is still disabled (called from
+ * macb_mac_link_up() before TE is set).
+ */
+ queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
unlock:
spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
}
--
2.56.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC net 3/6] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 1/6] net: macb: take bp->lock around NCR read-modify-writes Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up Steffen Dirkwinkel
@ 2026-10-22 9:38 ` Steffen Dirkwinkel
2026-10-23 9:39 ` sashiko-bot
2026-10-22 9:38 ` [PATCH RFC net 4/6] net: macb: rotate unreaped TX frames behind the ring base on link-up Steffen Dirkwinkel
` (3 subsequent siblings)
6 siblings, 1 reply; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, stable
From: Christian Taedcke <christian.taedcke@weidmueller.com>
macb_interrupt() defers TX completion handling to NAPI, but when it
schedules the poll it only masks TCOMP, even though TXUBR is enabled
alongside it (both are part of MACB_TX_INT_FLAGS). macb_tx_poll() is
asymmetric in the same way and only re-enables TCOMP. TXUBR is thus
left unmasked while responsibility for handling it has been deferred
to NAPI.
Unlike an edge event, TXUBR is a persistent condition: the controller
keeps it asserted for as long as the transmitter reads a buffer
descriptor whose used bit is set. Leaving a level-triggered source
enabled while NAPI owns its processing means the interrupt refires
immediately after the handler returns, before the poll has had a
chance to clear the underlying condition. This turns into a hard
interrupt storm that pegs a CPU in the (threaded) MAC IRQ handler and,
on PREEMPT_RT, triggers RT throttling ("sched: RT throttling
activated"), taking the network interface down.
Several situations can keep the used-bit read asserted across a poll -
for example unreaped completed descriptors still sitting at tx_tail,
or a transmit restart racing with macb_start_xmit(). The specific
trigger does not matter: as long as the source stays unmasked, any
persistent assertion is enough to storm, so the interrupt handling
itself must be made self-limiting.
Mask TXUBR together with TCOMP in the IDR write when scheduling the TX
NAPI, and re-enable both from the napi_complete path in
macb_tx_poll(), making the TX interrupt mask/unmask symmetric and
consistent with how the driver already treats every other
NAPI-serviced source. The pending TXUBR is still recorded in
queue->txubr_pending before masking and acted on by macb_tx_restart(),
so no event is lost. A persistent TXUBR now degrades to NAPI-paced
polling instead of a CPU-pegging hard interrupt storm.
Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com>
---
drivers/net/ethernet/cadence/macb_main.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 97494933eb1e..14c8299cb7f3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1979,7 +1979,7 @@ static int macb_tx_poll(struct napi_struct *napi, int budget)
(unsigned int)(queue - bp->queues), work_done, budget);
if (work_done < budget && napi_complete_done(napi, work_done)) {
- queue_writel(queue, IER, MACB_BIT(TCOMP));
+ queue_writel(queue, IER, MACB_BIT(TCOMP) | MACB_BIT(TXUBR));
/* Packet completions only seem to propagate to raise
* interrupts when interrupts are enabled at the time, so if
@@ -2174,7 +2174,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
if (status & (MACB_BIT(TCOMP) |
MACB_BIT(TXUBR))) {
- queue_writel(queue, IDR, MACB_BIT(TCOMP));
+ queue_writel(queue, IDR, MACB_BIT(TCOMP) |
+ MACB_BIT(TXUBR));
macb_queue_isr_clear(bp, queue, MACB_BIT(TCOMP) |
MACB_BIT(TXUBR));
if (status & MACB_BIT(TXUBR)) {
--
2.56.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC net 4/6] net: macb: rotate unreaped TX frames behind the ring base on link-up
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
` (2 preceding siblings ...)
2026-10-22 9:38 ` [PATCH RFC net 3/6] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms Steffen Dirkwinkel
@ 2026-10-22 9:38 ` Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 5/6] net: macb: kick the transmitter " Steffen Dirkwinkel
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, Steffen Dirkwinkel, stable
From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
Start the link-up rotation in gem_shuffle_tx_one_ring() at the first
frame that still has to be sent instead of at tx_tail, and schedule the
TX NAPI poll once transmit is enabled again.
macb_mac_link_down() masks TCOMP before disabling transmit. A frame
completed in that window keeps the used bit on its first descriptor and
is never reaped. The rotation moves it to index 0, where the controller
starts after link-up, reads the used bit and stops; macb_tx_restart()
then keeps writing TSTART onto the same descriptor, an interrupt storm
that on PREEMPT_RT ends in RCU stalls.
Keeping the completed frames behind the ring base leaves tx_tail on
them for NAPI to reap. Reaping in link-down instead would have to wait
for NAPI there, which deadlocks against macb_close() and the PM
callbacks disabling it.
Fixes: 881a0263d502 ("net: macb: Shuffle the tx ring before enabling tx")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
---
drivers/net/ethernet/cadence/macb_main.c | 43 +++++++++++++++++++++++---------
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 14c8299cb7f3..6e7ead23d0b7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -798,7 +798,7 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
/* Use juggling algorithm to left rotate tx ring and tx skb array */
static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
{
- unsigned int head, tail, count, ring_size, desc_size;
+ unsigned int head, tail, count, ring_size, desc_size, start, done;
struct macb_tx_skb tx_skb, *skb_curr, *skb_next;
struct macb_dma_desc *desc_curr, *desc_next;
unsigned int i, cycles, shift, curr, next;
@@ -817,8 +817,19 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
ring_size = bp->tx_ring_size;
count = CIRC_CNT(head, tail, ring_size);
- if (!(tail % ring_size))
- goto unlock;
+ /* Frames completed while link-down had TCOMP masked are still
+ * unreaped and their used bit would stop the controller at the ring
+ * base, so rotate the first frame still to be sent there instead.
+ */
+ rmb();
+ for (start = tail; start != head &&
+ (macb_tx_desc(queue, start)->ctrl & MACB_BIT(TX_USED)); start++)
+ while (start + 1 != head && !macb_tx_skb(queue, start)->skb)
+ start++;
+ done = start - tail;
+
+ if (!(start % ring_size))
+ goto reset_hw_ptr;
if (!count) {
queue->tx_head = 0;
@@ -826,7 +837,7 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
goto reset_hw_ptr;
}
- shift = tail % ring_size;
+ shift = start % ring_size;
cycles = gcd(ring_size, shift);
for (i = 0; i < cycles; i++) {
@@ -866,24 +877,23 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
sizeof(struct macb_tx_skb));
}
- queue->tx_head = count;
- queue->tx_tail = 0;
+ queue->tx_tail = done ? ring_size - done : 0;
+ queue->tx_head = queue->tx_tail + count;
/* Make descriptor updates visible to hardware */
wmb();
reset_hw_ptr:
- /* tx_tail was reset to the ring base, so TBQP must be reprogrammed
- * to match; otherwise it keeps pointing at a stale descriptor. Safe
- * to write directly here as TX is still disabled (called from
- * macb_mac_link_up() before TE is set).
+ /* The first frame to be sent is now at the ring base, so TBQP must
+ * be reprogrammed to match; otherwise it keeps pointing at a stale
+ * descriptor. Safe to write directly here as TX is still disabled
+ * (called from macb_mac_link_up() before TE is set).
*/
queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
-unlock:
spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
}
-/* Rotate the queue so that the tail is at index 0 */
+/* Rotate the queue so that the first frame to be sent is at index 0 */
static void gem_shuffle_tx_rings(struct macb *bp)
{
struct macb_queue *queue;
@@ -958,6 +968,15 @@ static void macb_mac_link_up(struct phylink_config *config,
macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
spin_unlock_irqrestore(&bp->lock, flags);
+ if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
+ /* Reap the frames completed while link-down had TCOMP masked */
+ local_bh_disable();
+ for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+ if (READ_ONCE(queue->tx_tail) != READ_ONCE(queue->tx_head))
+ napi_schedule(&queue->napi_tx);
+ local_bh_enable();
+ }
+
netif_tx_wake_all_queues(netdev);
}
--
2.56.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC net 5/6] net: macb: kick the transmitter on link-up
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
` (3 preceding siblings ...)
2026-10-22 9:38 ` [PATCH RFC net 4/6] net: macb: rotate unreaped TX frames behind the ring base on link-up Steffen Dirkwinkel
@ 2026-10-22 9:38 ` Steffen Dirkwinkel
2026-10-23 9:39 ` sashiko-bot
2026-10-22 9:38 ` [PATCH RFC net 6/6] net: macb: keep best-effort traffic on TX queue 0 Steffen Dirkwinkel
2026-10-22 9:55 ` [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Nicolai Buchwitz
6 siblings, 1 reply; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, Steffen Dirkwinkel, stable
From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
Write TSTART in macb_mac_link_up() for every queue that still has
frames outstanding once transmit has been re-enabled.
Frames queued before a link-down stay in the ring, and setting NCR.TE
does not start the transmitter; the driver relies on the next
macb_start_xmit() to write TSTART. If BQL stopped the queue before the
link went down, that transmit never comes and the queue stays idle
until the netdev watchdog fires.
Like the rotation, the kick is skipped on the at91rm9200 EMAC, which
uses its own TX path.
Fixes: 881a0263d502 ("net: macb: Shuffle the tx ring before enabling tx")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
---
drivers/net/ethernet/cadence/macb_main.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 6e7ead23d0b7..3dfc7ffdd598 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -903,6 +903,8 @@ static void gem_shuffle_tx_rings(struct macb *bp)
gem_shuffle_tx_one_ring(queue);
}
+static void macb_tx_restart(struct macb_queue *queue);
+
static void macb_mac_link_up(struct phylink_config *config,
struct phy_device *phydev,
unsigned int mode, phy_interface_t interface,
@@ -969,6 +971,10 @@ static void macb_mac_link_up(struct phylink_config *config,
spin_unlock_irqrestore(&bp->lock, flags);
if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
+ /* With BQL holding a queue stopped no xmit would issue TSTART */
+ for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+ macb_tx_restart(queue);
+
/* Reap the frames completed while link-down had TCOMP masked */
local_bh_disable();
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
--
2.56.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC net 6/6] net: macb: keep best-effort traffic on TX queue 0
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
` (4 preceding siblings ...)
2026-10-22 9:38 ` [PATCH RFC net 5/6] net: macb: kick the transmitter " Steffen Dirkwinkel
@ 2026-10-22 9:38 ` Steffen Dirkwinkel
2026-10-22 9:55 ` [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Nicolai Buchwitz
6 siblings, 0 replies; 11+ messages in thread
From: Steffen Dirkwinkel @ 2026-10-22 9:38 UTC (permalink / raw)
To: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King
Cc: netdev, linux-kernel, linux-rt-devel, Christian Taedcke,
Nicolai Buchwitz, Andrea della Porta, Steffen Dirkwinkel
From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
Add an ndo_select_queue() that puts every frame on queue 0 unless
traffic classes are configured (taprio, or software mqprio).
The GEM serves its transmit queues in strict priority order, the
highest queue first, while the stack hashes flows over all of them. On
a saturated link the flows on the lower queues starve until the netdev
watchdog fires on them, and the timeout path cannot help as nothing is
wrong with the queue.
XPS settings, the socket queue cache and the queue choice of
PACKET_QDISC_BYPASS sockets no longer take effect on multi-queue GEMs
without traffic classes; queues 1 and up stay reachable through tc.
Fixes: 02c958dd3446 ("net/macb: add TX multiqueue support for gem")
Assisted-by: LLM
Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
---
drivers/net/ethernet/cadence/macb_main.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 3dfc7ffdd598..eae43490c5f7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4596,10 +4596,21 @@ static void macb_tx_timeout(struct net_device *netdev, unsigned int q)
macb_tx_restart(&bp->queues[q]);
}
+/* GEM serves TX queues in strict priority order, the highest queue first */
+static u16 macb_select_queue(struct net_device *netdev, struct sk_buff *skb,
+ struct net_device *sb_dev)
+{
+ if (netdev_get_num_tc(netdev))
+ return netdev_pick_tx(netdev, skb, sb_dev);
+
+ return 0;
+}
+
static const struct net_device_ops macb_netdev_ops = {
.ndo_open = macb_open,
.ndo_stop = macb_close,
.ndo_start_xmit = macb_start_xmit,
+ .ndo_select_queue = macb_select_queue,
.ndo_set_rx_mode = macb_set_rx_mode,
.ndo_get_stats64 = macb_get_stats,
.ndo_eth_ioctl = macb_ioctl,
--
2.56.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
` (5 preceding siblings ...)
2026-10-22 9:38 ` [PATCH RFC net 6/6] net: macb: keep best-effort traffic on TX queue 0 Steffen Dirkwinkel
@ 2026-10-22 9:55 ` Nicolai Buchwitz
6 siblings, 0 replies; 11+ messages in thread
From: Nicolai Buchwitz @ 2026-10-22 9:55 UTC (permalink / raw)
To: Steffen Dirkwinkel
Cc: Théo Lebrun, Conor Dooley, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Kevin Hao, Sebastian Andrzej Siewior, Russell King, netdev,
linux-kernel, linux-rt-devel, Christian Taedcke,
Andrea della Porta, stable, Steffen Dirkwinkel
Hi Steffen
On 22.10.2026 11:38, Steffen Dirkwinkel wrote:
> Hello Théo, Christian, all,
>
> I've been looking at RCU and TX stalls on macb devices (Xilinx ZynqMP
> on
> Beckhoff CX9240) for a while and have been carrying downstream patches
> similar to Christian's. A while ago I let Claude try to reproduce this
> on my
> devices, it was able to reproduce and found some fixes but I wasn't
> happy with the quality of the patches and my understanding of them. I
> briefly talked to Théo in Prague and he suggested I send what I have as
> either the patches or the reproducers may be useful for ideas.
> I've finally gotten around to it and also ran some more testing and
> improved
> the patches in the meantime. The series includes Christian's patches in
> the
> original form (should also at least be reworded) and I also have Patch
> 1 in here
> since the bench seems to have reproduced the issue on hardware (see
> below).
>
> If anyone has other ideas / patches for these issues I'm also happy to
> run tests on devices.
Thanks for posting this RFC. I will go later through the details, but
wanted to let you
know that I'm also investigating the macb watchdog issue on RPi RP1 [1].
Yesterday Théo also shared some findings in the thread.
Three things from that work which touch your series:
- Patch 3 changes the IER write in macb_tx_poll() that I need a readl
after. The write
is posted, a TCOMP landing between the ring check and the write
reaching the MAC is
lost. That seems to be the RP1 stall, masking TXUBR does not change
it.
The two patches need ordering.
- The watchdog only recovers a lost TSTART. With a missed completion
TBQP == tx_head and
macb_tx_restart() does nothing, the queue never comes back. I have a
patch that
schedules TX NAPI from macb_tx_timeout() when the tail descriptor has
TX_USED set.
- RP1 has a single TX queue, so the starvation from patch 6 is not what
we see.
I can run the series on an RP1 board with my reproducer if that helps.
[1]
https://github.com/raspberrypi/linux/issues/7661#issuecomment-6322904343
> [...]
Thanks,
Nicolai
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up
2026-10-22 9:38 ` [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up Steffen Dirkwinkel
@ 2026-10-23 9:39 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-23 9:39 UTC (permalink / raw)
To: Steffen Dirkwinkel; +Cc: linux-rt-devel, linux-kernel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: macb: missing TX_USED bit on desc 0 in gem_shuffle_tx_one_ring()
--
commit ded0ab42c83afe9d93d9bc6c0ab9623a1dc038f5
Author: Christian Taedcke <christian.taedcke@weidmueller.com>
net: macb: reprogram TBQP after shuffling the TX ring on link-up
This commit reprograms the hardware transmit buffer queue pointer (TBQP) to
queue->tx_ring_dma in gem_shuffle_tx_one_ring() whenever tx_tail is reset to
0 during link-up, preventing TBQP from pointing at a stale descriptor.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4b50378675378..97494933eb1ee 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -823,7 +823,7 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
> if (!count) {
> queue->tx_head = 0;
> queue->tx_tail = 0;
> - goto unlock;
> + goto reset_hw_ptr;
> }
>
> shift = tail % ring_size;
> @@ -872,6 +872,13 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
> /* Make descriptor updates visible to hardware */
> wmb();
>
> +reset_hw_ptr:
> + /* tx_tail was reset to the ring base, so TBQP must be reprogrammed
> + * to match; otherwise it keeps pointing at a stale descriptor. Safe
> + * to write directly here as TX is still disabled (called from
> + * macb_mac_link_up() before TE is set).
> + */
> + queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
[Severity: High]
When count is 0 and gem_shuffle_tx_one_ring() in
drivers/net/ethernet/cadence/macb_main.c jumps from the !count branch to
reset_hw_ptr without rotating the ring, is descriptor 0 guaranteed to have
MACB_BIT(TX_USED) set before TBQP is reprogrammed to queue->tx_ring_dma?
When a multi-descriptor frame (such as scatter-gather or segmentation
offload) wraps around the end of tx_ring so that descriptor 0 is a non-first
descriptor of that frame, macb_tx_map() clears MACB_BIT(TX_USED) on all
descriptors of the frame and sets MACB_BIT(TX_USED) on the sentinel
descriptor at tx_head. On frame completion, the hardware only sets
MACB_BIT(TX_USED) on the first descriptor of the frame, and
macb_tx_complete() unmaps the buffers without setting MACB_BIT(TX_USED) on
the remaining descriptors:
drivers/net/ethernet/cadence/macb_main.c:macb_tx_complete() {
...
/* TX_USED bit is only set by hardware on the very first buffer
* descriptor of the transmitted frame.
*/
if (!(ctrl & MACB_BIT(TX_USED)))
break;
...
}
If the queue subsequently drains (count == 0) at a non-zero ring index
(tail % ring_size != 0) before link-up, the TX_USED sentinel sits at
desc[tail % ring_size] while desc[0]->ctrl still has TX_USED cleared and
desc[0]->addr holds an unmapped dma address.
Jumping to reset_hw_ptr points TBQP at desc[0] with TX_USED == 0. If
MACB_BIT(TSTART) is then written to NCR (for example when another queue
transmits in macb_start_xmit() or when TSTART is kicked on link-up) before a
new frame is mapped on this queue, can the controller read the stale unmapped
address in desc[0]->addr and advance TBQP past index 0 while tx_head and
tx_tail remain 0?
In comparison, macb_tx_error_task() marks descriptor 0 as used and issues a
write barrier before writing TBQP:
drivers/net/ethernet/cadence/macb_main.c:macb_tx_error_task() {
...
/* Set end of TX queue */
desc = macb_tx_desc(queue, 0);
macb_set_addr(bp, desc, 0);
desc->ctrl = MACB_BIT(TX_USED);
/* Make descriptor updates visible to hardware */
wmb();
/* Reinitialize the TX desc queue */
queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
...
}
Should the !count path also set MACB_BIT(TX_USED) on descriptor 0 and call
wmb() before reprogramming TBQP?
> unlock:
> spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-std-macb-v14-send-v1-0-9187bc545bf6@beckhoff.com?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC net 3/6] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms
2026-10-22 9:38 ` [PATCH RFC net 3/6] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms Steffen Dirkwinkel
@ 2026-10-23 9:39 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-23 9:39 UTC (permalink / raw)
To: Steffen Dirkwinkel; +Cc: linux-rt-devel, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: macb: lost TXUBR event while masked in macb_tx_poll() stalls TX queue
- [Medium] net: macb: TXUBR left unmasked when macb_tx_poll() reschedules NAPI
--
commit a3dccad77456cfe6ddb20424831a548f63c5269b
Author: Christian Taedcke <christian.taedcke@weidmueller.com>
net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms
This commit updates macb_interrupt() to mask MACB_BIT(TXUBR) alongside
MACB_BIT(TCOMP) when scheduling the TX NAPI poll, and updates macb_tx_poll()
to re-enable both bits when NAPI completes.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 97494933eb1ee..14c8299cb7f37 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1979,7 +1979,7 @@ static int macb_tx_poll(struct napi_struct *napi, int budget)
> (unsigned int)(queue - bp->queues), work_done, budget);
>
> if (work_done < budget && napi_complete_done(napi, work_done)) {
> - queue_writel(queue, IER, MACB_BIT(TCOMP));
> + queue_writel(queue, IER, MACB_BIT(TCOMP) | MACB_BIT(TXUBR));
>
> /* Packet completions only seem to propagate to raise
> * interrupts when interrupts are enabled at the time, so if
[Severity: Medium]
In drivers/net/ethernet/cadence/macb_main.c, when work_done < budget and
napi_complete_done(napi, work_done) returns true, macb_tx_poll() writes
MACB_BIT(TCOMP) | MACB_BIT(TXUBR) to IER to unmask both TX interrupt sources.
Immediately afterward, macb_tx_poll() checks macb_tx_complete_pending(queue)
to handle packets that completed while interrupts were disabled:
if (macb_tx_complete_pending(queue)) {
queue_writel(queue, IDR, MACB_BIT(TCOMP));
macb_queue_isr_clear(bp, queue, MACB_BIT(TCOMP));
netdev_vdbg(bp->netdev, "TX poll: packets pending, reschedule\n");
napi_schedule(napi);
}
Unlike macb_rx_poll(), which writes bp->rx_intr_mask to IER and then masks
the same bp->rx_intr_mask in IDR when macb_rx_pending(queue) is true:
if (macb_rx_pending(queue)) {
queue_writel(queue, IDR, bp->rx_intr_mask);
macb_queue_isr_clear(bp, queue, MACB_BIT(RCOMP));
netdev_vdbg(bp->netdev, "poll: packets pending, reschedule\n");
napi_schedule(napi);
}
should this reschedule block in macb_tx_poll() also write
MACB_BIT(TCOMP) | MACB_BIT(TXUBR) to IDR and macb_queue_isr_clear() so that
MACB_BIT(TXUBR) is not left unmasked while napi_tx is rescheduled and polling?
> @@ -2174,7 +2174,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>
> if (status & (MACB_BIT(TCOMP) |
> MACB_BIT(TXUBR))) {
> - queue_writel(queue, IDR, MACB_BIT(TCOMP));
> + queue_writel(queue, IDR, MACB_BIT(TCOMP) |
> + MACB_BIT(TXUBR));
> macb_queue_isr_clear(bp, queue, MACB_BIT(TCOMP) |
> MACB_BIT(TXUBR));
> if (status & MACB_BIT(TXUBR)) {
[Severity: High]
Can masking MACB_BIT(TXUBR) in macb_interrupt() cause a TXUBR halt event to
be lost during NAPI polling, stalling the TX queue until macb_tx_timeout()
fires?
In drivers/net/ethernet/cadence/macb_main.c, when macb_interrupt() handles a
normal TX completion where status only has MACB_BIT(TCOMP) set, it now writes
MACB_BIT(TCOMP) | MACB_BIT(TXUBR) to IDR, masking TXUBR while
queue->txubr_pending remains false, and schedules queue->napi_tx. Similarly,
in macb_tx_poll(), queue->txubr_pending is cleared to false before calling
macb_tx_restart() while TXUBR remains masked until napi_complete_done() (or
across multiple polls when work_done == budget):
rmb(); // ensure txubr_pending is up to date
if (queue->txubr_pending) {
queue->txubr_pending = false;
netdev_vdbg(bp->netdev, "poll: tx restart\n");
macb_tx_restart(queue);
}
When the transmitter reads a descriptor with MACB_BIT(TX_USED) set, it halts.
As noted in the comments in macb_tx_poll() and macb_rx_poll() (and commits
0bf476fc3624 and 1900e30d0ef7), events on this controller only propagate to
raise interrupts if the corresponding interrupt bit is enabled at the moment
the event occurs; re-enabling the bit in IER later does not raise an
interrupt.
If the race between macb_start_xmit() updating descriptors and the controller
reading MACB_BIT(TX_USED) (commit 4298388574da) happens while MACB_BIT(TXUBR)
is masked, the transmitter halts at the unsent descriptor (with TBQP !=
tx_head) without latching MACB_BIT(TXUBR) in ISR, so queue->txubr_pending
stays false.
In macb_tx_poll(), macb_tx_complete() then reaps completed frames and stops at
the unsent frame at queue->tx_tail (where MACB_BIT(TX_USED) is 0), and skips
macb_tx_restart() because queue->txubr_pending is false. When
napi_complete_done() succeeds and macb_tx_poll() writes
MACB_BIT(TCOMP) | MACB_BIT(TXUBR) to IER, no interrupt is raised for the
earlier TXUBR halt, and macb_tx_complete_pending(queue) returns false because
macb_tx_desc(queue, queue->tx_tail)->ctrl still has MACB_BIT(TX_USED) clear:
if (queue->tx_head != queue->tx_tail) {
/* Make hw descriptor updates visible to CPU */
rmb();
if (macb_tx_desc(queue, queue->tx_tail)->ctrl & MACB_BIT(TX_USED))
retval = true;
}
Could NAPI then complete without calling macb_tx_restart(), leaving the
transmitter halted with unsent frames in the ring until the netdev watchdog
times out?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-std-macb-v14-send-v1-0-9187bc545bf6@beckhoff.com?part=3
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH RFC net 5/6] net: macb: kick the transmitter on link-up
2026-10-22 9:38 ` [PATCH RFC net 5/6] net: macb: kick the transmitter " Steffen Dirkwinkel
@ 2026-10-23 9:39 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-23 9:39 UTC (permalink / raw)
To: Steffen Dirkwinkel; +Cc: linux-rt-devel, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] net: macb: DMA of freed skb in macb_mac_link_up() when empty ring resets TBQP
- [High] net: macb: unbased TBQP index in macb_tx_restart() breaks link-up kick
--
commit 19c83c729b5e6d7a587a6b6ecc879a9a247907de
Author: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
net: macb: kick the transmitter on link-up
This commit calls macb_tx_restart() for each queue in macb_mac_link_up()
after enabling transmit so that pending frames in the ring are transmitted
even if byte queue limits previously stopped the queue.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6e7ead23d0b74..3dfc7ffdd5985 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -969,6 +971,10 @@ static void macb_mac_link_up(struct phylink_config *config,
> spin_unlock_irqrestore(&bp->lock, flags);
>
> if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
> + /* With BQL holding a queue stopped no xmit would issue TSTART */
> + for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> + macb_tx_restart(queue);
[Severity: Critical]
Can kicking the transmitter here in macb_mac_link_up() cause the controller
to transmit an already unmapped and freed skb buffer on an empty queue?
When a multi-descriptor frame wraps across the end of the ring,
macb_tx_map() clears the used bit on all descriptors of the frame and
places the sentinel used bit at the new queue->tx_head. The controller only
sets the used bit on the first descriptor of a transmitted frame, and
macb_tx_complete() unmaps the buffers via macb_tx_unmap() and frees the skb
without setting the used bit on the remaining descriptors. As a result,
descriptor 0 can retain a cleared used bit and a stale buffer address while
the sentinel sits at start % ring_size != 0.
When that queue is empty (count == 0) with start % ring_size != 0 during
link-up, macb_mac_link_up() -> gem_shuffle_tx_rings() ->
gem_shuffle_tx_one_ring() skips the ring rotation and reprograms hardware
to descriptor 0:
drivers/net/ethernet/cadence/macb_main.c:gem_shuffle_tx_one_ring() {
if (!(start % ring_size))
goto reset_hw_ptr;
if (!count) {
queue->tx_head = 0;
queue->tx_tail = 0;
goto reset_hw_ptr;
}
...
reset_hw_ptr:
queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
}
Unlike macb_tx_error_task(), which explicitly marks descriptor 0 as used
before resetting the hardware pointer:
drivers/net/ethernet/cadence/macb_main.c:macb_tx_error_task() {
/* Set end of TX queue */
desc = macb_tx_desc(queue, 0);
macb_set_addr(bp, desc, 0);
desc->ctrl = MACB_BIT(TX_USED);
}
gem_shuffle_tx_one_ring() leaves descriptor 0 with its stale control word.
When macb_tx_restart() then sets MACB_BIT(TSTART) in the global NCR
register because another queue has pending frames, won't the controller
start reading from descriptor 0 on the empty queue, transmit the freed
buffer onto the network, and advance the hardware pointer past index 0
while queue->tx_head and queue->tx_tail remain at 0?
> +
> /* Reap the frames completed while link-down had TCOMP masked */
> local_bh_disable();
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
[Severity: High]
Does macb_tx_restart() compute the descriptor index from queue_readl(queue,
TBQP) without subtracting the ring base address, causing missed kicks or
spurious restarts on link-up?
Earlier in macb_mac_link_up(), gem_shuffle_tx_one_ring() rotates the ring
so that done completed frames sit at [ring_size - done, ring_size), unsent
frames sit at [0, count - done), and the hardware pointer is reset to the
ring base:
drivers/net/ethernet/cadence/macb_main.c:gem_shuffle_tx_one_ring() {
queue->tx_tail = done ? ring_size - done : 0;
queue->tx_head = queue->tx_tail + count;
/* Make descriptor updates visible to hardware */
wmb();
reset_hw_ptr:
queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
}
Then macb_mac_link_up() calls macb_tx_restart(queue) before scheduling
queue->napi_tx to reap those completed frames:
drivers/net/ethernet/cadence/macb_main.c:macb_tx_restart() {
if (queue->tx_head == queue->tx_tail)
goto out_tx_ptr_unlock;
tbqp = queue_readl(queue, TBQP) / macb_dma_desc_get_size(bp);
tbqp = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, tbqp));
head_idx = macb_adj_dma_desc_idx(bp, macb_tx_ring_wrap(bp, queue->tx_head));
if (tbqp == head_idx)
goto out_tx_ptr_unlock;
}
Because queue_readl(queue, TBQP) returns the raw 32-bit address
lower_32_bits(queue->tx_ring_dma) rather than a byte offset from the start
of the ring, dividing it directly by macb_dma_desc_get_size(bp) only wraps
to index 0 if lower_32_bits(queue->tx_ring_dma) is an exact multiple of
bp->tx_ring_size * macb_dma_desc_get_size(bp).
In macb_alloc(), queue->tx_ring_dma is not aligned to that product in
several configurations:
drivers/net/ethernet/cadence/macb_main.c:macb_alloc() {
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
queue->tx_ring = tx + macb_tx_ring_size_per_queue(bp) * q;
queue->tx_ring_dma = tx_dma + macb_tx_ring_size_per_queue(bp) * q;
}
Specifically:
- On multi-queue controllers with descriptor prefetch,
macb_tx_ring_size_per_queue(bp) adds bp->tx_bd_rd_prefetch bytes per
queue, shifting queue->tx_ring_dma for queues >= 1.
- When 64-bit addressing and ptp timestamping are both enabled,
macb_dma_desc_get_size(bp) is 24 bytes, which does not evenly divide a
page-aligned base address even on queue 0.
- When bp->tx_ring_size * macb_dma_desc_get_size(bp) exceeds page size and
dma_alloc_coherent() returns an address aligned only to page size.
When tbqp at the ring base wraps to a non-zero base_idx instead of 0:
- If count - done == base_idx unsent frames are waiting at [0, base_idx),
head_idx equals tbqp, so macb_tx_restart() skips writing MACB_BIT(TSTART)
and leaves the stopped queue stuck.
- If all queued frames completed while link-down masked completion
interrupts (done == count > 0), queue->tx_head != queue->tx_tail is still
true before napi_tx runs, and head_idx is 0 while tbqp is non-zero, so
macb_tx_restart() writes MACB_BIT(TSTART) onto a used descriptor at index
0 and triggers a spurious used-bit-read interrupt.
Should macb_tx_restart() subtract lower_32_bits(queue->tx_ring_dma) from
queue_readl(queue, TBQP) before dividing by macb_dma_desc_get_size(bp)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-std-macb-v14-send-v1-0-9187bc545bf6@beckhoff.com?part=5
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-10-23 9:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-22 9:38 [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 1/6] net: macb: take bp->lock around NCR read-modify-writes Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 2/6] net: macb: reprogram TBQP after shuffling the TX ring on link-up Steffen Dirkwinkel
2026-10-23 9:39 ` sashiko-bot
2026-10-22 9:38 ` [PATCH RFC net 3/6] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms Steffen Dirkwinkel
2026-10-23 9:39 ` sashiko-bot
2026-10-22 9:38 ` [PATCH RFC net 4/6] net: macb: rotate unreaped TX frames behind the ring base on link-up Steffen Dirkwinkel
2026-10-22 9:38 ` [PATCH RFC net 5/6] net: macb: kick the transmitter " Steffen Dirkwinkel
2026-10-23 9:39 ` sashiko-bot
2026-10-22 9:38 ` [PATCH RFC net 6/6] net: macb: keep best-effort traffic on TX queue 0 Steffen Dirkwinkel
2026-10-22 9:55 ` [PATCH RFC net 0/6] net: macb: fix TX stalls, IRQ storms and a lost NCR update on link-up Nicolai Buchwitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®