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

[Théo Lebrun]

Replying to sashiko
https://sashiko.dev/#/patchset/20260805-macb-context-v8-0-bc302ffd1174%40bootlin.com

> commit 2dad125ac3428043e94f084f940a4694a6830209
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: read ISR inside bp->lock critical section
>
> This commit modifies the macb_interrupt handler to acquire the bp->lock
> spinlock before reading the ISR register. If the ISR is empty, it unlocks
> and returns IRQ_NONE, rather than doing an unprotected early read.
>
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -2193,12 +2193,13 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> >  	bool hresp_err = false;
> >  	u32 status;
> >  
> > -	status = queue_readl(queue, ISR);
> > +	spin_lock(&bp->lock);
>
> [Severity: High]
> Could this unconditional lock acquisition cause severe latency or lockups
> for shared interrupts?
>
> The IRQ line is registered with IRQF_SHARED. If a shared device triggers
> an interrupt while bp->lock is held elsewhere, the CPU handling it will
> spin in hardirq context with IRQs disabled just to return IRQ_NONE.
>
> Looking at macb_tx_error_task(), it acquires bp->lock and holds it while
> calling macb_halt_tx(), which can poll for up to 14ms. If a shared interrupt
> fires during this window, won't it cause severe system latency?

Exactly same review as last iteration. See my reply here:
https://lore.kernel.org/all/DKH4YK5JVVNB.1XC12P899ZJOP@xxxxxxxxxxx/

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