[PATCH net 0/3] net: macb: fix close races (and RX refill error handling)

[PATCH net 0/3] net: macb: fix close races (and RX refill error handling)

[Théo Lebrun]

The context-swapping series [0] has been ongoing for a while. The most
interesting part is a proper hardware shutdown sequence, which is truly
lacking in other parts of the MACB driver: at close, at suspend and in
the HRESP error task.

Instead of introducing that sequence for a new feature, apply it now to
fix the main offender, the close path. That fixes the races listed in
patch 3 and will allow the sequence to be reused later.

The first two patches are also fixes, but not related to the shutdown
sequence: they address allocation-failure codepaths. They are sent
alongside patch 3 because they touch the same code and would cause
merge conflicts if applied separately.

Now, let me list issues I'm aware of that we do *not* fix here, to make
the scope explicit:
 - HRESP task should sync with all other contexts. It frees buffers
   under the feet of the whole driver. The hardware shutdown sequence
   will help.
 - Suspend callback is also racy with BH primitives. We'll be able to
   reuse the hardware shutdown sequence.
 - Even if we assume tasks are frozen, the phylink ops aren't and might
   trigger between suspend and resume callbacks. Here we need to
   (1) early return in phylink ops and (2) at resume put the HW in its
   proper state according to phylink ops that occured.
 - Alloc failure codepaths aren't perfect outside open. Hardware and
   ring buffers are left in a sad state. We can probably do better.

[0]: https://lore.kernel.org/all/20260812-macb-context-v9-0-7ddbf5f715e0@bootlin.com/

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Théo Lebrun (3):
      net: macb: never give hardware a NULL RX buffer
      net: macb: propagate RX ring refill errors
      net: macb: quiesce IRQs and drain BH on interface close

 drivers/net/ethernet/cadence/macb.h      |   7 +-
 drivers/net/ethernet/cadence/macb_main.c | 119 ++++++++++++++++++++++++-------
 2 files changed, 98 insertions(+), 28 deletions(-)
---
base-commit: 994db8ab9d90c64dd641b7ead6efe2eaea7a50dc
change-id: 20260918-macb-close-dbbd23b4b8f2

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH net 1/3] net: macb: never give hardware a NULL RX buffer

[Théo Lebrun]

The refill logic is simple: iterate over all pending rx slots, allocate
SKB (& DMA map) if needed and hand it off the to hardware by clearing
the RX_USED flag.

If the refill operation fails mid-way, it early returns leaving the
following slots untouched. In the normal case that is fine, because all
slots have been properly initialised (and might have been already used
by HW meaning they won't be reused). When slots have not been
initialised however, we are in trouble.

After dma_alloc_coherent() of the rx ring buffer, all slots have NULL
pointers and RX_USED cleared meaning HW will try using them. Ensure
this does not happen by setting the RX_USED flag on all slots before
calling refill at buffer alloc, in gem_init_rx_ring(). That way even if
refill fails on an alloc/dma_map, the HW won't try using NULL pointers
as buffers.

Theoretical bugfix, never encountered in practice. To reproduce,
introduce memory pressure (less than 512 SKBs of free memory) and open
the interface.

Note that this codepath also hits at resume, on HRESP errors and on
set_ringparam (while interface is running).

Fixes: 4df95131ea80 ("net/macb: change RX path for GEM")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b8234ac4b602..751fa9e68099 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2785,9 +2785,14 @@ static int macb_alloc(struct macb *bp)
 
 static void gem_init_rx_ring(struct macb_queue *queue)
 {
+	unsigned int i;
+
 	queue->rx_tail = 0;
 	queue->rx_prepared_head = 0;
 
+	for (i = 0; i < queue->bp->rx_ring_size; i++)
+		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
+
 	gem_rx_refill(queue);
 }
 

-- 
2.55.0

Re: [PATCH net 1/3] net: macb: never give hardware a NULL RX buffer

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · 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).

- [High] gem_init_rx_ring()
  (drivers/net/ethernet/cadence/macb_main.c:2793) now sets…
- [High] gem_init_rx_ring() now publishes MACB_BIT(RX_USED) on every RX
  descriptor of a queue, resets queue->rx_tail/rx_prepared_head and then…

--

net: macb: never give hardware a NULL RX buffer

This makes gem_init_rx_ring() set the RX_USED bit on every RX descriptor
before calling gem_rx_refill(), so that a refill that fails part way
through does not leave descriptors owned by hardware while their skb
pointers are still NULL.  The change affects macb_open(), macb_resume(),
the HRESP error recovery path and set_ringparam().

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index b8234ac4b602..751fa9e68099 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2785,9 +2785,14 @@ static int macb_alloc(struct macb *bp)
>  
>  static void gem_init_rx_ring(struct macb_queue *queue)
>  {
> +	unsigned int i;
> +
>  	queue->rx_tail = 0;
>  	queue->rx_prepared_head = 0;
>  
> +	for (i = 0; i < queue->bp->rx_ring_size; i++)
> +		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
> +

[Severity: High]

Does RX_USED now carry two different meanings that the RX consume path
cannot tell apart?

gem_rx() uses RX_USED at queue->rx_tail as the only indication that
hardware delivered a frame, and it has no bound against
queue->rx_prepared_head:

drivers/net/ethernet/cadence/macb_main.c:gem_rx() {
	rxused = (desc->addr & MACB_BIT(RX_USED)) ? true : false;
	addr = macb_get_addr(bp, desc);

	if (!rxused)
		break;
	...
	queue->rx_tail++;
	count++;

	if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF))) {
		netdev_err(bp->netdev,
			   "not whole frame pointed by descriptor\n");
		bp->netdev->stats.rx_dropped++;
		queue->stats.rx_dropped++;
		break;
	}
	skb = queue->rx_skbuff[entry];
	if (unlikely(!skb)) {
		netdev_err(bp->netdev,
			   "inconsistent Rx descriptor chain\n");
		...
}

So for a slot that gem_rx_refill() never got to prepare (the exact
memory-pressure case in the commit message, where it breaks out of the
loop on netdev_alloc_skb() or dma_map_single() failure), gem_rx() no
longer stops at "if (!rxused) break;".  It advances rx_tail, counts the
slot as work, hits the SOF/EOF test with ctrl == 0 on a fresh ring, emits
a non-ratelimited netdev_err() and bumps rx_dropped.

Then does the poll reschedule itself forever?  macb_rx_poll() consults
macb_rx_pending(), which also looks only at RX_USED at rx_tail:

drivers/net/ethernet/cadence/macb_main.c:macb_rx_pending() {
	entry = macb_rx_ring_wrap(bp, queue->rx_tail);
	desc = macb_rx_desc(queue, entry);
	rmb();
	return (desc->addr & MACB_BIT(RX_USED)) != 0;
}

The next slot is also software-marked RX_USED, so macb_rx_poll() calls
napi_schedule() again.  Since gem_rx_refill() only fills up to
CIRC_SPACE(rx_prepared_head, rx_tail, rx_ring_size), the slot at rx_tail
is never the one refilled, so while allocations keep failing nothing
clears RX_USED ahead of rx_tail.  Can this turn into a self-sustaining
NAPI/softirq loop with two unratelimited error lines and one bogus
rx_dropped per iteration, and once allocations recover, a full ring
traversal (rx_ring_size, 512 by default) of phantom completions that
walks rx_tail past slots hardware never wrote?

Before this patch gem_rx() stopped at those same slots on
"if (!rxused) break;".

Reachable at least via macb_resume() and macb_hresp_error_task(), which
both ignore the refill result and re-enable RX and NAPI afterwards.

[Severity: High]

Is any exclusion against a running RX poll needed here now that the whole
ring is published as RX_USED?

macb_hresp_error_task() runs from system_bh_wq, masks queue interrupts,
clears RE/TE, and then calls mog_init_rings() with no napi_disable(), no
napi_synchronize() and no lock:

drivers/net/ethernet/cadence/macb_main.c:macb_hresp_error_task() {
	netif_tx_stop_all_queues(netdev);
	netif_carrier_off(netdev);

	bp->macbgem_ops.mog_init_rings(bp);
	...
}

Masking interrupts does not stop a poll that is already scheduled or
running on another CPU (per-queue IRQs can be affinitized, and NAPI can
be threaded).  Previously such a concurrent gem_rx() would stop at the
first hardware-owned descriptor; with every descriptor marked RX_USED,
does it now walk the entire ring reading stale desc->ctrl (the new loop
does not clear ctrl), setting queue->rx_skbuff[entry] = NULL,
dma_unmap_single()ing and napi_gro_receive()ing the buffers?

Meanwhile gem_rx_refill() called from gem_init_rx_ring() walks the same
entries, and for any entry whose rx_skbuff it still sees as non-NULL it
takes the else branch:

drivers/net/ethernet/cadence/macb_main.c:gem_rx_refill() {
	} else {
		desc->ctrl = 0;
		dma_wmb();
		desc->addr &= ~MACB_BIT(RX_USED);
	}
}

which re-arms hardware with the address of a buffer the concurrent poll
may have just unmapped and handed to the stack.  Can the device then DMA
into stack-owned memory?  The two contexts also both write
queue->rx_tail / rx_prepared_head and can both allocate for the same
entry, leaking an skb and its DMA mapping.

>  	gem_rx_refill(queue);
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-close-v1-0-05e32ce98813%40bootlin.com

[PATCH net 2/3] net: macb: propagate RX ring refill errors

[Théo Lebrun]

gem_rx_refill() is responsible for Rx SKB allocation, including at open,
but its prototype indicates a void return value.

Therefore we change the code to propagate allocation and DMA mapping
errors back up the stack, making sure the open fails if it occurs.
Change all those to return errno-style ints:
 - gem_rx_refill()
 - its parent gem_init_rx_ring()
 - its grand-parent gem_init_rings()
 - the macbgem_ops.mog_init_rings function pointer
 - its grand-uncle macb_init_rings()

Theoretical bugfix, never encountered in practice. To reproduce,
introduce memory pressure (less than 512 SKBs of free memory) and open
the interface. I expect the last queue to be unuseable because it has
zero usable rx buffers. Nothing will ever trigger a refill on that
queue which only happens once a frame has been received.

Note that other callers of refill (resume, HRESP error task, NAPI)
cannot do anything useful with that error and keep their best-effort
refill, hoping it will improve.

Fixes: 4df95131ea80 ("net/macb: change RX path for GEM")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h      |  2 +-
 drivers/net/ethernet/cadence/macb_main.c | 30 +++++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index d6931c41f39d..cfaa0ca49f1a 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1197,7 +1197,7 @@ struct macb_queue;
 struct macb_or_gem_ops {
 	int	(*mog_alloc_rx_buffers)(struct macb *bp);
 	void	(*mog_free_rx_buffers)(struct macb *bp);
-	void	(*mog_init_rings)(struct macb *bp);
+	int	(*mog_init_rings)(struct macb *bp);
 	int	(*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
 			  int budget);
 };
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 751fa9e68099..c418f859cc34 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1486,13 +1486,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
 	return packets;
 }
 
-static void gem_rx_refill(struct macb_queue *queue)
+static int gem_rx_refill(struct macb_queue *queue)
 {
 	struct macb *bp = queue->bp;
 	struct macb_dma_desc *desc;
 	struct sk_buff *skb;
 	unsigned int entry;
 	dma_addr_t paddr;
+	int err = 0;
 
 	while (CIRC_SPACE(queue->rx_prepared_head, queue->rx_tail,
 			bp->rx_ring_size) > 0) {
@@ -1509,6 +1510,7 @@ static void gem_rx_refill(struct macb_queue *queue)
 			if (unlikely(!skb)) {
 				netdev_err(bp->netdev,
 					   "Unable to allocate sk_buff\n");
+				err = -ENOMEM;
 				break;
 			}
 
@@ -1518,6 +1520,7 @@ static void gem_rx_refill(struct macb_queue *queue)
 					       DMA_FROM_DEVICE);
 			if (dma_mapping_error(&bp->pdev->dev, paddr)) {
 				dev_kfree_skb(skb);
+				err = -ENOMEM;
 				break;
 			}
 
@@ -1558,6 +1561,8 @@ static void gem_rx_refill(struct macb_queue *queue)
 
 	netdev_vdbg(bp->netdev, "rx ring: queue: %p, prepared head %d, tail %d\n",
 		    queue, queue->rx_prepared_head, queue->rx_tail);
+
+	return err;
 }
 
 /* Mark DMA descriptors from begin up to and not including end as unused */
@@ -2783,7 +2788,7 @@ static int macb_alloc(struct macb *bp)
 	return -ENOMEM;
 }
 
-static void gem_init_rx_ring(struct macb_queue *queue)
+static int gem_init_rx_ring(struct macb_queue *queue)
 {
 	unsigned int i;
 
@@ -2793,14 +2798,16 @@ static void gem_init_rx_ring(struct macb_queue *queue)
 	for (i = 0; i < queue->bp->rx_ring_size; i++)
 		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
 
-	gem_rx_refill(queue);
+	return gem_rx_refill(queue);
 }
 
-static void gem_init_rings(struct macb *bp)
+static int gem_init_rings(struct macb *bp)
 {
 	struct macb_queue *queue;
 	struct macb_dma_desc *desc = NULL;
+	int last_err = 0;
 	unsigned int q;
+	int err;
 	int i;
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
@@ -2813,11 +2820,15 @@ static void gem_init_rings(struct macb *bp)
 		queue->tx_head = 0;
 		queue->tx_tail = 0;
 
-		gem_init_rx_ring(queue);
+		err = gem_init_rx_ring(queue);
+		if (err)
+			last_err = err;
 	}
+
+	return last_err;
 }
 
-static void macb_init_rings(struct macb *bp)
+static int macb_init_rings(struct macb *bp)
 {
 	int i;
 	struct macb_dma_desc *desc = NULL;
@@ -2832,6 +2843,8 @@ static void macb_init_rings(struct macb *bp)
 	bp->queues[0].tx_head = 0;
 	bp->queues[0].tx_tail = 0;
 	desc->ctrl |= MACB_BIT(TX_WRAP);
+
+	return 0;
 }
 
 static void macb_reset_hw(struct macb *bp)
@@ -3162,7 +3175,9 @@ static int macb_open(struct net_device *netdev)
 		goto pm_exit;
 	}
 
-	bp->macbgem_ops.mog_init_rings(bp);
+	err = bp->macbgem_ops.mog_init_rings(bp);
+	if (err)
+		goto free_rings;
 	macb_init_buffers(bp);
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
@@ -3200,6 +3215,7 @@ static int macb_open(struct net_device *netdev)
 		napi_disable(&queue->napi_rx);
 		napi_disable(&queue->napi_tx);
 	}
+free_rings:
 	macb_free(bp);
 pm_exit:
 	pm_runtime_put_sync(&bp->pdev->dev);

-- 
2.55.0

Re: [PATCH net 2/3] net: macb: propagate RX ring refill errors

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 0 · 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).

- [High] Incomplete fix: the new errno from
  gem_rx_refill()/gem_init_rx_ring()/mog_init_rings() is discarded by…
- [High] macb_set_ringparam()
  (drivers/net/ethernet/cadence/macb_main.c:3737-3749) closes and…

Pre-existing issues:
- [High] macb_hresp_error_task() (macb_main.c:1998-2035, BH-workqueue
  context) calls bp->macbgem_ops.mog_init_rings(bp) -> gem_init_rings()…

--

net: macb: propagate RX ring refill errors

This changes gem_rx_refill(), gem_init_rx_ring(), gem_init_rings(),
macb_init_rings() and the macbgem_ops.mog_init_rings function pointer to
return errno-style ints, so that macb_open() fails when RX skb allocation
or DMA mapping fails. The other refill callers (resume, HRESP error task,
NAPI) keep their best-effort behaviour.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index d6931c41f39d..cfaa0ca49f1a 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1197,7 +1197,7 @@ struct macb_queue;
>  struct macb_or_gem_ops {
>  	int	(*mog_alloc_rx_buffers)(struct macb *bp);
>  	void	(*mog_free_rx_buffers)(struct macb *bp);
> -	void	(*mog_init_rings)(struct macb *bp);
> +	int	(*mog_init_rings)(struct macb *bp);
>  	int	(*mog_rx)(struct macb_queue *queue, struct napi_struct *napi,
>  			  int budget);
>  };
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 751fa9e68099..c418f859cc34 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1486,13 +1486,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  	return packets;
>  }
>  
> -static void gem_rx_refill(struct macb_queue *queue)
> +static int gem_rx_refill(struct macb_queue *queue)
>  {
>  	struct macb *bp = queue->bp;
>  	struct macb_dma_desc *desc;
>  	struct sk_buff *skb;
>  	unsigned int entry;
>  	dma_addr_t paddr;
> +	int err = 0;
>  
>  	while (CIRC_SPACE(queue->rx_prepared_head, queue->rx_tail,
>  			bp->rx_ring_size) > 0) {
> @@ -1509,6 +1510,7 @@ static void gem_rx_refill(struct macb_queue *queue)
>  			if (unlikely(!skb)) {
>  				netdev_err(bp->netdev,
>  					   "Unable to allocate sk_buff\n");
> +				err = -ENOMEM;
>  				break;
>  			}
>  
> @@ -1518,6 +1520,7 @@ static void gem_rx_refill(struct macb_queue *queue)
>  					       DMA_FROM_DEVICE);
>  			if (dma_mapping_error(&bp->pdev->dev, paddr)) {
>  				dev_kfree_skb(skb);
> +				err = -ENOMEM;
>  				break;
>  			}
>  
> @@ -1558,6 +1561,8 @@ static void gem_rx_refill(struct macb_queue *queue)
>  
>  	netdev_vdbg(bp->netdev, "rx ring: queue: %p, prepared head %d, tail %d\n",
>  		    queue, queue->rx_prepared_head, queue->rx_tail);
> +
> +	return err;
>  }

[Severity: High]
The commit message says:

    Note that other callers of refill (resume, HRESP error task, NAPI)
    cannot do anything useful with that error and keep their best-effort
    refill, hoping it will improve.

Is the "hoping it will improve" part true for those callers, given the
same analysis used to justify the open() fix?

gem_init_rx_ring() first marks every descriptor as software owned and
resets both cursors:

	queue->rx_tail = 0;
	queue->rx_prepared_head = 0;

	for (i = 0; i < queue->bp->rx_ring_size; i++)
		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);

	return gem_rx_refill(queue);

gem_rx_refill() then breaks out on the first netdev_alloc_skb() or
dma_mapping_error() failure, so it can return with zero descriptors handed
to the hardware.

With no hardware-owned descriptor, can the controller ever complete a
receive and raise RCOMP?  bp->rx_intr_mask is only MACB_RX_INT_FLAGS
(RCOMP | ISR_ROVR) unless the part has MACB_CAPS_NEEDS_RSTONUBR:

	bp->rx_intr_mask = MACB_RX_INT_FLAGS;
	if (bp->caps & MACB_CAPS_NEEDS_RSTONUBR)
		bp->rx_intr_mask |= MACB_BIT(RXUBR);

and even on those parts macb_interrupt_misc() only toggles NCR.RE without
refilling.  The single remaining refill trigger, gem_rx(), is only reached
through napi_schedule on RCOMP/ROVR.

The callers that discard the new return value re-arm the receiver anyway:

macb_resume():

	if (macb_is_gem(bp))
		gem_init_rx_ring(queue);
	...
	macb_init_hw(bp);

macb_hresp_error_task():

	bp->macbgem_ops.mog_init_rings(bp);
	...
	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
	macb_writel(bp, NCR, ctrl);

gem_rx():

	gem_rx_refill(queue);

	return count;

So can a queue still end up permanently RX-dead after resume, after HRESP
recovery, or at runtime when a poll drains every armed descriptor and the
following refill fails on its first allocation, with recovery only through
an administrative down/up?  Would it make sense for those paths to retry
(NAPI reschedule or delayed work), or to keep the receiver disabled for
the affected queue, rather than only propagating the error at open()?

>  
>  /* Mark DMA descriptors from begin up to and not including end as unused */

[ ... ]

> @@ -2793,14 +2798,16 @@ static int macb_alloc(struct macb *bp)
>  	for (i = 0; i < queue->bp->rx_ring_size; i++)
>  		macb_rx_desc(queue, i)->addr |= MACB_BIT(RX_USED);
>  
> -	gem_rx_refill(queue);
> +	return gem_rx_refill(queue);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but it sits on the call chain
being changed here, so it may be worth a look: is the HRESP path allowed to
re-run gem_init_rx_ring()/gem_rx_refill() while an RX poll is in flight?

macb_hresp_error_task() runs in BH workqueue context and only masks the
device interrupt sources:

	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
		queue_writel(queue, IDR, bp->rx_intr_mask |
					 MACB_TX_INT_FLAGS |
					 MACB_BIT(HRESP));
	}
	...
	bp->macbgem_ops.mog_init_rings(bp);

There is no napi_disable() or napi_synchronize(), and neither side takes
bp->lock around queue->rx_tail, queue->rx_prepared_head, queue->rx_skbuff[]
or the descriptors.  Can this sequence happen?

CPU0, gem_rx() poll at entry E (hardware has completed that descriptor)

CPU1, HRESP worker, gem_rx_refill() sees rx_skbuff[E] still non-NULL:
	} else {
		desc->ctrl = 0;
		dma_wmb();
		desc->addr &= ~MACB_BIT(RX_USED);
	}

CPU0 then continues in gem_rx():
	queue->rx_skbuff[entry] = NULL;
	...
	dma_unmap_single(&bp->pdev->dev, addr,
			 bp->rx_buffer_size, DMA_FROM_DEVICE);

and passes the skb up.  Once macb_hresp_error_task() rewrites NCR with
RE|TE, does the DMA engine then write the next frame into a buffer that is
already unmapped and owned by the stack?  The unsynchronised resets of
rx_tail and rx_prepared_head to 0 against gem_rx()/gem_rx_refill()'s own
updates of the same counters also look like they could arm a descriptor
twice or leak an skb.

[ ... ]

> @@ -3162,7 +3175,9 @@ static int macb_open(struct net_device *netdev)
>  		goto pm_exit;
>  	}
>  
> -	bp->macbgem_ops.mog_init_rings(bp);
> +	err = bp->macbgem_ops.mog_init_rings(bp);
> +	if (err)
> +		goto free_rings;

[Severity: High]
This adds a new, fairly easy to hit macb_open() failure path that returns
before the napi_enable() loop.  Does macb_set_ringparam() need updating for
it?  It closes and reopens the interface and discards the result:

	if (netif_running(bp->netdev)) {
		reset = 1;
		macb_close(bp->netdev);
	}

	bp->rx_ring_size = new_rx_size;
	bp->tx_ring_size = new_tx_size;

	if (reset)
		macb_open(bp->netdev);

	return 0;

Because the ops are called directly rather than through __dev_open(), the
netdev stays marked running while the rings are freed and both NAPI
instances are left disabled.  The next close then calls napi_disable() on
instances that were never re-enabled, and napi_disable_locked() spins:

net/core/dev.c:napi_disable_locked() {
	...
	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}
	...
}

Nothing clears those bits without an intervening napi_enable(), so can an
"ethtool -G" resize that fails allocation under memory pressure leave the
next "ip link set dev down" looping forever with rtnl_lock held?

>  	macb_init_buffers(bp);
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> @@ -3200,6 +3215,7 @@ static int macb_open(struct net_device *netdev)
>  		napi_disable(&queue->napi_rx);
>  		napi_disable(&queue->napi_tx);
>  	}
> +free_rings:
>  	macb_free(bp);
>  pm_exit:
>  	pm_runtime_put_sync(&bp->pdev->dev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-close-v1-0-05e32ce98813%40bootlin.com

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

[Théo Lebrun]

The macb_close() operation is facing races as it disables IRQs late in
its sequence and keeps BH primitives alive while shutdown.

Non exhaustive list of races that could occur:

 - macb_tx_error_task() could be scheduled and access the buffers freed
   by macb_close().

 - macb_tx_error_task() or macb_hresp_error_task() might re-enable
   interrupts after the IDR write in macb_close().

 - macb_close() calls napi_disable() meaning that if
   macb_tx_error_task() occurs later, it will deadlock on napi_disable()
   that shouldn't be called if NAPI is already disabled.

 - macb_hresp_error_task() might reinit every RX/TX ring under
   macb_close()'s foot.

 - macb_interrupt() might re-enable NAPI just after it has been
   disabled by macb_close().

Instead, disable all our primitives one by one:
 - (1) mask and sync on IRQ handlers,
 - (2) drain any scheduled bp->hresp_err_bh_work,
 - (3) drain any scheduled queue->tx_error_task,
 - (4) drain queue->napi_rx/napi_tx,
 - (5) drain bp->tx_lpi_work.

Careful! Ordering is important because our scheduling primitives can
wake each other up. Recap table:

|               |   enable/disable   |           schedule            |
|               |----|-------|-------|------|----|----|--------|-----|
|               |IRQs|napi_tx|napi_rx|tx_lpi|napi|napi|tx_error|hresp|
| Context       |    |       |       | task | rx | tx |  task  |task |
|===============|====|=======|=======|======|====|====|========|=====|
| open          | X  |   X   |   X   |      |    |    |        |     |
| link_up       | X  |       |       |      |    |    |        |     |
| link_down     | X  |       |       |      |    |    |        |     |
| close         | X  |   X   |   X   |      |    |    |        |     |
| enable_tx_lpi |    |       |       |  X   |    |    |        |     |
| swap          | X  |   X   |   X   |  X   |    |    |        |     |
| suspend       | X  |   X   |   X   |      |    |    |        |     |
| resume        | X  |   X   |   X   |      |    |    |        |     |
|---------------|----|-------|-------|------|----|----|--------|-----|
| irq & netpoll | X  |       |       |      | X  | X  |   X    | X   |
|---------------|----|-------|-------|------|----|----|--------|-----|
| napi_rx       | X  |       |       |      | X  |    |        |     |
| napi_tx       | X  |       |       |  X   |    | X  |        |     |
|---------------|----|-------|-------|------|----|----|--------|-----|
| tx_error_task | X  |   X   |       |      |    |    |        |     |
| hresp task    | X  |       |       |      |    |    |        |     |

As example, one ordering constraint that can be deduced from the table:
napi_tx can schedule tx_lpi_task meaning napi_tx must be disabled
before tx_lpi_task, else we risk napi_tx re-enabling tx_lpi_task after
it has been disabled by macb_close().

We do *not* use IDR masking to shutdown IRQs because that risks
conflicting with BH primitives we have not disabled yet. For example if
we writel(IDR) in macb_close() and napi_rx is pending then IRQs might
be unmasked by the NAPI poll. As for why we do not use disable_irq():
we will have situations where we are quiesced but want to listen to
some IRQs and (minor reason) we register shared IRQ handlers so we
shouldn't disable the full IRQ line.

Instead we introduce a bool that tells macb_interrupt() to self-disarm.
Its default value is true as we start closed. It gets set to false
while interface is active. Reading into my crystal ball, we'll reuse
that flag in suspend/WOL, set_ringparam and change_mtu (context swap).

Note that old IRQ handler tried preventing a race with close by
self-disarming based on netif_running(). This might work, but it does
not prevent a race with the error codepath of macb_open() which needs
to run with IRQs dis-armed but netif_running() returns true during that
time.

Fixes: e86cd53afc59 ("net/macb: better manage tx errors")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h      |  5 ++
 drivers/net/ethernet/cadence/macb_main.c | 84 ++++++++++++++++++++++++--------
 2 files changed, 69 insertions(+), 20 deletions(-)

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);
+
+	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;
@@ -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:
@@ -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;
 
+	macb_quiesce_start(bp);
+
+	/* 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, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		napi_disable(&queue->napi_rx);
-		napi_disable(&queue->napi_tx);
+	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);
 
 	phylink_stop(bp->phylink);
 	phylink_disconnect_phy(bp->phylink);
@@ -4759,6 +4798,11 @@ static int macb_init_dflt(struct platform_device *pdev)
 	bp->tx_ring_size = DEFAULT_TX_RING_SIZE;
 	bp->rx_ring_size = DEFAULT_RX_RING_SIZE;
 
+	/* No locking needed because the IRQs are not requested yet. The
+	 * flag is cleared by macb_open() and re-armed by macb_close().
+	 */
+	bp->irq_quiesced = true;
+
 	/* set the queue register mapping once for all: queue0 has a special
 	 * register mapping but we don't want to test the queue index then
 	 * compute the corresponding register offset at run time.

-- 
2.55.0

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

[netdev-bot+sashiko]

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.

> 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?

> +
> +	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()?

macb_suspend() still open-codes the NAPI loop, with no bp->irq_quiesced
update, no synchronize_irq() and no draining of queue->tx_error_task or
bp->hresp_err_bh_work:

	netif_device_detach(netdev);
	for (q = 0, queue = bp->queues; q < bp->num_queues;
	     ++q, ++queue) {
		napi_disable(&queue->napi_rx);
		napi_disable(&queue->napi_tx);
	}

	if (!(bp->wol & MACB_WOL_ENABLED)) {
		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();
		...
		macb_reset_hw(bp);

In the non-WOL path the interrupt enables left by macb_mac_link_up()
(bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP)) are still unmasked
until that later macb_reset_hw(), and netif_device_detach() does not affect
netif_running(), so the ISR stays armed with bp->irq_quiesced == false.

A TX error interrupt in that window reaches macb_interrupt_misc():

	if (unlikely(status & (MACB_TX_ERR_FLAGS))) {
		queue_writel(queue, IDR, MACB_TX_INT_FLAGS);
		schedule_work(&queue->tx_error_task);

and macb_tx_error_task() begins with napi_disable(&queue->napi_tx) on a NAPI
that macb_suspend() already disabled. napi_disable_locked() then never
returns:

net/core/dev.c:napi_disable_locked() {
	...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}
	...
}

The worker wedges holding the netdev instance lock, so macb_resume()'s
napi_enable() blocks as well. The same window lets macb_hresp_error_task()
re-init every ring and rewrite IER/NCR/carrier behind suspend's back.

The new comment says "ISR must not drive NAPI & BH mechanisms. True when
the interface is closed.", and the commit message lists suspend/WOL as
future work ("we'll reuse that flag in suspend/WOL, set_ringparam and
change_mtu"). Given that the old netif_running() check is now gone, should
macb_suspend()/macb_resume() switch to macb_quiesce_start()/
macb_quiesce_end() in this patch rather than later?

> @@ -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?

macb_set_ringparam() drops the macb_open() return value entirely:

	if (netif_running(bp->netdev)) {
		reset = 1;
		macb_close(bp->netdev);
	}

	bp->rx_ring_size = new_rx_size;
	bp->tx_ring_size = new_tx_size;

	if (reset)
		macb_open(bp->netdev);

	return 0;

Failures before macb_quiesce_end() (pm_runtime_resume_and_get(),
macb_alloc()'s dma_alloc_coherent(), mog_init_rings() RX skb allocation)
return with the NAPI instances still disabled from the preceding
macb_close(). Failures after it now take the reset_hw label, which calls
macb_quiesce_start() and disables them a second time.

Since this path bypasses dev_open()/dev_close(), netif_running() stays
true, so the next ndo_stop ('ip link set dev ethX down', another
ethtool -G, or macb_suspend()) re-enters macb_close() ->
macb_quiesce_start() -> napi_disable() on an already disabled NAPI, and
napi_disable_locked() spins forever with RTNL and the netdev instance lock
held.

Should macb_set_ringparam() propagate macb_open()'s error, and should
macb_quiesce_start()/macb_quiesce_end() be made robust against being
called twice in a row?

> @@ -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?

macb_set_ringparam() calls macb_close() with the netdev up and the qdisc
still live:

	if (netif_running(bp->netdev)) {
		reset = 1;
		macb_close(bp->netdev);
	}

In that path macb_start_xmit() remains callable for the whole
macb_quiesce_start() window (synchronize_irq() per queue,
cancel_work_sync() on hresp_err_bh_work and each tx_error_task,
two napi_disable() per queue, cancel_delayed_work_sync() on tx_lpi_work),
since ethtool only holds RTNL.

Two consequences I am unsure about:

Frames mapped into the TX ring and kicked with NCR.TSTART after napi_tx is
disabled and after macb_interrupt() started answering with
queue_writel(queue, IDR, -1) will never be reaped; they are dropped by
macb_free()'s unmap loop and only counted in queue->stats.tx_dropped.

An xmit landing after netdev_tx_reset_queue()'s dql_reset() calls
netdev_tx_sent_queue() for completions that are never accounted, since
macb_free() does no BQL accounting and macb_open() does not reset BQL
again, so dql->num_queued > num_completed persists across the resize. Can
that leave the queue XOFF-ed?

The commit message explains the new ordering as "Drain the BH contexts
before stopping the queues: NAPI completion and tx_error_task wake them
up", and the recap table only lists 'swap' as a future context. Would
keeping netif_tx_stop_all_queues() first, or converting
macb_set_ringparam() to dev_close()/dev_open() semantics, address this?

>  	phylink_stop(bp->phylink);
>  	phylink_disconnect_phy(bp->phylink);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-close-v1-0-05e32ce98813%40bootlin.com