Re: [PATCH net-next v6 14/16] net: macb: read ISR inside bp->lock critical section

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Replying to non-net sashiko
https://sashiko.dev/#/patchset/20260731-macb-context-v6-0-49d5a1439d48%40bootlin.com

> commit c95e9317ebb0315f714eea20911c711f5b1a1ce7
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: read ISR inside bp->lock critical section
>
> This commit moves the read of the Interrupt Status Register (ISR)
> inside the spinlock critical section in the macb_interrupt handler.
> The intention is to prevent a race condition where the status might
> change before the lock is acquired.
>
> > 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
> > @@ -2190,12 +2190,13 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> >  	struct net_device *netdev = bp->netdev;
> >  	u32 status;
> >  
> > +	spin_lock(&bp->lock);
>
> [Severity: High]
> Does this unconditional lock acquisition cause hardirq stalls for shared
> interrupts?
>
> If this device shares an IRQ line, macb_interrupt() is called for other
> devices' interrupts and will grab bp->lock before checking the ISR register.
>
> If another CPU is executing macb_tx_error_task(), which holds bp->lock and
> can stall for up to 14ms via macb_halt_tx(), wouldn't the CPU handling the
> shared interrupt spin with hard IRQs disabled for the entire duration?

Yes, and we cannot do anything better (at least ATM).
 - If we readl(ISR) outside bp->lock, it might contain an outdated value
   once we enter bp->lock.
 - To fix, we must readl(ISR) inside bp->lock, but then we risk stalling
   in this above (niche) case.

To recap the edge case described: we must have had a tx error
(MACB_TX_ERR_FLAGS) and have the HW refuse to halt (I don't know in
which context it can occur & I've never seen that).

Picking correctness > performance, we'll tolerate the latency risk.

> [Severity: Critical]
> Can this lead to a recursive spinlock deadlock when netconsole is active?
>
> If a hardware error like HRESP occurs, macb_interrupt_misc() logs the error
> using netdev_err(). When netconsole is enabled on this interface, the printk()
> call synchronously invokes netpoll_send_skb_on_dev(), which recursively calls
> macb_poll_controller().
>
> Since macb_poll_controller() calls macb_interrupt() on the same CPU, and the
> outer macb_interrupt() execution already holds bp->lock, will this inner
> attempt to acquire the same lock spin forever?

Yes, this was reported on [13/16].
https://lore.kernel.org/all/DKFFXBM6VKXZ.22PW13MLAXSF@bootlin.com/

Solution: defer the netdev_err() out of the critical section.

---

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260731-macb-context-v6-0-49d5a1439d48%40bootlin.com

> commit d8496423c6f6f3cdfb35cfe0e924250ba7610fa1
> Author: Théo Lebrun <theo.lebrun@bootlin.com>
>
> net: macb: read ISR inside bp->lock critical section
>
> The macb IRQ handler used to read the queue ISR register before taking
> bp->lock, and returned IRQ_NONE without touching the lock when the read
> came back empty.  This patch moves the ISR read inside the bp->lock
> critical section, so the lock is now acquired unconditionally at handler
> entry.
>
> No Link: tags are present in the commit header.
>
> > 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.
> >
> > One risk remains with spurious interrupts that would, in addition to
> > taking excessive CPU time, also create lock contention. How bad is it?
> > Probably not too bad.
>
> [Severity: Low]
> [Finding: main-1-1]
> [Sources: opus-5, old-prompts]
> This isn't a bug, but could the changelog name the concrete symptom?
>
> "We risk a race on spinlock acquire; status might have changed" does not
> say what breaks.  The pre-patch handler already re-read ISR at the bottom
> of its loop:
>
> drivers/net/ethernet/cadence/macb_main.c:macb_interrupt() {
> 	...
> 		status = queue_readl(queue, ISR);
> 	}
>
> 	spin_unlock(&bp->lock);
> 	...
> }
>
> so a stale first snapshot was tolerated by design.
>
> The rest of the series does supply a rationale that the message leaves
> out: macb_context_swap_start() sets bp->ctx_swap and does
> queue_writel(IDR, -1) / queue_readl(ISR) / macb_queue_isr_clear(-1)
> under spin_lock_irqsave(&bp->lock), and the handler's locked loop then
> tests bp->ctx_swap.  Reading ISR under bp->lock is what keeps the status
> snapshot consistent with that state.

This paragraph about bp->ctx_swap is fuzzy to me and sounds wrong. It
isn't ISR being read in bp->lock that saves us, it is bp->ctx_swap
being read in bp->lock. And that is always done (introduced in this
series).

> Would it be worth spelling that dependency out in the changelog, and
> quantifying "Probably not too bad" rather than leaving it open?

The goal is to avoid duplicated handling of the same ISR status. So all
macb_interrupt() codepaths that have side-effects are at risk. Most
should handle it fine in the normal case, but we are at risk:

 - Two napi_schedule_irqoff() are idempotent if fast enough.

 - Two schedule_work(tx_error_task) are idempotent if fast enough.

 - Double increment of rx_overruns.

 - Two queue_work(hresp_err_bh_work) are idempotent if fast enough.

 - Two pm_wakeup_event() is safe.

I'll expand the commit message.

> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 65d5ff8a5e230..27824e4688882 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)
> >  	struct net_device *netdev = bp->netdev;
> >  	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: High]
> [Finding: main-1-0]
> [Sources: opus-5, old-prompts]
> Can this recursively acquire bp->lock when netconsole is in use?
>
> macb_interrupt() now takes bp->lock before it knows whether the interrupt
> belongs to this device, and printk() calls still run inside that critical
> section.  macb_interrupt_misc() is called from within the locked loop and
> does:
>
> [...]

This is the same report as above and as in [13/16].

 - Moved the netdev_err() out of the macb_interrupt() bp->lock.

 - Ignoring voluntarily the debug calls. If you are debugging the MACB
   driver then you know what you are doing. And you probably don't want
   to debug MACB using netconsole...

> [Severity: Medium]
> [Finding: main-5-5]
> [Sources: opus-5, old-prompts]
> Does this add hardirq latency for other devices on the shared line?
>
> The interrupt is requested shared:
>
> drivers/net/ethernet/cadence/macb_main.c:macb_probe() {
> 	...
> 		err = devm_request_irq(&pdev->dev, queue->irq, macb_interrupt,
> 				       IRQF_SHARED, netdev->name, queue);
> 	...
> }
>
> so macb_interrupt() also runs for interrupts owned by other devices on
> the line and for spurious/level assertions.  Those cases used to complete
> lock-free in two MMIO cycles; now they block on bp->lock first.
>
> [...]

Yes it can add latency. No we cannot do anything in this series. Here we
just fix a correctness bug.

Proper fixes would be:

 - Per-queue locking mechanism.

 - I expect the main (only?) reason for shared support is queues using
   the same IRQ lane. In that case it would be more efficient to
   acquire a single handler that handles all queues at once.

   Instead of calling macb_interrupt() N times and locking N times, we'd
   do it once and iterate inside.

Both are completely unrelated to this series. Here, as said before, we
do correctness > performance.

Thanks,

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