[PATCH net] net: nixge: stop IRQ producers before draining DMA error tasklet

[PATCH net] net: nixge: stop IRQ producers before draining DMA error tasklet

[Myeonghun Pak]

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

Keep the initial DMA channel stop while the completion IRQ handlers are
still installed. Then free both IRQs to stop and synchronize the tasklet
producers before draining error recovery.

A tasklet queued before the IRQs are freed can restart both channels.
Stop them again after tasklet_kill() returns so error recovery cannot
undo the final stop before the descriptors are released.

This issue was identified during our ongoing static-analysis research
while reviewing kernel code.

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>
---
Compile-tested nixge.o on net commit
6dc989ea46b96ce170840174b4a38c4a387fb005 with x86_64 allyesconfig
and allmodconfig, both with W=1. No hardware testing was performed.

 drivers/net/ethernet/ni/nixge.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 230d5ff99dd7..7bba6f8e64fe 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -920,6 +920,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));
@@ -927,11 +928,20 @@ 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);
+
+	/* Error recovery may have restarted DMA, so stop both channels again. */
+	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
+	nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
+			    cr & (~XAXIDMA_CR_RUNSTOP_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));
+
 	nixge_hw_dma_bd_release(ndev);
 
 	return 0;
-- 
2.53.0

Re: [PATCH net] net: nixge: stop IRQ producers before draining DMA error tasklet

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (3) · Medium: 0 (2) · 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 second stop the patch adds to nixge_stop() (lines 937-943)
  only clears DMACR.RS on the RX and TX AXI DMA channels.

Pre-existing issues:
- [High] nixge_hw_dma_bd_release()
  (drivers/net/ethernet/ni/nixge.c:244-281) frees priv->rx_bd_v, the RX…
- [High] The DMA error tasklet nixge_dma_err_handler() runs in softirq
  context and starts by calling __nixge_device_reset() for TX and RX…
- [High] nixge_dma_err_handler() takes no lock while it resets tx_bd_ci,
  tx_bd_tail and rx_bd_ci (lines 816-818), rewrites every TX descriptor,…
- [Medium] nixge_hw_dma_bd_release() calls devm_kfree(priv->tx_skb) (line
  273) and frees tx_bd_v without first calling nixge_tx_skb_unmap() on…
- [Medium] nixge_open() calls nixge_device_reset() first.

--

net: nixge: stop IRQ producers before draining DMA error tasklet

This reorders nixge_stop() so both IRQs are freed before tasklet_kill()
drains the DMA error tasklet. That stops the interrupt handlers from
rescheduling the tasklet afterwards. It also clears RUNSTOP on both DMA
channels a second time after tasklet_kill(), in case error recovery
restarted them.

> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 230d5ff99dd7e..7bba6f8e64fe1 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c

[ ... ]

> @@ -927,11 +928,20 @@ 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);

[Severity: High]
This is a pre-existing issue, but what serializes nixge_dma_err_handler()
against the TX paths? The tasklet takes no lock while it unmaps every
tx_skb[] entry and resets the ring indices:

drivers/net/ethernet/ni/nixge.c: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;
    ...
}

Two other paths update the same state:

- nixge_start_xmit(), which holds only netif_tx_lock. The error IRQ
  handlers never stop the TX queue.
- nixge_tx_irq()->nixge_start_xmit_done(), in hardirq context. This can
  run on another CPU because tx_irq and rx_irq are separate lines.

For example:

CPU1
nixge_rx_irq()            /* DMA error */
  tasklet_schedule()
nixge_dma_err_handler()
  lp->tx_bd_tail = 0;

CPU0
nixge_start_xmit()
  tx_skb->skb = skb;      /* tx_skb[tx_bd_tail] */
  ++priv->tx_bd_tail;

Another case: a TX completion on CPU0 runs nixge_start_xmit_done(), which
calls nixge_tx_skb_unmap() on the same entry the tasklet is unmapping.

nixge_tx_skb_unmap() checks and then frees tx_skb->mapping and tx_skb->skb
without a lock. Can this cause a double dma unmap and a double
dev_kfree_skb_any()? Could the tasklet also free an skb whose descriptor
the hardware then transmits from? The sleeping reset poll at the start of
the tasklet makes this window wider.

nixge_stop() has a similar window between the first RUNSTOP clear and the
free_irq() calls above.

>  
> +	tasklet_kill(&priv->dma_err_tasklet);
> +

[Severity: High]
This isn't a bug introduced by this patch, but nixge_stop() now waits
here for the error tasklet to finish its recovery. Can
nixge_dma_err_handler() sleep in softirq context?

nixge_dma_err_handler()
  __nixge_device_reset()
    nixge_dma_poll_timeout(priv, offset, status, ..., 10, 1000)
      readl_poll_timeout()
        usleep_range()

nixge_dma_poll_timeout() wraps readl_poll_timeout(), not
readl_poll_timeout_atomic(). With sleep_us set to 10, it calls
might_sleep_if(). It then calls usleep_range() whenever the reset bit has
not cleared on the first read. The comment in __nixge_device_reset() says
the reset "takes a while to complete", so the sleeping path is expected
to run.

Would every TX or RX DMA error interrupt then trigger "BUG: scheduling
while atomic"? Using readl_poll_timeout_atomic(), or moving the recovery
to a workqueue, would avoid that.

> +	/* Error recovery may have restarted DMA, so stop both channels again. */
> +	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> +	nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
> +			    cr & (~XAXIDMA_CR_RUNSTOP_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));
> +

[Severity: High]
Is clearing RUNSTOP enough to quiesce the engines before
nixge_hw_dma_bd_release() frees the rings?

On AXI DMA, clearing DMACR.RS only requests a halt. The engine stops
after it finishes or flushes the current transfer and any outstanding
descriptor updates. Nothing here polls XAXIDMA_RX_SR_OFFSET or
XAXIDMA_TX_SR_OFFSET for the halted bit, and the channels are not reset.

This block is meant for the case where the error tasklet has just
restarted the engines. In that case nixge_dma_err_handler() has
re-enabled the interrupts and handed the whole RX ring back to the
hardware:

drivers/net/ethernet/ni/nixge.c:nixge_dma_err_handler() {
    ...
	cr |= XAXIDMA_IRQ_ALL_MASK;
	/* Finally write to the Rx channel control register */
	nixge_dma_write_reg(lp, XAXIDMA_RX_CR_OFFSET, cr);
    ...
	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)));
    ...
}

Right after the posted RS clear, nixge_hw_dma_bd_release() unmaps and
frees the RX skbs and frees both coherent BD rings. Can an in-flight
frame or a BD status writeback land in that freed memory?

Frames can still arrive if phy_disconnect() did not power down the PHY,
for example when the PHY driver has no suspend callback or WoL is
enabled.

The bare RS clear also leaves the coalesce, delay and error interrupt
enables set by the tasklet, even though the handlers are now freed.

Other drivers for the same IP wait for the halt:

- axienet_dma_stop() in xilinx_axienet_main.c clears RS and the IRQ
  enables, polls XAXIDMA_SR_HALT_MASK, and then resets "to ensure DMA is
  really stopped".
- xilinx_dma_stop_transfer() in drivers/dma/xilinx/xilinx_dma.c polls
  DMASR.HALTED after clearing RUNSTOP.

The commit message says:

    Stop them again after tasklet_kill() returns so error recovery cannot
    undo the final stop before the descriptors are released.

Would one of these be a better final quiesce before the release?

- Call the existing __nixge_device_reset() on both channels here. It
  writes XAXIDMA_CR_RESET_MASK and waits for it to clear.
- Wait for the halted bit before releasing the descriptors.

>  	nixge_hw_dma_bd_release(ndev);

[Severity: High]
This isn't a bug introduced by this patch, but nixge_hw_dma_bd_release()
frees rx_bd_v, the RX skbs, tx_skb and tx_bd_v without setting the
pointers back to NULL. What happens on the next ifup if an allocation in
nixge_hw_dma_bd_init() fails?

For example, if dma_alloc_coherent() for tx_bd_v fails, tx_bd_v is NULL.
rx_bd_v and tx_skb still hold the addresses that the previous
nixge_stop() freed:

nixge_open()
  nixge_device_reset()
    nixge_hw_dma_bd_init()
      out:
        nixge_hw_dma_bd_release()

The release then does the following with stale pointers:

- Walks the freed rx_bd_v ring.
- Calls dma_unmap_single() and dev_kfree_skb() on skbs that were already
  freed.
- Calls dma_free_coherent() on rx_bd_v a second time.
- Calls devm_kfree() on a tx_skb that is no longer on the devres list.

If the failure happens partway through the RX skb loop, the release also
unmaps descriptors whose phys field was never filled in.

nixge_device_reset() only logs the error:

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

So nixge_open() still returns success with NULL or freed rings.
nixge_start_xmit() and the IRQ handlers then use those rings. The next
nixge_stop() calls nixge_hw_dma_bd_release() on the same stale pointers
again.

Should the release set the pointers to NULL, and should
nixge_device_reset() return the -ENOMEM to nixge_open()?

[Severity: Medium]
This is also a pre-existing issue, but does nixge_stop() leak the pending
TX skbs and their DMA mappings? nixge_hw_dma_bd_release() frees the
tx_skb array and tx_bd_v without first calling nixge_tx_skb_unmap() on
each entry:

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

TX skbs are only unmapped and freed in two places:

- nixge_start_xmit_done(), on a TX completion interrupt.
- nixge_dma_err_handler().

Some TX descriptors may still be pending when nixge_stop() clears
RUNSTOP and frees the IRQs. For example, the link may have gone down, or
a completion interrupt may be held back by coalescing. Are those skbs
and their mappings then lost on every ifdown?

[Severity: Medium]
This is another pre-existing issue, not introduced by this patch, but the
nixge_open() error paths skip this release entirely.
nixge_device_reset() allocates the rings, allocates and maps 128 RX
skbs, and sets RUNSTOP on both channels. If of_phy_connect() or either
request_irq() fails, nixge_open() takes these paths:

	phy = of_phy_connect(ndev, priv->phy_node,
			     &nixge_handle_link_change, 0, priv->phy_mode);
	if (!phy)
		return -ENODEV;
    ...
err_rx_irq:
	free_irq(priv->tx_irq, ndev);
err_tx_irq:
	napi_disable(&priv->napi);
	phy_stop(phy);
	phy_disconnect(phy);
	tasklet_kill(&priv->dma_err_tasklet);

Neither path calls nixge_hw_dma_bd_release() or stops the DMA. Do the
coherent rings, the RX skbs and their mappings leak here, with RX DMA
still running into them?

The next nixge_open() overwrites tx_bd_v, tx_skb and rx_bd_v, so these
allocations can never be reclaimed.

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

Re: [PATCH net] net: nixge: stop IRQ producers before draining DMA error tasklet

[Myeonghun Pak]

Thanks for the review. I agree that clearing RUNSTOP alone is insufficient.
I will send v2 with a checked DMA reset after freeing both IRQs and
draining the tasklet. On reset timeout, it retains the DMA buffers and
rejects subsequent opens.

The other findings are pre-existing issues and need separate fixes.

pw-bot: cr

2026년 10월 4일 (일) 오전 12:14, <netdev-bot+sashiko@kernel.org>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (3) · Medium: 0 (2) · 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 second stop the patch adds to nixge_stop() (lines 937-943)
>   only clears DMACR.RS on the RX and TX AXI DMA channels.
>
> Pre-existing issues:
> - [High] nixge_hw_dma_bd_release()
>   (drivers/net/ethernet/ni/nixge.c:244-281) frees priv->rx_bd_v, the RX…
> - [High] The DMA error tasklet nixge_dma_err_handler() runs in softirq
>   context and starts by calling __nixge_device_reset() for TX and RX…
> - [High] nixge_dma_err_handler() takes no lock while it resets tx_bd_ci,
>   tx_bd_tail and rx_bd_ci (lines 816-818), rewrites every TX descriptor,…
> - [Medium] nixge_hw_dma_bd_release() calls devm_kfree(priv->tx_skb) (line
>   273) and frees tx_bd_v without first calling nixge_tx_skb_unmap() on…
> - [Medium] nixge_open() calls nixge_device_reset() first.
>
> --
>
> net: nixge: stop IRQ producers before draining DMA error tasklet
>
> This reorders nixge_stop() so both IRQs are freed before tasklet_kill()
> drains the DMA error tasklet. That stops the interrupt handlers from
> rescheduling the tasklet afterwards. It also clears RUNSTOP on both DMA
> channels a second time after tasklet_kill(), in case error recovery
> restarted them.
>
> > diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> > index 230d5ff99dd7e..7bba6f8e64fe1 100644
> > --- a/drivers/net/ethernet/ni/nixge.c
> > +++ b/drivers/net/ethernet/ni/nixge.c
>
> [ ... ]
>
> > @@ -927,11 +928,20 @@ 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);
>
> [Severity: High]
> This is a pre-existing issue, but what serializes nixge_dma_err_handler()
> against the TX paths? The tasklet takes no lock while it unmaps every
> tx_skb[] entry and resets the ring indices:
>
> drivers/net/ethernet/ni/nixge.c: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;
>     ...
> }
>
> Two other paths update the same state:
>
> - nixge_start_xmit(), which holds only netif_tx_lock. The error IRQ
>   handlers never stop the TX queue.
> - nixge_tx_irq()->nixge_start_xmit_done(), in hardirq context. This can
>   run on another CPU because tx_irq and rx_irq are separate lines.
>
> For example:
>
> CPU1
> nixge_rx_irq()            /* DMA error */
>   tasklet_schedule()
> nixge_dma_err_handler()
>   lp->tx_bd_tail = 0;
>
> CPU0
> nixge_start_xmit()
>   tx_skb->skb = skb;      /* tx_skb[tx_bd_tail] */
>   ++priv->tx_bd_tail;
>
> Another case: a TX completion on CPU0 runs nixge_start_xmit_done(), which
> calls nixge_tx_skb_unmap() on the same entry the tasklet is unmapping.
>
> nixge_tx_skb_unmap() checks and then frees tx_skb->mapping and tx_skb->skb
> without a lock. Can this cause a double dma unmap and a double
> dev_kfree_skb_any()? Could the tasklet also free an skb whose descriptor
> the hardware then transmits from? The sleeping reset poll at the start of
> the tasklet makes this window wider.
>
> nixge_stop() has a similar window between the first RUNSTOP clear and the
> free_irq() calls above.
>
> >
> > +     tasklet_kill(&priv->dma_err_tasklet);
> > +
>
> [Severity: High]
> This isn't a bug introduced by this patch, but nixge_stop() now waits
> here for the error tasklet to finish its recovery. Can
> nixge_dma_err_handler() sleep in softirq context?
>
> nixge_dma_err_handler()
>   __nixge_device_reset()
>     nixge_dma_poll_timeout(priv, offset, status, ..., 10, 1000)
>       readl_poll_timeout()
>         usleep_range()
>
> nixge_dma_poll_timeout() wraps readl_poll_timeout(), not
> readl_poll_timeout_atomic(). With sleep_us set to 10, it calls
> might_sleep_if(). It then calls usleep_range() whenever the reset bit has
> not cleared on the first read. The comment in __nixge_device_reset() says
> the reset "takes a while to complete", so the sleeping path is expected
> to run.
>
> Would every TX or RX DMA error interrupt then trigger "BUG: scheduling
> while atomic"? Using readl_poll_timeout_atomic(), or moving the recovery
> to a workqueue, would avoid that.
>
> > +     /* Error recovery may have restarted DMA, so stop both channels again. */
> > +     cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> > +     nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
> > +                         cr & (~XAXIDMA_CR_RUNSTOP_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));
> > +
>
> [Severity: High]
> Is clearing RUNSTOP enough to quiesce the engines before
> nixge_hw_dma_bd_release() frees the rings?
>
> On AXI DMA, clearing DMACR.RS only requests a halt. The engine stops
> after it finishes or flushes the current transfer and any outstanding
> descriptor updates. Nothing here polls XAXIDMA_RX_SR_OFFSET or
> XAXIDMA_TX_SR_OFFSET for the halted bit, and the channels are not reset.
>
> This block is meant for the case where the error tasklet has just
> restarted the engines. In that case nixge_dma_err_handler() has
> re-enabled the interrupts and handed the whole RX ring back to the
> hardware:
>
> drivers/net/ethernet/ni/nixge.c:nixge_dma_err_handler() {
>     ...
>         cr |= XAXIDMA_IRQ_ALL_MASK;
>         /* Finally write to the Rx channel control register */
>         nixge_dma_write_reg(lp, XAXIDMA_RX_CR_OFFSET, cr);
>     ...
>         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)));
>     ...
> }
>
> Right after the posted RS clear, nixge_hw_dma_bd_release() unmaps and
> frees the RX skbs and frees both coherent BD rings. Can an in-flight
> frame or a BD status writeback land in that freed memory?
>
> Frames can still arrive if phy_disconnect() did not power down the PHY,
> for example when the PHY driver has no suspend callback or WoL is
> enabled.
>
> The bare RS clear also leaves the coalesce, delay and error interrupt
> enables set by the tasklet, even though the handlers are now freed.
>
> Other drivers for the same IP wait for the halt:
>
> - axienet_dma_stop() in xilinx_axienet_main.c clears RS and the IRQ
>   enables, polls XAXIDMA_SR_HALT_MASK, and then resets "to ensure DMA is
>   really stopped".
> - xilinx_dma_stop_transfer() in drivers/dma/xilinx/xilinx_dma.c polls
>   DMASR.HALTED after clearing RUNSTOP.
>
> The commit message says:
>
>     Stop them again after tasklet_kill() returns so error recovery cannot
>     undo the final stop before the descriptors are released.
>
> Would one of these be a better final quiesce before the release?
>
> - Call the existing __nixge_device_reset() on both channels here. It
>   writes XAXIDMA_CR_RESET_MASK and waits for it to clear.
> - Wait for the halted bit before releasing the descriptors.
>
> >       nixge_hw_dma_bd_release(ndev);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but nixge_hw_dma_bd_release()
> frees rx_bd_v, the RX skbs, tx_skb and tx_bd_v without setting the
> pointers back to NULL. What happens on the next ifup if an allocation in
> nixge_hw_dma_bd_init() fails?
>
> For example, if dma_alloc_coherent() for tx_bd_v fails, tx_bd_v is NULL.
> rx_bd_v and tx_skb still hold the addresses that the previous
> nixge_stop() freed:
>
> nixge_open()
>   nixge_device_reset()
>     nixge_hw_dma_bd_init()
>       out:
>         nixge_hw_dma_bd_release()
>
> The release then does the following with stale pointers:
>
> - Walks the freed rx_bd_v ring.
> - Calls dma_unmap_single() and dev_kfree_skb() on skbs that were already
>   freed.
> - Calls dma_free_coherent() on rx_bd_v a second time.
> - Calls devm_kfree() on a tx_skb that is no longer on the devres list.
>
> If the failure happens partway through the RX skb loop, the release also
> unmaps descriptors whose phys field was never filled in.
>
> nixge_device_reset() only logs the error:
>
>         if (nixge_hw_dma_bd_init(ndev))
>                 netdev_err(ndev, "%s: descriptor allocation failed\n",
>                            __func__);
>
> So nixge_open() still returns success with NULL or freed rings.
> nixge_start_xmit() and the IRQ handlers then use those rings. The next
> nixge_stop() calls nixge_hw_dma_bd_release() on the same stale pointers
> again.
>
> Should the release set the pointers to NULL, and should
> nixge_device_reset() return the -ENOMEM to nixge_open()?
>
> [Severity: Medium]
> This is also a pre-existing issue, but does nixge_stop() leak the pending
> TX skbs and their DMA mappings? nixge_hw_dma_bd_release() frees the
> tx_skb array and tx_bd_v without first calling nixge_tx_skb_unmap() on
> each entry:
>
>         if (priv->tx_skb)
>                 devm_kfree(ndev->dev.parent, priv->tx_skb);
>
> TX skbs are only unmapped and freed in two places:
>
> - nixge_start_xmit_done(), on a TX completion interrupt.
> - nixge_dma_err_handler().
>
> Some TX descriptors may still be pending when nixge_stop() clears
> RUNSTOP and frees the IRQs. For example, the link may have gone down, or
> a completion interrupt may be held back by coalescing. Are those skbs
> and their mappings then lost on every ifdown?
>
> [Severity: Medium]
> This is another pre-existing issue, not introduced by this patch, but the
> nixge_open() error paths skip this release entirely.
> nixge_device_reset() allocates the rings, allocates and maps 128 RX
> skbs, and sets RUNSTOP on both channels. If of_phy_connect() or either
> request_irq() fails, nixge_open() takes these paths:
>
>         phy = of_phy_connect(ndev, priv->phy_node,
>                              &nixge_handle_link_change, 0, priv->phy_mode);
>         if (!phy)
>                 return -ENODEV;
>     ...
> err_rx_irq:
>         free_irq(priv->tx_irq, ndev);
> err_tx_irq:
>         napi_disable(&priv->napi);
>         phy_stop(phy);
>         phy_disconnect(phy);
>         tasklet_kill(&priv->dma_err_tasklet);
>
> Neither path calls nixge_hw_dma_bd_release() or stops the DMA. Do the
> coherent rings, the RX skbs and their mappings leak here, with RX DMA
> still running into them?
>
> The next nixge_open() overwrites tx_bd_v, tx_skb and rx_bd_v, so these
> allocations can never be reclaimed.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003041242.650599-1-mhun512%40gmail.com