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

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

Hello Conor,

On Fri Jul 3, 2026 at 2:09 PM CEST, Conor Dooley wrote:
> 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.

Yes "slept" wasn't the right word, "waiting" would have been more
appropriate. Or it could be a race even if bp->lock acquired
immediately.

>> 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.

You are correct: this commit doesn't see change_mtu/set_ringparam take
bp->lock. It only will happen in the future. Will fix.

>
>> 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?

I agree!

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

I expect all modern(TM) MACB blocks to be write-to-clear, at least
that's what I heard but I don't remember from where.

Thanks,

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