Re: [PATCH net-next v3 13/15] net: macb: re-read ISR inside IRQ handler locked section

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2844 bytes --]

Hey,

I'll admit to being a bit confused by this patch..

On Wed, Jul 01, 2026 at 05:59:16PM +0200, Théo Lebrun wrote:
> 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.
> 
> If we tried grabbing bp->lock while already acquired, we might have
> slept and the status might have been updated. Our most likely

This mention of sleeping I think should be removed, it implies sleeping
is required for the status to be updated.

> competitor in this race (condition) is a swap operation, used in
> change_mtu and set_ringparam. It is the only MACB codepath that resets
> interrupts and HW inside a bp->lock critical section. Other codepaths
> that clear HW IRQ status do so outside the bp->lock critical section.

Where do change_mtu and set_ringparam take the bp lock?
As far as I can see, they don't. The commit message should reflect how
the code behaves at the time of the patch, not at some point in the
future after it.

> We can only detect spurious interrupts before grabbing bp->lock if
> MACB_CAPS_ISR_CLEAR_ON_WRITE. If we don't, then we only read ISR once.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 7245c345c78f..5a32d5cb759e 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2184,13 +2184,21 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  	struct net_device *netdev = bp->netdev;
>  	u32 status;
>  
> -	status = queue_readl(queue, ISR);
> -
> -	if (unlikely(!status))
> -		return IRQ_NONE;
> +	/* detect spurious interrupts without grabbing bp->lock */
> +	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE) {
> +		status = queue_readl(queue, ISR);
> +		if (unlikely(!status))
> +			return IRQ_NONE;
> +	}

To be honest, this check feels like penalising the likely^2 case* with an
extra read favour of the unlikely case where there's contention on the
lock and the contending function is capable of affecting the status
register. Could we get away with just the single check of the register
with the lock taken?

* although I don't know what percentage of hardware supports this cap,
so maybe most devices will never run this code

>  
>  	spin_lock(&bp->lock);
>  
> +	status = queue_readl(queue, ISR);
> +	if (unlikely(!status)) {
> +		spin_unlock(&bp->lock);
> +		return IRQ_NONE;
> +	}
> +
>  	while (status) {
>  		/* close possible race with dev_close */
>  		if (unlikely(!netif_running(netdev))) {
> 
> -- 
> 2.55.0

> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]