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

[sashiko-bot]

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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxx?part=3