Re: [PATCH net 2/2] 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:
- [Medium] Incomplete interrupt masking in the NAPI reschedule path leaves TXUBR unmasked, causing unnecessary hard interrupts.
- [Medium] Ad-hoc cross-CPU synchronization flag with mismatched memory barriers causes a data race.
--

commit 8cade27ad39e331dd72cf96ffc689e581ab23335
Author: Christian Taedcke <christian.taedcke@xxxxxxxxxxxxxxx>

net: macb: mask TXUBR during TX NAPI poll to prevent IRQ storms

This commit attempts to fix an interrupt storm caused by TXUBR remaining
unmasked while NAPI handles completions. It masks TXUBR alongside TCOMP
during NAPI scheduling and uses a flag to track pending TXUBR events.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> --- 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

[Severity: Medium]
In the reschedule path just below this change, does TXUBR also need to be
masked?

When packets are still pending, the code reschedules NAPI and masks TCOMP:

	if (macb_tx_complete_pending(queue)) {
		queue_writel(queue, IDR, MACB_BIT(TCOMP));
		macb_queue_isr_clear(bp, queue, MACB_BIT(TCOMP));
		...
		napi_schedule(napi);
	}

Since IER just unmasked both TCOMP and TXUBR in the lines above, failing to
mask TXUBR here leaves it unmasked while NAPI takes over. Under heavy TX load,
does this cause unnecessary hard interrupts?

[ ... ]

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

[Severity: Medium]
Is this use of a plain boolean and memory barriers sufficient for cross-CPU
synchronization here?

The commit message notes that pending TXUBR is recorded in
queue->txubr_pending, and the interrupt handler updates it like this:

		if (status & MACB_BIT(TXUBR)) {
			queue->txubr_pending = true;
			wmb(); // ensure softirq can see update
		}

Concurrently, macb_tx_poll() locklessly reads and clears it:

	rmb(); // ensure txubr_pending is up to date
	if (queue->txubr_pending) {
		queue->txubr_pending = false;

Using wmb() and rmb() enforces instruction ordering but does not flush
caches, which creates a data race on the plain boolean under the kernel
memory model. Could this lead to undefined behavior or lost wakeups if the
compiler caches or reorders the flag?

Could this be replaced with atomic bit operations like set_bit() and
test_and_clear_bit() instead to safely synchronize between the hardirq
and softirq contexts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-upstreaming-macb-irq-storm-v1-0-ab3115b5a13a@xxxxxxxxxxxxxxx?part=2