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

[Théo Lebrun]

Replying to sashiko
https://sashiko.dev/#/patchset/20260803-macb-context-v7-0-4d7d4af04849%40bootlin.com

> commit add26bbfae8660854cbc34d74da1557f7bfd9fae
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> 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.
>
> In practice, we protect ourselves from interpreting the same ISR status
> twice. To understand issues, read macb_interrupt() body and imagine
> each codepath being performed twice:
>  - 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*.
>
> [...]
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 7e77c24da8a63..0778ce9b3223d 100644
> > --- 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);
> >  
> > -	if (unlikely(!status))
> > +	status = queue_readl(queue, ISR);
>
> [...]
>
> > +	if (unlikely(!status)) {
> > +		spin_unlock(&bp->lock);
> >  		return IRQ_NONE;
> > -
> > -	spin_lock(&bp->lock);
> > +	}
>
> [Severity: High]
> Does unconditionally acquiring bp->lock in this shared IRQ handler before
> verifying the interrupt status introduce severe lock contention?
>
> Since macb_interrupt() is registered with IRQF_SHARED, it is called when
> another device on the same IRQ line triggers an interrupt.
>
> If macb_tx_error_task() is running on another CPU, it acquires bp->lock
> with IRQs disabled and polls hardware for up to 14ms (MACB_HALT_TIMEOUT):
>
> macb_tx_error_task() {
>     ...
>     spin_lock_irqsave(&bp->lock, flags);
>     netif_tx_stop_all_queues(bp->netdev);
>     if (macb_halt_tx(bp)) {
>     ...
> }
>
> During this time, wouldn't the CPU servicing the foreign interrupt spin on
> bp->lock with local interrupts disabled for up to 14ms, only to eventually
> read ISR == 0 and return IRQ_NONE? Could this stall other devices sharing
> the IRQ line?

So it's describing two separate issues here.
As usual, Sashiko isn't the best at summaries.

 - macb_interrupt() now unconditionally grabs bp->lock. That combined
   with it being a shared IRQ => when we land in the IRQ_NONE case we
   now have locked bp->lock for "no reason".

   => As described before, correctness > performance. Here we don't have
      the choice unless we fully redesign the driver.

   See this reply to Sashiko on previous revision:
   https://lore.kernel.org/all/DKFGXHSHU73O.JE9E3TEWDAOX@xxxxxxxxxxx/

 - macb_tx_error_task() has a codepath where it blocks bp->lock for a
   14ms timeout.

   => Unrelated to this specific patch or even to the whole series.

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