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

[PATCH net v2 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/

---
Changes in v2:
- P1: ensure gem_rx() and macb_rx_pending() don't consume the
  descriptors where alloc failed.
- P2: gem_rx_refill() now only propagates an error if zero descriptors
  are ready. We tolerate partial refill hoping the next one will help.
- P2: we do *NOT* fix the set_ringparam codepath reported by Sashiko.
  Proper implementation will come with context swapping which will
  reuse the infra put in place in P3.
- P3: the hresp_err_bh_work/tx_lpi_work Sashiko-reported race has been
  fixed in a standalone, *NOT* here.
  https://lore.kernel.org/netdev/20260925-macb-netdev-register-race-v1-1-752f90ce1c39@bootlin.com/
- Take 2x Reviewed-by: Nicolai.
- Rebase onto latest net/main (11536ee3d3e0), nothing to report.
- Link to v1: https://patch.msgid.link/20260918-macb-close-v1-0-05e32ce98813@bootlin.com
- Nicolai's v1 reviews (duplicate thread):
  https://patch.msgid.link/20260918-macb-close-v1-0-221d916b7961@bootlin.com

To: Conor Dooley <conor.dooley@microchip.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Simon Horman <horms@kernel.org>
To: Sean Anderson <sean.anderson@linux.dev>
To: Antoine Tenart <atenart@kernel.org>
To: Eric Dumazet <edumazet@kernel.org>
To: Nicolas Ferre <nicolas.ferre@microchip.com>
To: Russell King <linux@armlinux.org.uk>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Nicolai Buchwitz <nb@tipi-net.de>
Cc: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Tawfik Bayouk <tawfik.bayouk@mobileye.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Maxime Chevallier <maxime.chevallier@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 | 133 ++++++++++++++++++++++++-------
 2 files changed, 112 insertions(+), 28 deletions(-)
---
base-commit: 7c416e8f8423e6fe9b48e0b9c174bd902789f8db
change-id: 20260918-macb-close-dbbd23b4b8f2

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

[PATCH net v2 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 to the hardware by clearing
the RX_USED flag.

If the refill operation fails mid-way, it early returns leaving the
remaining slots untouched. In an initialised ring that is safe: a slot
is either owned by the hardware holding a valid buffer, or
software-owned (RX_USED set) waiting for refill to hand it a new one.
When slots have never 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.

Note we tweak gem_rx() and macb_rx_pending(): their previous stop
condition was only if desc was RX_USED. Now it must be either RX_USED
or we got out of the range of successfully allocated descriptors
(detected using the rx_tail and rx_prepared_head cursors).
Otherwise gem_rx() could consume unallocated 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 8e5c034dc3a4..7f25574928d7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1596,6 +1596,13 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
 		dma_addr_t addr;
 		bool rxused;
 
+		/* Only descriptors in [rx_tail, rx_prepared_head) were armed
+		 * for hardware. Outside, we might have RX_USED descriptors for
+		 * alloc failures.
+		 */
+		if (queue->rx_tail == queue->rx_prepared_head)
+			break;
+
 		entry = macb_rx_ring_wrap(bp, queue->rx_tail);
 		desc = macb_rx_desc(queue, entry);
 
@@ -1859,6 +1866,10 @@ static bool macb_rx_pending(struct macb_queue *queue)
 	struct macb_dma_desc *desc;
 	unsigned int entry;
 
+	/* No armed descriptor left: nothing can be pending. */
+	if (macb_is_gem(bp) && queue->rx_tail == queue->rx_prepared_head)
+		return false;
+
 	entry = macb_rx_ring_wrap(bp, queue->rx_tail);
 	desc = macb_rx_desc(queue, entry);
 
@@ -2795,9 +2806,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

[PATCH net v2 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 no descriptors
were allocated. 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()

We tolerate some allocation failures: we accept running with the rx ring
only partially filled with successful descriptors. It is important we
refuse the zero-valid-descriptor case: nothing would ever trigger a
refill, which only happens once a frame has been received.

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 unusable because it has
zero usable rx buffers.

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 | 33 +++++++++++++++++++++++++-------
 2 files changed, 27 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 7f25574928d7..18a1b5f7ad91 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1486,7 +1486,7 @@ 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;
@@ -1558,6 +1558,14 @@ 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);
+
+	/* Fail if queue has zero prepared descriptors. This is critical because
+	 * nothing will ever trigger a refill again.
+	 */
+	if (queue->rx_prepared_head == queue->rx_tail)
+		return -ENOMEM;
+
+	return 0;
 }
 
 /* Mark DMA descriptors from begin up to and not including end as unused */
@@ -2804,7 +2812,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;
 
@@ -2814,14 +2822,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) {
@@ -2834,11 +2844,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;
@@ -2853,6 +2867,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)
@@ -3183,7 +3199,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) {
@@ -3221,6 +3239,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

[PATCH net v2 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
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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 18a1b5f7ad91..1d6361c0d8bc 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2009,6 +2009,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);
@@ -2151,8 +2198,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;
@@ -3179,8 +3226,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");
@@ -3204,10 +3249,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);
 
@@ -3234,11 +3276,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:
@@ -3249,19 +3290,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);
@@ -4783,6 +4822,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 v2 2/3] net: macb: propagate RX ring refill errors

[Nicolai Buchwitz]

On 25.9.2026 15:59, Théo Lebrun wrote:
> 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 no descriptors
> were allocated. 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()
> 
> We tolerate some allocation failures: we accept running with the rx 
> ring
> only partially filled with successful descriptors. It is important we
> refuse the zero-valid-descriptor case: nothing would ever trigger a
> refill, which only happens once a frame has been received.
> 
> 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 unusable because it has
> zero usable rx buffers.
> 
> 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>
> ---

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (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] RX can stall permanently once no descriptor is armed (rx_tail ==
  rx_prepared_head) and refill has failed.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  widens it.

--

net: macb: never give hardware a NULL RX buffer

This patch sets RX_USED on every GEM RX descriptor in gem_init_rx_ring()
before the refill. A failed allocation or DMA mapping then cannot leave
hardware-owned slots pointing at NULL buffers. gem_rx() and
macb_rx_pending() now also stop once rx_tail reaches rx_prepared_head.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 8e5c034dc3a48..7f25574928d71 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1859,6 +1866,10 @@ static bool macb_rx_pending(struct macb_queue *queue)
>  	struct macb_dma_desc *desc;
>  	unsigned int entry;
>  
> +	/* No armed descriptor left: nothing can be pending. */
> +	if (macb_is_gem(bp) && queue->rx_tail == queue->rx_prepared_head)
> +		return false;
> +

[Severity: High]
Can RX on a GEM queue stall for good once every armed descriptor has
been consumed and gem_rx_refill() keeps failing?

Under memory pressure, the sequence would be:

gem_rx()
    rx_tail == rx_prepared_head -> break
    gem_rx_refill()
        netdev_alloc_skb() or dma_map_single() fails -> break
macb_rx_poll()
    napi_complete_done()
    queue_writel(queue, IER, bp->rx_intr_mask)
    macb_rx_pending()
        rx_tail == rx_prepared_head -> return false
    (no napi_schedule())

At that point every descriptor has RX_USED set. The hardware cannot
complete a frame, so RCOMP cannot fire. gem_rx_refill() is only called
from gem_rx() and gem_init_rx_ring(), and no timer or work item retries
it.

bp->rx_intr_mask is MACB_RX_INT_FLAGS. RXUBR is only added for
MACB_CAPS_NEEDS_RSTONUBR parts:

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

Zynq and the EMAC would still get a retry from macb_interrupt() on each
incoming frame through RXUBR.

On other GEM variants, is anything left to restart NAPI apart from
ISR_ROVR, if the controller raises it at all in this state? If not, RX
on that queue would stay dead after memory recovers, until the
interface is closed and reopened.

Before this patch, macb_rx_pending() returned true for the stale
RX_USED slot at rx_tail. NAPI kept rescheduling itself and so kept
retrying gem_rx_refill().

Later in the series, "net: macb: propagate RX ring refill errors"
handles the open-time case where nothing gets armed, by making
macb_open() fail. That commit leaves NAPI, resume and HRESP refill as
best effort. Its message also notes that with zero armed descriptors
nothing would ever trigger a refill. So this steady-state case seems
to remain at the end of the series.

[ ... ]

> @@ -2795,9 +2806,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);
>  }

[Severity: High]
This is a pre-existing issue, but this loop seems to widen it.
macb_hresp_error_task() reinitialises every RX ring without disabling
or synchronising RX NAPI:

macb_hresp_error_task()
    queue_writel(queue, IDR, ...)
    macb_writel(bp, NCR, ctrl & ~(RE | TE))
    bp->macbgem_ops.mog_init_rings(bp)
        gem_init_rx_ring()

It calls neither napi_disable() nor napi_synchronize(), and takes no
lock that gem_rx() also takes. With per-queue IRQs on different CPUs,
or with threaded NAPI, a gem_rx() poll already running on another CPU
keeps touching rx_tail, rx_prepared_head, the descriptors and
rx_skbuff[] at the same time.

The new loop ORs RX_USED into descriptors the hardware still owns
(valid buffer, ctrl == 0). A concurrent gem_rx() may have passed the
cursor check using stale values. Could it then see RX_USED with
ctrl == 0, log "not whole frame pointed by descriptor", and do
rx_tail++?

Suppose that increment lands after rx_tail = 0 but before
gem_rx_refill() runs. CIRC_SPACE(0, 1, rx_ring_size) is then 0, so
refill arms nothing. The whole ring is left RX_USED and RX stalls.

The same race already existed in other forms before this patch. Both
CPUs can run gem_rx_refill() on the same NULL rx_skbuff[] entry, and
each allocates and maps an skb. That either leaks one skb or leaves
desc->addr and rx_skbuff[] out of sync.

The HRESP refill can also take the rx_skbuff[entry] != NULL branch in
gem_rx_refill() and clear RX_USED on a descriptor whose skb gem_rx() on
the other CPU is about to unmap and hand to napi_gro_receive(). Once RE
is re-enabled, wouldn't the hardware DMA into an skb that the stack
already owns?

Later in the series, "net: macb: quiesce IRQs and drain BH on interface
close" only covers close and the open error path.
macb_hresp_error_task() looks unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-macb-close-v2-0-360efa565914%40bootlin.com

Re: [PATCH net v2 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: 1 (1) · Medium: 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).

- [High] The patch adds a failure path to macb_open(): if
  mog_init_rings() (gem_init_rings()) finds any queue with zero prepared…
- [Medium] The patch treats a GEM RX queue with zero prepared descriptors
  as unrecoverable.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  macb_hresp_error_task() re-initialises every RX ring with no exclusion…

--

net: macb: propagate RX ring refill errors

This commit changes gem_rx_refill(), gem_init_rx_ring(), gem_init_rings(),
macb_init_rings() and the mog_init_rings op so they return errno-style ints.
gem_rx_refill() now returns -ENOMEM when a queue ends up with zero prepared
RX descriptors, and macb_open() fails in that case. The other refill callers
(resume, the HRESP error task and NAPI) still ignore the result.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 7f25574928d71..18a1b5f7ad916 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1558,6 +1558,14 @@ 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);
> +
> +	/* Fail if queue has zero prepared descriptors. This is critical because
> +	 * nothing will ever trigger a refill again.
> +	 */
> +	if (queue->rx_prepared_head == queue->rx_tail)
> +		return -ENOMEM;
> +
> +	return 0;
>  }

[Severity: Medium]

This comment says a queue with zero prepared descriptors can never be
refilled. What happens when the other callers reach that state? Only
macb_open() checks the new return value. The other three paths drop it:

gem_rx()
    gem_rx_refill(queue);

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

macb_resume()
    if (macb_is_gem(bp))
        gem_init_rx_ring(queue);

gem_init_rx_ring() resets rx_tail and rx_prepared_head to 0 and sets RX_USED
on every descriptor. gem_rx_refill() stops at the first failed
netdev_alloc_skb() or dma_map_single(). So resume and HRESP recovery end up
with zero armed descriptors whenever slot 0 has no skb and its allocation
fails.

In the NAPI path, gem_rx() can drain up to rx_prepared_head, and then the
end-of-poll refill can fail on the first empty slot.

Once there, macb_rx_pending() returns early:

    if (macb_is_gem(bp) && queue->rx_tail == queue->rx_prepared_head)
        return false;

That means macb_rx_poll() does not reschedule NAPI. RCOMP also cannot fire
while every descriptor is software-owned. On top of that,
macb_hresp_error_task() goes on to set NCR.RE|TE and call
netif_carrier_on() and netif_tx_start_all_queues() on that ring.

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."

Doesn't the new comment say the opposite, that nothing will trigger another
refill? Would the queue then stay dead until the interface is taken down and
brought back up, while carrier and TX still report up? On a single-queue GEM
that would be all RX.

One caveat: ISR_ROVR is part of rx_intr_mask, and macb_interrupt() schedules
napi_rx on it. If the hardware raises receive-overrun while no descriptors
are available, NAPI would run and retry the refill, which would contradict
the new comment. The code alone doesn't settle this.

[ ... ]

> @@ -2814,14 +2822,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)

[ ... ]

> @@ -2834,11 +2844,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;
>  }

[Severity: High]

This is a pre-existing issue, not one introduced by this patch.
macb_hresp_error_task() re-initialises every RX ring through this helper,
and nothing stops a NAPI RX poll from running at the same time.

macb_interrupt_misc() queues the task on system_bh_wq, on whichever CPU took
the HRESP interrupt:

    queue_work(system_bh_wq, &bp->hresp_err_bh_work);

The task masks interrupts through IDR and clears NCR.RE/TE. It then calls
mog_init_rings() -> gem_init_rx_ring() -> gem_rx_refill(). That resets
rx_tail and rx_prepared_head and re-arms every slot whose rx_skbuff[entry]
is non-NULL.

Meanwhile, gem_rx() for another queue (or threaded NAPI) can be running on
another CPU:

    skb = queue->rx_skbuff[entry];
    ...
    queue->rx_skbuff[entry] = NULL;

After that it unmaps the buffer, passes the skb to napi_gro_receive(), and
calls gem_rx_refill() itself.

Neither path takes bp->lock, and the task never calls napi_disable() or
otherwise synchronises with NAPI. Can the HRESP refill see a stale non-NULL
rx_skbuff[entry] and clear RX_USED on a descriptor whose buffer was just
unmapped and handed to the stack?

If so, the MAC would DMA into memory owned by the network stack once NCR.RE
is set again. The two refills could also both allocate for the same NULL
slot, leaking one skb and its DMA mapping.

This looks unresolved at the end of the series too. "net: macb: quiesce
IRQs and drain BH on interface close" drains this work only in close and in
the open error path. macb_hresp_error_task() still does not disable or
synchronise with NAPI.

[ ... ]

> @@ -3183,7 +3199,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) {

[Severity: High]

Could this new failure path turn a ring size change into a hang on the next
ifdown? On a running interface, macb_set_ringparam() does:

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

    return 0;

It ignores the return value of macb_open(). By this point macb_close() has
already called napi_disable() on every napi_rx/napi_tx, plus macb_free()
and pm_runtime_put().

If mog_init_rings() now fails here, goto free_rings skips the napi_enable()
loop. NAPI stays disabled, but the netdev still has IFF_UP and
__LINK_STATE_START set.

The next "ip link set dev X down" calls macb_close() again. Its
napi_disable() ends up in napi_disable_locked():

    while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
        usleep_range(20, 200);
        val = READ_ONCE(n->state);
    }

Won't this loop forever while holding RTNL? The second macb_close() would
also call pm_runtime_put() again and underflow the runtime PM usage count.

Before this patch, the same GFP_ATOMIC netdev_alloc_skb() failure let
macb_open() succeed with one degraded queue. The unchecked macb_open() in
macb_set_ringparam() could already be hit through macb_alloc() failures,
but this new failure path makes it much more likely.

This also looks unresolved at the end of the series. macb_set_ringparam()
still ignores the return value of macb_open(), and macb_close() ->
macb_quiesce_start() still calls napi_disable() on the already-disabled
NAPI.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-macb-close-v2-0-360efa565914%40bootlin.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] macb_probe() initializes bp->hresp_err_bh_work and
  bp->tx_lpi_work (lines 6056-6057) only after register_netdev() (line…

Pre-existing issues:
- [High] The suspend path still has the same tx_error_task vs. NAPI
  deadlock that this patch fixes for close.
- [High] macb_set_ringparam()
  (drivers/net/ethernet/cadence/macb_main.c:3801-3812) calls…
- [High] macb_set_ringparam() (line 3803) calls macb_close() directly,
  holding only RTNL, so the core's…

--

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

This patch adds an irq_quiesced flag. macb_interrupt() checks it and
disarms itself while the interface is closed. It also adds
macb_quiesce_start() and macb_quiesce_end(). macb_quiesce_start() drains
the IRQ handlers, hresp_err_bh_work, tx_error_task, NAPI and tx_lpi_work,
in that order. macb_close() and the macb_open() error path now use these
helpers.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 18a1b5f7ad916..1d6361c0d8bc9 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2009,6 +2009,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: Low]
macb_probe() initializes these works only after register_netdev() has
returned:

drivers/net/ethernet/cadence/macb_main.c:macb_probe() {
    ...
	err = register_netdev(netdev);
	...
	INIT_WORK(&bp->hresp_err_bh_work, macb_hresp_error_task);
	INIT_DELAYED_WORK(&bp->tx_lpi_work, macb_tx_lpi_work_fn);
    ...
}

Userspace can open the interface inside that window, for example in
response to the registration uevent. Suppose the open then fails in
phy_set_mode_ext(), phy_power_on() or macb_phylink_connect(), or the
interface is closed right away. Can cancel_work_sync() here then trigger
the WARN_ON(!work->func) in __flush_work() on a work that is still
zeroed?

The close path already cancelled tx_lpi_work before this patch. The new
parts are the hresp_err_bh_work cancel and the cancels on the macb_open()
error path.

Would moving INIT_WORK() and INIT_DELAYED_WORK() before register_netdev(),
or into macb_init_dflt(), avoid this?

> +
> +	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);
> +	}

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
macb_suspend() still seems to have the same tx_error_task vs. NAPI
deadlock that this patch fixes for close. It disables NAPI without
setting irq_quiesced and without cancelling tx_error_task or
hresp_err_bh_work:

drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
    ...
	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);
	}
    ...
}

In the non-WoL branch the IRQs are still unmasked at this point. On a TX
error, macb_interrupt()->macb_interrupt_misc() can still call
schedule_work(&queue->tx_error_task). On HRESP it can queue
hresp_err_bh_work.

In the WoL branch, a tx_error_task queued just before the IDR write can
also run after the napi_disable() calls.

macb_tx_error_task() then calls this on a NAPI that is already disabled:

	napi_disable(&queue->napi_tx);

Would that spin forever in napi_disable_locked() while holding
netdev_lock? macb_resume() then calls napi_enable(), which takes the same
netdev_lock, so it looks like resume would never complete.

The commit message says the flag will be reused for suspend/WOL, and the
patch is Cc: stable for this class of deadlock. Could macb_suspend() and
macb_resume() use macb_quiesce_start() and macb_quiesce_end() too?

[ ... ]

> @@ -3234,11 +3276,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);

[Severity: High]
This isn't a bug introduced by this patch, but macb_set_ringparam() calls
macb_close() and macb_open() directly and ignores macb_open()'s return
value:

drivers/net/ethernet/cadence/macb_main.c:macb_set_ringparam() {
    ...
	if (netif_running(bp->netdev)) {
		reset = 1;
		macb_close(bp->netdev);
	}
    ...
	if (reset)
		macb_open(bp->netdev);

	return 0;
}

Every macb_open() failure path leaves NAPI disabled. The macb_alloc() and
mog_init_rings() failures jump past macb_quiesce_end(). The later failures
call macb_quiesce_start() here.

These paths also free the rings and drop the runtime PM reference, but
__LINK_STATE_START stays set. A large ring size can make macb_alloc() fail
under memory pressure, and ethtool -G still reports success.

A later ip link set down, or another ethtool -G, calls
macb_close()->macb_quiesce_start() again. Does napi_disable() then wait
forever in napi_disable_locked() with RTNL held? If that step were somehow
passed, would macb_free() and pm_runtime_put() run a second time?

This also doesn't match the new comments. They describe irq_quiesced as
"True when the interface is closed" and say it is "cleared by macb_open()
and re-armed by macb_close()".

> +
>  	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:
> @@ -3249,19 +3290,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);

[Severity: High]
This is a pre-existing issue, but it is about draining contexts before
teardown. macb_set_ringparam() calls macb_close() directly under RTNL, so
the core's dev_deactivate_many()/synchronize_net() never runs to drain an
ndo_start_xmit that is already in flight.

netif_tx_stop_all_queues() only sets the XOFF bits. It doesn't take the
xmit lock, and it doesn't wait for a macb_start_xmit() that has already
passed the queue-stopped check.

macb_start_xmit() writes queue->tx_skb and the TX descriptors under
queue->tx_ptr_lock. Neither macb_close() nor macb_free() takes that lock
before this:

drivers/net/ethernet/cadence/macb_main.c:macb_free() {
    ...
			kfree(queue->tx_skb);
			queue->tx_skb = NULL;
    ...
}

The same applies to the dma_free_coherent() of the TX ring.

Can an xmit that is still running during ethtool -G then write into the
freed tx_skb array or the freed TX ring? This seems more likely on
PREEMPT_RT, where the xmit path and tx_ptr_lock can be preempted.

Would netif_tx_disable() or an explicit synchronize_net() here close this
window?

> +	for (q = 0; q < bp->num_queues; ++q)
>  		netdev_tx_reset_queue(netdev_get_tx_queue(netdev, q));

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-macb-close-v2-0-360efa565914%40bootlin.com