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

[Théo Lebrun]

Replying to non-net sashiko
https://sashiko.dev/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

> commit 48c8a049fccfd5d7c55b15a7fd08dc4eca66d836
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> Date:   Wed Aug 12 08:03:29 2026 +0000
>
>     net: macb: read ISR inside bp->lock critical section
>     
>     The IRQ handler reads ISR register into the `status` stack variable.
>     If empty, it early returns. Else, it grabs bp->lock and iterates on
>     the status bits.
>     
>     We risk a race on spinlock acquire; status might have changed.
>     Move the readl(ISR) inside the bp->lock critical section.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index d23a0d08c2769..6d79663e42bfe 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -2190,12 +2190,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]
> Does taking this lock unconditionally cause severe latency spikes for other
> devices on a shared interrupt line?
>
> If a shared interrupt fires while the driver's error handling task is running,
> this CPU might spin with local interrupts disabled for up to 14ms:
>
> macb_tx_error_task()
>     spin_lock(&bp->lock);
>     macb_halt_tx()
>         read_poll_timeout_atomic(..., MACB_HALT_TIMEOUT, ...)
>
> (MACB_HALT_TIMEOUT is 14000us)

Yes this is correct. And something we will live with.
 - Our IRQ line is shared only with other queues of the same MAC.
 - That timeout is scaled to 10 Mbits/s, on 1G this timeout should be
   100 times smaller. We just don't bother scaling it with link speed.

[...]

> > -	if (unlikely(!status))
> > +	status = queue_readl(queue, ISR);
>
> [Severity: High]
> This is a pre-existing issue, but can this unconditional hardware register
> read trigger a synchronous external abort (bus fault) if the MAC is in
> runtime suspend?
>
> If another device on the same shared IRQ line triggers an interrupt while
> the MAC's clocks are gated via macb_clks_disable() in macb_runtime_suspend(),
> accessing memory-mapped registers of a clock-gated peripheral typically
> causes a bus fault.

Again, we have IRQF_SHARED only to share with other queues of the same
MAC. So that theory is moot. Long term I'd like to drop IRQF_SHARED.

>
> > +	if (unlikely(!status)) {
> > +		spin_unlock(&bp->lock);
> >  		return IRQ_NONE;
> > -
> > -	spin_lock(&bp->lock);
> > +	}
> >  
> >  	while (status) {
>
> [Severity: High]
> This is a pre-existing issue, but will this loop infinitely if an unhandled
> interrupt bit (like PFR) is set on hardware with Write-1-to-Clear ISR
> behavior?

Yes. Long term I'd like to introduce a max iteration count.
Completely unrelated to context swapping.

> [Severity: High]
> This isn't a bug introduced by this patch, but does the driver have a
> use-after-free risk during interface closure or driver removal?
>
> In macb_close(), the context structure is freed without synchronizing with
> or cancelling the asynchronous error tasks:

Yes. This one I'll fix. See previous email.

Re: [PATCH net-next v9 08/17] net: macb: introduce macb_context struct for buffer management
https://lore.kernel.org/netdev/DLCLQPVD8B9N.NFKWDS6SH0QN@xxxxxxxxxxx/

[...]

---

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

> commit 4cc0cf203c223199f230330ef1014c2cac4c4841
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: read ISR inside bp->lock critical section
>
> This moves the queue_readl(queue, ISR) in macb_interrupt() from before the
> bp->lock acquisition to inside the critical section, and unlocks on the
> early return path when status is zero.  The stated goal is to avoid acting
> on a stale ISR sample.
>
> > We risk a race on spinlock acquire; status might have changed.
> > Move the readl(ISR) inside the bp->lock critical section.
> >
> > In practice, we protect ourselves from interpreting the same ISR status
> > twice.
>
> [Severity: Low]
> [Finding: gpt-5-6-sol-1-3]
> [Sources: gpt-5-6-sol, opus-5]
> Could the changelog name the two contexts that can observe the same ISR
> sample, and the hardware precondition?

If any other context clears an IRQ then we race against that. Grep for
macb_queue_isr_clear in the code; for example the suspend !WOL case.
Maybe the commit message is badly written; it isn't a notion of
interpreting the same bit twice.

[...]

> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index d23a0d08c276..6d79663e42bf 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -2190,12 +2190,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);
> >  
> > -	if (unlikely(!status))
> > +	status = queue_readl(queue, ISR);
> > +	if (unlikely(!status)) {
> > +		spin_unlock(&bp->lock);
> >  		return IRQ_NONE;
> > -
> > -	spin_lock(&bp->lock);
> > +	}
>
> [Severity: Medium]
> [Finding: main-3-6]
> [Sources: opus-5]
> Does this remove the lock-free "not my interrupt" fast path?
>
> The handler is registered shared in macb_init():

Yes! Once again, we share only with other queues of the same MAC.
We don't support running alongside other devices.

[...]

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