Re: [PATCH net-next v5 13/15] net: macb: read ISR inside bp->lock critical section

[Théo Lebrun]

Replies to sashiko reviews!
https://sashiko.dev/#/patchset/20260724-macb-context-v5-0-569b1852bc7f%40bootlin.com

sashiko-netdev had nothing to say on this patch.

> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 4c94c23d925a2..c832b6c1b98cd 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -2185,12 +2185,13 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> >  	struct net_device *netdev = bp->netdev;
> >  	u32 status;
> >
> > -	status = queue_readl(queue, ISR);
> > +	spin_lock(&bp->lock);
>
> [Severity: High]
> Does unconditionally acquiring this lock introduce a recursive deadlock with
> netconsole?
>
> If a network error occurs, macb_tx_error_task() acquires bp->lock and calls
> netdev_err(). If netconsole is active, it attempts to transmit the log
> immediately by invoking macb_poll_controller(), which disables local
> interrupts and directly calls macb_interrupt() on the same CPU:
>
> macb_tx_error_task()
>    spin_lock(&bp->lock);
>    netdev_err();
>       ...
>       macb_poll_controller()
>          macb_interrupt()
>             spin_lock(&bp->lock); // self-deadlock
>
> Previously, macb_interrupt() would read the ISR register first and safely
> return IRQ_NONE if status was zero (which is normal during a synthetic
> netpoll invocation), avoiding the lock.

yes, good catch. Let's move the printk() calls out of the atomic
section. This is the only place where it could cause issues, no other
printk() lives in the bp->lock critical section.

Both netdev_err() calls are deferred to after the end of the section.
We drop the netdev_dbg() call. We all prefer small & simple atomic
sections anyway.

> [Severity: High]
> Could acquiring this lock before checking the interrupt status cause latency
> spikes for other devices sharing this IRQ line?
>
> The MACB interrupt is registered using IRQF_SHARED. This means
> macb_interrupt() is invoked when any device sharing the IRQ line raises
> an interrupt. Since bp->lock can be held for up to 14 milliseconds with
> local interrupts disabled (e.g., in macb_tx_error_task() polling
> macb_halt_tx()), other devices might be starved while spinning on this
> lock before even checking if MACB raised the interrupt.

Yes, but we have no other way. Reading the ISR status outside bp->lock
can lead to bugs. Reading it inside can lead to increased latency
spikes. Make it correct first then make it fast.

(A past iteration proposed reading it inside and out. I wasn't
 satisfied and neither were reviewers. We dropped that.)

The 14ms topic is a pre-existing issue with macb_tx_error_task() which
we won't address in this series. I don't know in what case it could
occur. Anyway, sharing your MACB IRQ lane with a critical peripheral is
a theoretical setup that most likely doesn't exist.

Thanks,

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