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.

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