Re: [PATCH net 2/2] net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms

Hello Christian,

My biggest gripe with this patch is the commit message. It's a massive
block of text which goes here and there without enough structure.

Let me try to pick issues I have with it and try to give a better
alternative (hoping I understood the topic clearly enough).

On Mon Jul 6, 2026 at 4:02 PM CEST, Christian Taedcke via B4 Relay wrote:
> From: Christian Taedcke <[email protected]>
>
> 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.

Yes, so said differently:

   TXUBR is acknowledged by the NAPI poll function. The IRQ signal is
   active until then but signal is left unmasked. Do as with TCOMP and
   mask the signal until its acknowledgment.

This sounds much more straight forward to me. And it also explains what
we do to solve the issue, which is info we expect to find in the first
commit message paragraph. The rest is to go into details onto the
specifics, the decisionmaking, etc.

> 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.

This whole paragraph is somewhat moot to me. It highlights level versus
edge interrupts but even with edge events the bug is present: it blocks
any other IRQ from the HW until the TXUBR ACK (in NAPI context).

Describing how the bug surfaces on your HW is interesting however.

> 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.

But this is unrelated? Whether TXUBR stays asserted across the poll
processing doesn't change the fact the IRQ is unmasked until we reach
NAPI context and the signal is still not ACKed.

> 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.

Again a lot of words for little info. Usually the patch change
(the "mask TXUBR with TCOMP" part) belongs to the first commit message
paragraph, we shouldn't have to wait until the last paragraph to know
about what a patch does.

Some question that is left in my head after reading, that could have
been answered by your commit message: your approach of masking TXUBR
until NAPI is one way, another would have been to ACK TXUBR from
macb_interrupt(). Have you investigated that approach? The answer might
be super straight forward.

> Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
> Cc: [email protected]
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Christian Taedcke <[email protected]>
> ---
>  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 b11cb8f068b7..f75cf2ffdf6f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1971,7 +1971,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
> @@ -2161,7 +2161,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)) {

We risk some race condition here, but that was present before your
patch. macb_interrupt() already grabs bp->lock but macb_tx_poll() does
zero efforts. (I'm not saying that macb_tx_poll() should grab bp->lock
which would throw away the benefits of our separate queues, the solution
is more complex.)

Sashiko reports that as well.

It's not up to you to fix this. Or if you do it, it should be a separate
patch. It is weird that our IRQ handlers all grab the interface-global
bp->lock rather than a queue-specific primitive, it would probably be a
big rework.

--

TLDR: if patch 2/2 fixes your bug, I'm OK with it. Please rewrite your
commit message though. I suspect patch 1/2 is not needed.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com