Re: [PATCH net 3/3] net: macb: quiesce IRQs and drain BH on interface close

Hello netdev-bot+sashiko,

On Tue Sep 22, 2026 at 10:37 PM CEST,  wrote:

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] macb_close() now calls macb_quiesce_start() (synchronize_irq()
  on every queue IRQ, cancel_work_sync(&bp->hresp_err_bh_work),…

Pre-existing issues:
- [High] macb_suspend()/macb_resume() still open-code the per-queue
  napi_disable()/napi_enable() loops without setting/clearing…
- [High] macb_set_ringparam() calls macb_close() and then macb_open()
  while discarding macb_open()'s return value and always returning 0.
- [Medium] bp->hresp_err_bh_work and bp->tx_lpi_work are initialized by
  INIT_WORK()/INIT_DELAYED_WORK() only *after* register_netdev() in…

--

net: macb: quiesce IRQs and drain BH on interface close

This adds macb_quiesce_start()/macb_quiesce_end() helpers plus a new
bp->irq_quiesced flag so that macb_interrupt() self-disarms while the
interface is down, and reorders macb_close() to drain IRQs, the two work
items, NAPI and the LPI delayed work in a defined order. macb_open() uses
the same helpers on its success and error paths.

A few questions below.

quoted

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index cfaa0ca49f1a..1cb2778fe49e 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h

@@ -1382,6 +1382,11 @@ struct macb {
 	struct delayed_work	tx_lpi_work;
 	u32			tx_lpi_timer;
 
+	/* ISR must not drive NAPI & BH mechanisms. True when the interface
+	 * is closed. Protected by bp->lock.
+	 */
+	bool			irq_quiesced;
+
 	int	rx_bd_rd_prefetch;
 	int	tx_bd_rd_prefetch;
 

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c418f859cc34..75bbde81d62c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -1995,6 +1995,53 @@ static int macb_tx_poll(struct napi_struct *napi, int budget)
 	return work_done;
 }
 
+static void macb_quiesce_start(struct macb *bp)
+{
+	struct macb_queue *queue;
+	unsigned long flags;
+	unsigned int q;
+
+	spin_lock_irqsave(&bp->lock, flags);
+	bp->irq_quiesced = true;
+	spin_unlock_irqrestore(&bp->lock, flags);
+
+	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+		synchronize_irq(queue->irq);
+
+	cancel_work_sync(&bp->hresp_err_bh_work);

[Severity: Medium]
This isn't a bug introduced by this patch, but can this cancel_work_sync()
run on a work_struct that has not been initialized yet?

In macb_probe(), register_netdev() happens before the two work items are
set up:

	err = register_netdev(netdev);
	if (err) {
		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
		goto err_out_free_tieoff;
	}

	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);
	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);

The queue IRQ handlers were already installed earlier by
macb_init_dflt()'s devm_request_irq(..., macb_interrupt, ...), and
register_netdev() publishes the device, so an 'ip link set up' racing with
the tail of probe can reach macb_open() -> macb_quiesce_end(), which clears
bp->irq_quiesced. From there an HRESP interrupt would let
macb_interrupt_misc() do queue_work(system_bh_wq, &bp->hresp_err_bh_work)
on a zeroed work_struct whose ->func is NULL, and macb_close() or the
macb_open() error path would call cancel_work_sync() /
cancel_delayed_work_sync() on the same uninitialized objects.

The pre-existing macb_close() already had the cancel_delayed_work_sync() on
bp->tx_lpi_work, so this only widens an existing window. Would moving both
INIT_WORK()/INIT_DELAYED_WORK() calls ahead of macb_init()/register_netdev()
be reasonable?

This race is out of this world, but does technically exist.
Let's fix it anyway.

quoted

+
+	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+		/* Must be done before NAPI is disabled: the task ends with a
+		 * napi_enable() call.
+		 */
+		cancel_work_sync(&queue->tx_error_task);
+
+		napi_disable(&queue->napi_rx);
+		napi_disable(&queue->napi_tx);
+	}
+
+	/* Must be done after napi_tx is disabled: its completion re-arms
+	 * the LPI timer.
+	 */
+	cancel_delayed_work_sync(&bp->tx_lpi_work);
+}
+
+static void macb_quiesce_end(struct macb *bp)
+{
+	struct macb_queue *queue;
+	unsigned long flags;
+	unsigned int q;
+
+	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
+		napi_enable(&queue->napi_rx);
+		napi_enable(&queue->napi_tx);
+	}
+
+	spin_lock_irqsave(&bp->lock, flags);
+	bp->irq_quiesced = false;
+	spin_unlock_irqrestore(&bp->lock, flags);
+}
+
 static void macb_hresp_error_task(struct work_struct *work)
 {
 	struct macb *bp = from_work(bp, work, hresp_err_bh_work);

@@ -2137,8 +2184,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 	spin_lock(&bp->lock);
 
 	while (status) {
-		/* close possible race with dev_close */
-		if (unlikely(!netif_running(netdev))) {
+		/* self-disarm while the netdev is closed */
+		if (unlikely(bp->irq_quiesced)) {
 			queue_writel(queue, IDR, -1);
 			macb_queue_isr_clear(bp, queue, -1);
 			break;

[Severity: High]
This is a pre-existing issue, but does the new gating condition need a
companion change in macb_suspend()/macb_resume()?

Pre-existing. Similarly to HRESP task, the infra will be put in place
and reusable to fix the suspend path which needs to shutdown all BH
features, even in WOL case.

[...]

quoted

@@ -3155,8 +3202,6 @@ static int macb_open(struct net_device *netdev)
 {
 	size_t bufsz = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + NET_IP_ALIGN;
 	struct macb *bp = netdev_priv(netdev);
-	struct macb_queue *queue;
-	unsigned int q;
 	int err;
 
 	netdev_dbg(bp->netdev, "open\n");

@@ -3180,10 +3225,7 @@ static int macb_open(struct net_device *netdev)
 		goto free_rings;
 	macb_init_buffers(bp);
 
-	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		napi_enable(&queue->napi_rx);
-		napi_enable(&queue->napi_tx);
-	}
+	macb_quiesce_end(bp);
 
 	macb_init_hw(bp);
 

@@ -3210,11 +3252,10 @@ static int macb_open(struct net_device *netdev)
 	phy_power_off(bp->phy);
 
 reset_hw:
+	/* The netdev stays down: quiesce and drain, as macb_close() does. */
+	macb_quiesce_start(bp);
+
 	macb_reset_hw(bp);
-	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		napi_disable(&queue->napi_rx);
-		napi_disable(&queue->napi_tx);
-	}
 free_rings:
 	macb_free(bp);
 pm_exit:

[Severity: High]
This is a pre-existing issue, but does the new reset_hw path make the
macb_set_ringparam() failure case worse?

We ignore macb_set_ringparam() close/open when netif_running() because
we know what the right solution is (context swapping) and it's coming.
One step at a time.

[...]

quoted

@@ -3225,19 +3266,17 @@ static int macb_open(struct net_device *netdev)
 static int macb_close(struct net_device *netdev)
 {
 	struct macb *bp = netdev_priv(netdev);
-	struct macb_queue *queue;
 	unsigned long flags;
 	unsigned int q;
 
-	netif_tx_stop_all_queues(netdev);
+	macb_quiesce_start(bp);
 
-	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		napi_disable(&queue->napi_rx);
-		napi_disable(&queue->napi_tx);
+	/* Drain the BH contexts before stopping the queues: NAPI completion
+	 * and tx_error_task wake them up.
+	 */
+	netif_tx_stop_all_queues(netdev);
+	for (q = 0; q < bp->num_queues; ++q)
 		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, q));
-	}
-
-	cancel_delayed_work_sync(&bp->tx_lpi_work);

[Severity: Medium]
Moving netif_tx_stop_all_queues() after the whole quiesce sequence looks
fine for the ndo_stop path, where the core already ran
dev_deactivate_many() so no xmit can happen. Is it also safe for the direct
caller?

Again, context swapping is the solution. Sashiko complains the
close/open dance in set_ringparam is unsafe, but anyway before this
series macb_close() is super race sensitive and we fix it here. One
step at a time.

[...]

Thanks,

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