Re: [PATCH net 1/2] net: macb: reprogram TBQP after shuffling the TX ring on link-up

On 2026-07-07 15:36:24 [+0200], Taedcke, Christian wrote:
> Thank you for the quick review! This is my first Linux kernel
> contribution, so I appreciate your feedback here.

You are doing good.

> On 7/6/2026 5:04 PM, Sebastian Andrzej Siewior wrote:
> > On 2026-07-06 16:02:14 [+0200], Christian Taedcke via B4 Relay wrote:
> >> From: Christian Taedcke <[email protected]>
> >>
> >> 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.
> > 
> > But this should also happen with !RT at which point the interrupt runs
> > at 100% CPU and the softirq has hardly an chance to make progress, no?
> 
> Problably yes. I had issues reproducing the issue since it appeared only
> on specific test setups when a lot packets where sent to another network
> device and this device's power was cut. And even then on some test runs
> the issue was not visible after a few hundred iterations. But after a
> restart of the whole test setup (including cold reboot of all devices)
> the issue sometimes appeared after 5 iterations.
> I only metion RT here because it was the only thing i tested. I only ran
> the RT kernel.
> Should I change the description?

It makes a difference if the problem you are facing is limited to
PREEMPT_RT (and so does not trigger on !PREEMPT_RT due to $REASON),
or also effects !PREEMPT_RT but may or may not trigger easily on
PREEMPT_RT.

> >> 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")
> > 
> > This is v7.0-rc4. So that RT tree of yours has some backports or did you
> > run into this while trying to reproduce it upstream?
> 
> There were some backports. I ran this on the linux-yocto kernel
> https://git.yoctoproject.org/linux-yocto branch
> v6.6/standard/preempt-rt/base.
> The "Fixes:" commit was backported as 0a47c3889fcd before their version
> of 6.6.130.
> 
> The kernel i reproduced the issue on was linux-yocto branch
> v6.6/standard/preempt-rt/base after 6.6.142 was merged into it.

It is usually good to reproduce the issue on vanilla ensuring that the
problem was not introduced by a backport or was solved differently
upstream.

> >> Cc: [email protected]
> >> Assisted-by: Claude:claude-opus-4-8
> >> Signed-off-by: Christian Taedcke <[email protected]>
> >> ---
> >>  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 fd282a1700fb..b11cb8f068b7 100644
> >> --- a/drivers/net/ethernet/cadence/macb_main.c
> >> +++ b/drivers/net/ethernet/cadence/macb_main.c
> >> @@ -820,7 +820,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;
> > 
> > This update is even needed for count == 0 case? I kind of do understand
> > that you need to updated if you shuffled the descriptors around.
> 
> This was my understanding before researching more because of the email
> from Kevin in this thread: count == 0 may happen anywhere within the ring
> (e.g. when both the tail and the head point to the middle).
> Resetting queue->tx_tail to 0 but not resetting TBQP results in them
> being out-of-sync.
> But as Kevin mentioned in his email TBQP is reset to the original
> value when transmit is disabled (by setting bit 3 in NCR register).
> 
> I will investigate this further why my code change fixed the issue for
> me, but according to the documentation in [1] it should be a no-op.

I see.

> [1] https://docs.amd.com/v/u/en-US/ug1085-zynq-ultrascale-trm pg. 1040
> 
> Christian

Sebastian