[PATCH net v3 0/2] net: nixge: fix tasklet and DMA teardown

[PATCH net v3 0/2] net: nixge: fix tasklet and DMA teardown

[Myeonghun Pak]

Separate the IRQ/tasklet ordering fix and the DMA shutdown fix from
v2, as requested by Daniel Machon.

Patch 1 releases both IRQ handlers before draining dma_err_tasklet.
Patch 2 masks channel interrupts after recovery has stopped and waits
for DMA reset completion before releasing the buffers. On reset timeout,
it retains those buffers and prevents a later open from replacing them.

Changes in v3:
- Split the IRQ/tasklet ordering fix and DMA shutdown fix into two patches.
- Give each fix its own subject and commit message.
- Order the nixge_stop() local declarations longest to shortest (RXT).

Changes in v2:
- Wait for DMA reset completion; retain buffers and reject reopen on timeout.

v2: https://lore.kernel.org/netdev/20261004042702.1130389-1-mhun512@gmail.com/
Review: https://lore.kernel.org/netdev/20261007104833.4ukkys7rnmv7ki5i@DEN-DL-M70577.microsemi.net/

Based on net commit 6dc989ea46b96ce170840174b4a38c4a387fb005.
Found by code inspection. No hardware testing.


Myeonghun Pak (2):
  net: nixge: free IRQs before killing the DMA error tasklet
  net: nixge: wait for DMA reset before releasing buffers

 drivers/net/ethernet/ni/nixge.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

base-commit: 6dc989ea46b96ce170840174b4a38c4a387fb005
-- 
2.53.0

[PATCH net v3 1/2] net: nixge: free IRQs before killing the DMA error tasklet

[Myeonghun Pak]

The TX and RX IRQ handlers can schedule dma_err_tasklet. Killing the
tasklet before freeing the IRQs leaves a window for either handler to
schedule it again. The tasklet can then access descriptors and TX skb
state after nixge_stop() releases them.

Free both IRQs before tasklet_kill() so the IRQ handlers have finished
and cannot queue further error recovery when the tasklet is drained.

Fixes: 492caffa8a1a ("net: ethernet: nixge: Add support for National Instruments XGE netdev")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/net/ethernet/ni/nixge.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 230d5ff..8697a76 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -927,11 +927,12 @@ static int nixge_stop(struct net_device *ndev)
 	nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
 			    cr & (~XAXIDMA_CR_RUNSTOP_MASK));
 
-	tasklet_kill(&priv->dma_err_tasklet);
-
+	/* Remove both producers before draining the error tasklet. */
 	free_irq(priv->tx_irq, ndev);
 	free_irq(priv->rx_irq, ndev);
 
+	tasklet_kill(&priv->dma_err_tasklet);
+
 	nixge_hw_dma_bd_release(ndev);
 
 	return 0;
-- 
2.53.0

Re: [PATCH net v3 1/2] net: nixge: free IRQs before killing the DMA error tasklet

[netdev-bot+sashiko]

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

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

- [Critical] In this commit, nixge_stop() in
  drivers/net/ethernet/ni/nixge.c clears RUNSTOP on both channels (lines…

Pre-existing issues:
- [High] nixge_dma_err_handler() runs as a tasklet (softirq, atomic
  context).
- [High] nixge_hw_dma_bd_release() frees priv->rx_bd_v, its RX skbs,
  priv->tx_skb and priv->tx_bd_v, but never sets those pointers to NULL.
- [High] nixge_dma_err_handler() takes no lock and does not quiesce NAPI
  or the TX queue.

--

net: nixge: free IRQs before killing the DMA error tasklet

This patch reorders nixge_stop() so the TX and RX IRQs are freed before
tasklet_kill() runs on dma_err_tasklet. The aim is to stop the IRQ
handlers from rescheduling the error tasklet after it has been drained
and the descriptor rings have been released.

> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 230d5ff99dd7e..8697a76241394 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -927,11 +927,12 @@ static int nixge_stop(struct net_device *ndev)
>  	nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
>  			    cr & (~XAXIDMA_CR_RUNSTOP_MASK));
>  
> -	tasklet_kill(&priv->dma_err_tasklet);
> -
> +	/* Remove both producers before draining the error tasklet. */
>  	free_irq(priv->tx_irq, ndev);
>  	free_irq(priv->rx_irq, ndev);
>  
> +	tasklet_kill(&priv->dma_err_tasklet);
> +
>  	nixge_hw_dma_bd_release(ndev);

[Severity: Critical]
Can the error tasklet restart the DMA engine here, after RUNSTOP has
already been cleared above?

Suppose nixge_tx_irq() or nixge_rx_irq() scheduled dma_err_tasklet
before free_irq(). Then tasklet_kill() waits for it to run to
completion. nixge_dma_err_handler() resets the AXI DMA, re-enables
XAXIDMA_IRQ_ALL_MASK, and sets RUNSTOP on both channels again. The RX
channel ends up owning the whole RX ring:

nixge_dma_err_handler() {
    ...
	nixge_dma_write_desc_reg(lp, XAXIDMA_RX_CDESC_OFFSET, lp->rx_bd_p);
	cr = nixge_dma_read_reg(lp, XAXIDMA_RX_CR_OFFSET);
	nixge_dma_write_reg(lp, XAXIDMA_RX_CR_OFFSET,
			    cr | XAXIDMA_CR_RUNSTOP_MASK);
	nixge_dma_write_desc_reg(lp, XAXIDMA_RX_TDESC_OFFSET, lp->rx_bd_p +
			    (sizeof(*lp->rx_bd_v) * (RX_BD_NUM - 1)));
    ...
	nixge_dma_write_reg(lp, XAXIDMA_TX_CR_OFFSET,
			    cr | XAXIDMA_CR_RUNSTOP_MASK);
}

Nothing stops the engine again before nixge_hw_dma_bd_release() unmaps
and frees the RX skbs and calls dma_free_coherent() on both rings.

Could the device then DMA received frames into freed memory, or fetch
freed descriptors?

The old ordering (RUNSTOP clear, tasklet_kill(), free_irq()) had the
same restart window, so the reordering does not create it. The next
patch in this series, "net: nixge: wait for DMA reset before releasing
buffers", looks like it closes the window. After tasklet_kill() it
clears RUNSTOP and IRQ_ALL on both channels again and does a polled AXI
DMA reset before the release. If the reset fails, it keeps the buffers.

>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but nixge_stop() now relies
on draining dma_err_tasklet. Is it safe for that tasklet to call a
sleeping poll helper?

nixge_dma_err_handler() runs in softirq context and calls
__nixge_device_reset() for both channels, which does:

__nixge_device_reset() {
    ...
	err = nixge_dma_poll_timeout(priv, offset, status,
				     !(status & XAXIDMA_CR_RESET_MASK), 10,
				     1000);
    ...
}

nixge_dma_poll_timeout() wraps readl_poll_timeout(). With a nonzero
sleep_us, poll_timeout_us() calls might_sleep_if() and then
usleep_range() between polls.

Wouldn't every error recovery run trigger a "sleeping function called
from invalid context" warning? And if the reset bit has not cleared on
the first read, wouldn't it hit "scheduling while atomic"?

Using readl_poll_timeout_atomic(), or moving recovery into a work item,
would avoid this. The problem goes back to the driver's introduction.

[Severity: High]
This is a pre-existing issue, but nixge_hw_dma_bd_release(), called
from nixge_stop() above, frees priv->rx_bd_v, the RX skbs, priv->tx_skb
and priv->tx_bd_v. It never sets those pointers to NULL.

nixge_hw_dma_bd_init() assigns the pointers one at a time and calls
nixge_hw_dma_bd_release() on failure:

nixge_hw_dma_bd_init() {
    ...
	priv->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
					   sizeof(*priv->tx_bd_v) * TX_BD_NUM,
					   &priv->tx_bd_p, GFP_KERNEL);
	if (!priv->tx_bd_v)
		goto out;
    ...
out:
	nixge_hw_dma_bd_release(ndev);
	return -ENOMEM;
}

Consider an open/stop cycle followed by a second open where the tx_bd_v
or tx_skb allocation fails. At that point priv->rx_bd_v still points at
the ring that was already freed.

Would release then read freed descriptor memory, call
dma_unmap_single() on stale addresses, call dev_kfree_skb() on skbs
that were already freed, and call dma_free_coherent() on the RX ring a
second time?

A stale tx_skb would also reach devm_kfree() for a resource that no
longer exists, which triggers the WARN_ON in devres_destroy().

nixge_device_reset() only logs the -ENOMEM:

	if (nixge_hw_dma_bd_init(ndev))
		netdev_err(ndev, "%s: descriptor allocation failed\n",
			   __func__);

So nixge_open() carries on with tx_bd_v == NULL or with freed rings.
Couldn't nixge_start_xmit() then dereference priv->tx_bd_v, and the
next nixge_stop() release the same stale pointers again?

[Severity: High]
This isn't a bug introduced by this patch, but nixge_dma_err_handler()
takes no lock and does not quiesce NAPI or the TX queue while it walks
the rings:

nixge_dma_err_handler() {
    ...
	for (i = 0; i < TX_BD_NUM; i++) {
		cur_p = &lp->tx_bd_v[i];
		tx_skb = &lp->tx_skb[i];
		nixge_tx_skb_unmap(lp, tx_skb);
    ...
	lp->tx_bd_ci = 0;
	lp->tx_bd_tail = 0;
	lp->rx_bd_ci = 0;
    ...
}

TX and RX use separate IRQ lines. An RX error can schedule the tasklet
while nixge_tx_irq()->nixge_start_xmit_done() is running on another
CPU. In that case both sides call nixge_tx_skb_unmap() on the same
entries:

nixge_tx_skb_unmap() {
    ...
	if (tx_skb->skb) {
		dev_kfree_skb_any(tx_skb->skb);
		tx_skb->skb = NULL;
	}
}

Can both sides pass the tx_skb->skb check and free the same skb twice,
or unmap the same mapping twice? Clearing XAXIDMA_IRQ_ALL_MASK in the
error path only blocks future interrupts. It does not stop a handler
that is already running.

nixge_start_xmit() has the same exposure. Only the netdev TX lock
serializes it, so it can fill tx_skb[] and advance tx_bd_tail while the
tasklet clears and resets them. nixge_recv() under NAPI can also
advance rx_bd_ci while the tasklet resets it.

Could this free an skb that was just queued, or leave the ring indices
out of sync?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018020840.3851552-1-mhun512%40gmail.com

[PATCH net v3 2/2] net: nixge: wait for DMA reset before releasing buffers

[Myeonghun Pak]

nixge_stop() clears RUNSTOP before releasing the DMA buffers, but this
does not ensure that outstanding DMA accesses have completed. Pending
error recovery can also restart both channels and enable interrupts
while the stop path drains dma_err_tasklet.

After both IRQ handlers and the error tasklet have been drained, mask
channel interrupts and clear RUNSTOP again. Reset the DMA engine and
wait for reset completion before releasing the buffers. Return the
reset result from __nixge_device_reset() so the stop path can check it.

If reset times out, retain the DMA buffers and reject subsequent opens
to prevent those buffers from being freed or their pointers overwritten
while DMA may still be using them.

Fixes: 492caffa8a1a ("net: ethernet: nixge: Add support for National Instruments XGE netdev")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/net/ethernet/ni/nixge.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 8697a76..8bfda27 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -185,6 +185,7 @@ struct nixge_priv {
 	void __iomem *dma_regs;
 
 	struct tasklet_struct dma_err_tasklet;
+	bool dma_stop_failed;
 
 	int tx_irq;
 	int rx_irq;
@@ -391,7 +392,7 @@ out:
 	return -ENOMEM;
 }
 
-static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
+static int __nixge_device_reset(struct nixge_priv *priv, off_t offset)
 {
 	u32 status;
 	int err;
@@ -407,6 +408,8 @@ static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
 				     1000);
 	if (err)
 		netdev_err(priv->ndev, "%s: DMA reset timeout!\n", __func__);
+
+	return err;
 }
 
 static void nixge_device_reset(struct net_device *ndev)
@@ -869,6 +872,10 @@ static int nixge_open(struct net_device *ndev)
 	struct phy_device *phy;
 	int ret;
 
+	/* A failed stop retained buffers which DMA may still be using. */
+	if (priv->dma_stop_failed)
+		return -EIO;
+
 	nixge_device_reset(ndev);
 
 	phy = of_phy_connect(ndev, priv->phy_node,
@@ -910,6 +917,7 @@ err_tx_irq:
 static int nixge_stop(struct net_device *ndev)
 {
 	struct nixge_priv *priv = netdev_priv(ndev);
+	int ret;
 	u32 cr;
 
 	netif_stop_queue(ndev);
@@ -920,6 +928,7 @@ static int nixge_stop(struct net_device *ndev)
 		phy_disconnect(ndev->phydev);
 	}
 
+	/* Stop DMA while the completion IRQ handlers are still installed. */
 	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
 	nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
 			    cr & (~XAXIDMA_CR_RUNSTOP_MASK));
@@ -933,6 +942,23 @@ static int nixge_stop(struct net_device *ndev)
 
 	tasklet_kill(&priv->dma_err_tasklet);
 
+	/* Error recovery may have restarted DMA and enabled interrupts. */
+	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
+	nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
+			    cr & ~(XAXIDMA_CR_RUNSTOP_MASK | XAXIDMA_IRQ_ALL_MASK));
+	cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET);
+	nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
+			    cr & ~(XAXIDMA_CR_RUNSTOP_MASK | XAXIDMA_IRQ_ALL_MASK));
+
+	/* Either channel's reset quiesces the entire AXI DMA engine. */
+	ret = __nixge_device_reset(priv, XAXIDMA_TX_CR_OFFSET);
+	if (ret) {
+		/* Retain DMA buffers and prevent a subsequent open replacing them. */
+		priv->dma_stop_failed = true;
+		netdev_err(ndev, "DMA stop failed; retaining DMA buffers\n");
+		return ret;
+	}
+
 	nixge_hw_dma_bd_release(ndev);
 
 	return 0;
-- 
2.53.0

Re: [PATCH net v3 2/2] net: nixge: wait for DMA reset before releasing buffers

[netdev-bot+sashiko]

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

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

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 only partly closes the hole it describes.
- [Medium] One reset timeout in nixge_stop() now disables the port for
  good, and nothing in the driver can recover it.
- [Low] On reset timeout, nixge_stop() now returns the error from
  ndo_stop (line 959).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the DMA error
  recovery tasklet sleeps in atomic context.
- [High] This problem wasn't introduced by this patch, but stale ring
  pointers can be freed twice.
- [High] This problem wasn't introduced by this patch, but DMA error
  recovery races with the data path.
- [Medium] This problem wasn't introduced by this patch, but
  nixge_open()'s error paths leave the DMA engine running and leak the…
- [Medium] This problem wasn't introduced by this patch, but in-flight TX
  skbs and their DMA mappings leak on every stop.

--

net: nixge: wait for DMA reset before releasing buffers

After the IRQ handlers and the error tasklet have been drained, this patch
makes nixge_stop() mask the channel interrupts and clear RUNSTOP a second
time. It then resets the AXI DMA engine and waits for the reset to finish
before releasing the DMA buffers. __nixge_device_reset() now returns the
poll result. On a timeout, the buffers are kept and a new dma_stop_failed
flag makes later opens fail.

> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 8697a76241394..8bfda27741b26 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c

[ ... ]

> @@ -391,7 +392,7 @@ static int nixge_hw_dma_bd_init(struct net_device *ndev)
>  	return -ENOMEM;
>  }
>  
> -static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
> +static int __nixge_device_reset(struct nixge_priv *priv, off_t offset)
>  {
>  	u32 status;
>  	int err;
> @@ -407,6 +408,8 @@ static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
>  				     1000);
>  	if (err)
>  		netdev_err(priv->ndev, "%s: DMA reset timeout!\n", __func__);
> +
> +	return err;
>  }

[Severity: High]
This is a pre-existing issue, but since this helper is being changed: can
__nixge_device_reset() sleep in softirq context?

The poll uses a non-zero sleep_us:

	err = nixge_dma_poll_timeout(priv, offset, status,
				     !(status & XAXIDMA_CR_RESET_MASK), 10,
				     1000);

nixge_dma_poll_timeout() wraps readl_poll_timeout(). That does
might_sleep_if(sleep_us != 0) and calls usleep_range() between polls.

nixge_dma_err_handler() is a tasklet scheduled from nixge_tx_irq() and
nixge_rx_irq(), and it starts with:

	__nixge_device_reset(lp, XAXIDMA_TX_CR_OFFSET);
	__nixge_device_reset(lp, XAXIDMA_RX_CR_OFFSET);

So any DMA error interrupt leads to:

nixge_tx_irq() / nixge_rx_irq()
  tasklet_schedule()
    nixge_dma_err_handler()
      __nixge_device_reset()
        readl_poll_timeout(..., 10, 1000)
          usleep_range()

Wouldn't this trigger "scheduling while atomic" whenever the reset bit is
still set on the first read? Would readl_poll_timeout_atomic() on the
tasklet path, or moving recovery to a workqueue, fit better?

[Severity: High]
This is also a pre-existing issue that this patch didn't introduce: does
nixge_dma_err_handler() race with the data path?

The tasklet rebuilds the rings without holding a lock:

	for (i = 0; i < TX_BD_NUM; i++) {
		cur_p = &lp->tx_bd_v[i];
		tx_skb = &lp->tx_skb[i];
		nixge_tx_skb_unmap(lp, tx_skb);
	[ ... ]
	lp->tx_bd_ci = 0;
	lp->tx_bd_tail = 0;
	lp->rx_bd_ci = 0;

It then does read-modify-writes of RX_CR and TX_CR.

Before scheduling it, nixge_tx_irq() and nixge_rx_irq() only mask
XAXIDMA_IRQ_ALL_MASK. They don't stop the TX queue or disable NAPI. So at
the same time:

- nixge_start_xmit() on another CPU can fill tx_skb[tx_bd_tail] and
  advance tx_bd_tail
- nixge_poll()->nixge_recv() can walk rx_bd_v[rx_bd_ci] and
  read-modify-write RX_CR
- a TX IOC interrupt already running on another CPU can call
  nixge_start_xmit_done()->nixge_tx_skb_unmap() on the same entries

With no shared lock, can two contexts both see tx_skb->skb != NULL and
both call dev_kfree_skb_any()? The same interleavings could also unmap
twice, corrupt the ring indices, and lose CR updates.

[ ... ]

> @@ -869,6 +872,10 @@ static int nixge_open(struct net_device *ndev)
>  	struct phy_device *phy;
>  	int ret;
>  
> +	/* A failed stop retained buffers which DMA may still be using. */
> +	if (priv->dma_stop_failed)
> +		return -EIO;
> +

[Severity: Medium]
Is there any way to recover once dma_stop_failed is set?

nixge_stop() sets the flag after a single __nixge_device_reset() poll, which
allows 1 ms in 10 us steps. Nothing ever clears the flag. Every later
nixge_open() returns -EIO without re-reading the DMA status or retrying the
reset.

nixge_remove() doesn't retry the reset or reclaim the kept buffers either.
The BD rings, the RX skbs and their mappings therefore stay leaked even
after unbind.

nixge_stop() also issues the reset without first waiting for the channels to
halt after clearing RUNSTOP. xilinx_axienet drives the same AXI DMA IP. Its
axienet_dma_stop() waits for XAXIDMA_SR_HALT_MASK on each channel, and
__axienet_device_reset() then allows 50 ms for the reset bit to clear:

	ret = read_poll_timeout(axienet_dma_in32, value,
				!(value & XAXIDMA_CR_RESET_MASK),
				DELAY_OF_ONE_MILLISEC, 50000, false, lp,
				XAXIDMA_TX_CR_OFFSET);

Is 1 ms enough for a stop-time reset of a busy engine?

The commit message says:

    If reset times out, retain the DMA buffers and reject subsequent opens

It doesn't mention that the port then stays unusable until the driver is
rebound.

>  	nixge_device_reset(ndev);
>  
>  	phy = of_phy_connect(ndev, priv->phy_node,

[Severity: Medium]
This isn't a bug introduced by this patch, but the error paths in
nixge_open() leave the DMA engine running and leak the rings.

nixge_device_reset()->nixge_hw_dma_bd_init() allocates the TX/RX BD rings,
maps the RX skbs, enables XAXIDMA_IRQ_ALL_MASK and sets RUNSTOP on both
channels. If of_phy_connect() then fails:

	if (!phy)
		return -ENODEV;

If request_irq() fails, the err_rx_irq/err_tx_irq labels only free the
other IRQ, disable NAPI, disconnect the PHY and kill the tasklet.

Neither path clears RUNSTOP, resets the DMA or calls
nixge_hw_dma_bd_release(). nixge_stop() is not called after a failed open.

On the next open, nixge_device_reset() still ignores the reset results.
nixge_hw_dma_bd_init() then overwrites priv->tx_bd_v, priv->tx_skb and
priv->rx_bd_v.

Doesn't that leak the old rings and RX skbs? If that reset timed out,
wouldn't DMA also still be pointed at them? A nixge_remove() after a failed
open also calls free_netdev() with RUNSTOP still set.

[ ... ]

> @@ -933,6 +942,23 @@ static int nixge_stop(struct net_device *ndev)
>  
>  	tasklet_kill(&priv->dma_err_tasklet);
>  
> +	/* Error recovery may have restarted DMA and enabled interrupts. */
> +	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> +	nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
> +			    cr & ~(XAXIDMA_CR_RUNSTOP_MASK | XAXIDMA_IRQ_ALL_MASK));
> +	cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET);
> +	nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
> +			    cr & ~(XAXIDMA_CR_RUNSTOP_MASK | XAXIDMA_IRQ_ALL_MASK));
> +
> +	/* Either channel's reset quiesces the entire AXI DMA engine. */
> +	ret = __nixge_device_reset(priv, XAXIDMA_TX_CR_OFFSET);

[Severity: High]
Does this checked reset come too late to protect the TX buffers?

Suppose a DMA error interrupt scheduled recovery before free_irq(). The
tasklet_kill() above then waits for nixge_dma_err_handler() to finish. That
handler does its own reset and drops both results:

	__nixge_device_reset(lp, XAXIDMA_TX_CR_OFFSET);
	__nixge_device_reset(lp, XAXIDMA_RX_CR_OFFSET);

	for (i = 0; i < TX_BD_NUM; i++) {
		cur_p = &lp->tx_bd_v[i];
		tx_skb = &lp->tx_skb[i];
		nixge_tx_skb_unmap(lp, tx_skb);

nixge_tx_skb_unmap() does dma_unmap_single()/dma_unmap_page() and
dev_kfree_skb_any(). The handler then rewrites CDESC/TDESC and sets
XAXIDMA_IRQ_ALL_MASK and RUNSTOP on both channels.

So the sequence is:

nixge_stop()
  free_irq()
  tasklet_kill()
    nixge_dma_err_handler()
      __nixge_device_reset()   <- times out, result ignored
      nixge_tx_skb_unmap()     <- unmaps and frees in-flight TX skbs
  __nixge_device_reset()       <- result checked only here
    priv->dma_stop_failed = true

If the tasklet's reset timed out, the TX buffers are already unmapped and
freed while the engine may still be reading them. Setting dma_stop_failed
afterwards can't undo that.

The open path has the same gap. nixge_device_reset() ignores both reset
results. nixge_hw_dma_bd_init() then installs new ring pointers and sets
RUNSTOP on an engine that may still be in reset.

Should nixge_dma_err_handler() and nixge_device_reset() also check the new
return value of __nixge_device_reset(), and skip releasing or re-arming on
failure?

> +	if (ret) {
> +		/* Retain DMA buffers and prevent a subsequent open replacing them. */
> +		priv->dma_stop_failed = true;
> +		netdev_err(ndev, "DMA stop failed; retaining DMA buffers\n");
> +		return ret;

[Severity: Low]
Does this return value reach anyone? __dev_close_many() ignores the
ndo_stop() result and marks the device down anyway:

		if (ops->ndo_stop)
			ops->ndo_stop(dev);

		netif_rx_mode_cancel_retry(dev);
		netif_set_up(dev, false);

So "ip link set down" appears to succeed. The only signs of the failure are
the dmesg line and a later -EIO from nixge_open().

Documentation/networking/driver.rst also requires that after ndo_stop the
hardware no longer receives or transmits data. This branch can't guarantee
that or report it.

> +	}
> +
>  	nixge_hw_dma_bd_release(ndev);

[Severity: High]
This is a pre-existing issue, but can nixge_hw_dma_bd_release() free the
rings twice?

It frees the RX skbs, rx_bd_v, tx_skb and tx_bd_v, but never resets
priv->rx_bd_v, priv->tx_skb or priv->tx_bd_v to NULL.

Take a normal open/stop cycle, followed by an open where
nixge_hw_dma_bd_init() fails its first allocation:

	priv->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
					   sizeof(*priv->tx_bd_v) * TX_BD_NUM,
					   &priv->tx_bd_p, GFP_KERNEL);
	if (!priv->tx_bd_v)
		goto out;

The out label calls nixge_hw_dma_bd_release() while priv->rx_bd_v and
priv->tx_skb still hold stale pointers. Wouldn't that walk freed coherent
memory and dma_unmap_single() stale addresses? It would also
dev_kfree_skb() skbs that are already freed, and dma_free_coherent()/
devm_kfree() the same buffers again.

If a later allocation in nixge_hw_dma_bd_init() fails instead, the out path
frees the rings but leaves the pointers set. nixge_device_reset() only logs
the failure:

	if (nixge_hw_dma_bd_init(ndev))
		netdev_err(ndev, "%s: descriptor allocation failed\n",
			   __func__);

nixge_open() then carries on, nixge_start_xmit() uses the freed
tx_bd_v/tx_skb, and this unconditional nixge_hw_dma_bd_release() frees
everything a second time.

[Severity: Medium]
This also wasn't introduced by this patch, but does nixge_stop() leak the
in-flight TX skbs and their DMA mappings?

nixge_hw_dma_bd_release() never walks priv->tx_skb[]. It only does:

	if (priv->tx_skb)
		devm_kfree(ndev->dev.parent, priv->tx_skb);

Only nixge_start_xmit_done() (from the TX IRQ) and the error tasklet call
nixge_tx_skb_unmap(). By this point the IRQ handlers are freed, the tasklet
is drained and the engine has been reset. Frames that were queued but not
yet completed are therefore never unmapped or freed.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018020840.3851552-1-mhun512%40gmail.com