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*. - Two pm_wakeup_event() is safe. One risk remains with spurious interrupts that would, in addition to taking excessive CPU time, also create lock contention. How bad is it? Probably not too bad. Reviewed-by: Nicolai Buchwitz Signed-off-by: Théo Lebrun --- drivers/net/ethernet/cadence/macb_main.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 4bf55994420f..100159662336 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2193,13 +2193,14 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id) bool hresp_err = false; u32 status; - status = queue_readl(queue, ISR); - - if (unlikely(!status)) - return IRQ_NONE; - 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