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

Replying to non-net sashiko
https://sashiko.dev/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

commit 48c8a049fccfd5d7c55b15a7fd08dc4eca66d836
Author: Théo Lebrun [off-list ref]
Date:   Wed Aug 12 08:03:29 2026 +0000

    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.

quoted

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d23a0d08c2769..6d79663e42bfe 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)
 	bool hresp_err = false;
 	u32 status;
 
-	status = queue_readl(queue, ISR);
+	spin_lock(&bp->lock);

[Severity: High]
Does taking this lock unconditionally cause severe latency spikes for other
devices on a shared interrupt line?

If a shared interrupt fires while the driver's error handling task is running,
this CPU might spin with local interrupts disabled for up to 14ms:

macb_tx_error_task()
    spin_lock(&bp->lock);
    macb_halt_tx()
        read_poll_timeout_atomic(..., MACB_HALT_TIMEOUT, ...)

(MACB_HALT_TIMEOUT is 14000us)

Yes this is correct. And something we will live with.
 - Our IRQ line is shared only with other queues of the same MAC.
 - That timeout is scaled to 10 Mbits/s, on 1G this timeout should be
   100 times smaller. We just don't bother scaling it with link speed.

[...]

quoted

-	if (unlikely(!status))
+	status = queue_readl(queue, ISR);

[Severity: High]
This is a pre-existing issue, but can this unconditional hardware register
read trigger a synchronous external abort (bus fault) if the MAC is in
runtime suspend?

If another device on the same shared IRQ line triggers an interrupt while
the MAC's clocks are gated via macb_clks_disable() in macb_runtime_suspend(),
accessing memory-mapped registers of a clock-gated peripheral typically
causes a bus fault.

Again, we have IRQF_SHARED only to share with other queues of the same
MAC. So that theory is moot. Long term I'd like to drop IRQF_SHARED.

quoted

+	if (unlikely(!status)) {
+		spin_unlock(&bp->lock);
 		return IRQ_NONE;
-
-	spin_lock(&bp->lock);
+	}
 
 	while (status) {

[Severity: High]
This is a pre-existing issue, but will this loop infinitely if an unhandled
interrupt bit (like PFR) is set on hardware with Write-1-to-Clear ISR
behavior?

Yes. Long term I'd like to introduce a max iteration count.
Completely unrelated to context swapping.

[Severity: High]
This isn't a bug introduced by this patch, but does the driver have a
use-after-free risk during interface closure or driver removal?

In macb_close(), the context structure is freed without synchronizing with
or cancelling the asynchronous error tasks:

Yes. This one I'll fix. See previous email.

Re: [PATCH net-next v9 08/17] net: macb: introduce macb_context struct for buffer management
https://lore.kernel.org/netdev/DLCLQPVD8B9N.NFKWDS6SH0QN@bootlin.com/ (local)

[...]

---

Replying to net sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

commit 4cc0cf203c223199f230330ef1014c2cac4c4841
Author: Théo Lebrun [off-list ref]

net: macb: read ISR inside bp->lock critical section

This moves the queue_readl(queue, ISR) in macb_interrupt() from before the
bp->lock acquisition to inside the critical section, and unlocks on the
early return path when status is zero.  The stated goal is to avoid acting
on a stale ISR sample.

quoted

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.

[Severity: Low]
[Finding: gpt-5-6-sol-1-3]
[Sources: gpt-5-6-sol, opus-5]
Could the changelog name the two contexts that can observe the same ISR
sample, and the hardware precondition?

If any other context clears an IRQ then we race against that. Grep for
macb_queue_isr_clear in the code; for example the suspend !WOL case.
Maybe the commit message is badly written; it isn't a notion of
interpreting the same bit twice.

[...]

quoted

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d23a0d08c276..6d79663e42bf 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)
 	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: Medium]
[Finding: main-3-6]
[Sources: opus-5]
Does this remove the lock-free "not my interrupt" fast path?

The handler is registered shared in macb_init():

Yes! Once again, we share only with other queues of the same MAC.
We don't support running alongside other devices.

[...]

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