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

[Théo Lebrun]

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 <nb@xxxxxxxxxxx>
Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
---
 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