[PATCH net v2 0/4] net: axienet: fix TX queue handling around a DMA error reset

[PATCH net v2 0/4] net: axienet: fix TX queue handling around a DMA error reset

[Sagi Maimon]

axienet_dma_err_handler() resets the DMA engine together with the MAC
and rebuilds the TX ring, but it does not coordinate with the transmit
path, and it does not leave the MAC configured the way it found it.

v1 was a single patch that woke the queue at the end of the handler.
The Sashiko review showed that this wake could be lost to a concurrent
transmit and could undo the stop that suspend installs, and it pointed
out two older problems: the handler races axienet_start_xmit() while it
tears down the ring, and the reset loses the negotiated link speed and
pause settings.

This version:

1/4 starts the TX queue in axienet_open().  Nothing does today, so a
    queue stopped at close stays stopped.  2/4 depends on it.
2/4 quiesces the TX path in axienet_stop() before the ring is freed,
    which the suspend path otherwise leaves racing.
3/4 quiesces the TX queue across the whole error reset and wakes it at
    the end, unless the interface is stopping or detached.
4/4 has phylink reprogram the link settings after the reset.

v1 also called netif_trans_update() and said this stopped the watchdog
from refiring.  No .ndo_tx_timeout is installed in this tree, so the
call and the claim are both gone.

Tested on an AXI Ethernet MAC behind a PCIe endpoint with the series
applied: traffic passes, and resumes after each of ten down/up cycles
and five module reloads, all made with traffic running, with nothing
logged.  Suspend, a down/up cycle with the queue stopped by a full ring,
and the DMA error path were not exercised.

v1 omitted the Assisted-by: tag; v2 adds it.

v1: https://lore.kernel.org/netdev/20260917121306.23499-1-maimon.sagi@gmail.com/

Sagi Maimon (4):
  net: axienet: start the TX queue in axienet_open()
  net: axienet: quiesce the TX path before axienet_stop() tears it down
  net: axienet: quiesce the TX queue across a DMA error reset
  net: axienet: reprogram the link settings after a DMA error reset

 .../net/ethernet/xilinx/xilinx_axienet_main.c | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)


base-commit: 879e280b8486d4612ad1aa050d6fada2dd80cf1c
-- 
2.47.0

[PATCH net v2 1/4] net: axienet: start the TX queue in axienet_open()

[Sagi Maimon]

axienet_open() never starts the transmit queue, and nothing else does
either: __dev_open() and dev_activate() leave the driver's queue state
alone.

axienet_start_xmit() stops the queue with netif_stop_queue() when the TX
ring fills, and axienet_tx_poll() wakes it again as completions free
descriptors.  If the interface is brought down while the queue is
stopped, __QUEUE_STATE_DRV_XOFF survives into the next axienet_open().
The ring is reinitialised empty, so no TX completion ever arrives to run
the wake in axienet_tx_poll(), and the interface cannot transmit until
the driver is reloaded.  The resume path is unaffected only because
netif_device_attach() wakes the queues.

Start the queue at the end of a successful axienet_open(), as most
drivers do.

Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes,
and resumes after each of ten down/up cycles and five module reloads,
all made with traffic running.  A queue left stopped across a down/up
cycle, with the ring full, was not reproduced.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 782f903d318f..fb26d2e39fac 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1700,6 +1700,11 @@ static int axienet_open(struct net_device *ndev)
 			goto err_phy;
 	}
 
+	/* Nothing else clears a stop left over from before the last close:
+	 * the ring is empty, so no TX completion will wake the queue.
+	 */
+	netif_start_queue(ndev);
+
 	return 0;
 
 err_free_eth_irq:
-- 
2.47.0

[PATCH net v2 2/4] net: axienet: quiesce the TX path before axienet_stop() tears it down

[Sagi Maimon]

On the legacy DMA path axienet_stop() stops the DMA engine and frees the
TX descriptor ring with axienet_dma_bd_release(), but never stops the
transmit queue or waits for a transmit already in progress.

On the dev_close() path this is covered by the core:
dev_deactivate_many() has already quiesced the qdisc and waited for
in-flight transmits with synchronize_net().  axienet_suspend() instead
calls axienet_stop() directly.  Its netif_device_detach() only sets
__QUEUE_STATE_DRV_XOFF, without taking the transmit lock, so an
axienet_start_xmit() that was already running can still be writing a
descriptor into lp->tx_bd_v, or kicking XAXIDMA_TX_TDESC, while the
engine is reset and the ring is freed underneath it.

Call netif_tx_disable() once TX NAPI is disabled and the error work has
been flushed.  It takes each queue's transmit lock, so it waits for any
transmit in progress, and nothing can wake the queue afterwards: the
error work returns early once lp->stopping is set, and axienet_tx_poll()
can no longer run.

The dmaengine path is left as it is.  There the completion callback can
wake the queue until the channel has been terminated, so it would need a
different ordering.

Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes,
and after each of ten down/up cycles and five module reloads, all made
with traffic running and each running this path, traffic resumes and
nothing is logged.  Suspend was not exercised.

Fixes: a3de357b087e ("net: axiemac: add PM callbacks to support suspend/resume")
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index fb26d2e39fac..6d448d0b523d 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1739,6 +1739,14 @@ static int axienet_stop(struct net_device *ndev)
 
 		napi_disable(&lp->napi_tx);
 		napi_disable(&lp->napi_rx);
+
+		/* Nothing can wake the queue now: the error work returns early
+		 * once lp->stopping is set, and TX NAPI is disabled.  Stop it and
+		 * wait out any transmit in progress before the ring goes away.
+		 * dev_close() has already done this, but axienet_suspend() calls
+		 * us directly.
+		 */
+		netif_tx_disable(ndev);
 	}
 
 	cancel_work_sync(&lp->rx_dim.work);
-- 
2.47.0

[PATCH net v2 3/4] net: axienet: quiesce the TX queue across a DMA error reset

[Sagi Maimon]

axienet_dma_err_handler() resets the DMA engine, frees every TX
descriptor's skb and mapping, and rewinds lp->tx_bd_ci and
lp->tx_bd_tail to 0.  It has two problems with the transmit path.

First, nothing excludes axienet_start_xmit() while it does so.
napi_disable() only stops axienet_tx_poll(), and the handler takes no
transmit lock.  A transmit running concurrently can publish an skb into
a descriptor that the handler then frees, and dereference it afterwards
in netdev_sent_queue(), or program a descriptor whose mapping the
handler has just released and kick XAXIDMA_TX_TDESC with a tail pointer
the handler is about to rewind.

Second, the handler never restarts the queue.  If the ring was full when
the error hit, axienet_start_xmit() had stopped the queue with
netif_stop_queue(), and that __QUEUE_STATE_DRV_XOFF survives the reset:
netdev_reset_queue() clears only __QUEUE_STATE_STACK_XOFF, and nothing
at all without CONFIG_BQL.  The wake in axienet_tx_poll() is reached
only when axienet_free_tx_chain() reclaims packets, which cannot happen
once the handler has cleared every status word, so the interface stops
transmitting until it is brought down and up again.

Quiesce the transmit path with netif_tx_disable() once TX NAPI is
disabled, so that no transmit is in progress or can start while the ring
is torn down, and wake the queue once the reset is complete.  Because
the handler now owns the queue state for its whole duration, the wake
cannot be lost to a concurrent netif_stop_queue().

Skip the wake if the interface is being stopped or the device has been
detached for suspend, or it would undo the stop that
netif_device_detach() installed; axienet_stop() and axienet_open() own
the queue state then.  A detach racing with the check is covered by
axienet_stop() quiescing the queue again before it tears anything down.

Both problems were reported by the Sashiko AI review bot.

Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes,
including across ten down/up cycles made with traffic running, with this
series applied.  The DMA error path itself was not exercised.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 6d448d0b523d..f16dbfc7dc93 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2724,6 +2724,11 @@ static void axienet_dma_err_handler(struct work_struct *work)
 	napi_disable(&lp->napi_tx);
 	napi_disable(&lp->napi_rx);
 
+	/* With TX NAPI disabled nothing else can wake the queue.  Stop it and
+	 * wait out any transmit in progress, so the ring can be torn down.
+	 */
+	netif_tx_disable(ndev);
+
 	axienet_setoptions(ndev, lp->options &
 			   ~(XAE_OPTION_TXEN | XAE_OPTION_RXEN));
 
@@ -2791,6 +2796,13 @@ static void axienet_dma_err_handler(struct work_struct *work)
 	napi_enable(&lp->napi_rx);
 	napi_enable(&lp->napi_tx);
 	axienet_setoptions(ndev, lp->options);
+
+	/* Leave the queue stopped if the interface is going down or the
+	 * device was detached for suspend: axienet_stop() and axienet_open()
+	 * own the queue state then.
+	 */
+	if (!READ_ONCE(lp->stopping) && netif_device_present(ndev))
+		netif_wake_queue(ndev);
 }
 
 /**
-- 
2.47.0

[PATCH net v2 4/4] net: axienet: reprogram the link settings after a DMA error reset

[Sagi Maimon]

axienet_dma_err_handler() resets the DMA engine, which resets the AXI
Ethernet core with it.  The handler then restores RCW1, the interrupt
enable mask, the MAC address, the multicast filter and lp->options, but
not the link speed field of XAE_EMMC_OFFSET, and it writes XAE_FCC with
only XAE_FCC_FCRX_MASK, discarding whatever pause configuration had been
negotiated.

axienet_mac_link_up() is the only code that programs the link speed and
the negotiated pause bits, and phylink calls it only when the link state
changes.  Nothing about the reset is visible to phylink, so it is never
called again: the MAC keeps its reset-default link speed while software
still believes the negotiated one is in effect, and on a 10 or 100 Mb/s
link frames are clocked at the wrong rate until an unrelated link flap
happens to rerun axienet_mac_link_up().  axienet_open() avoids this only
because it runs phylink_start() after axienet_device_reset().

Tell phylink the link was lost with phylink_mac_change(), so it takes
the link down and back up and calls axienet_mac_link_up() with the
current settings, under its own locking.  Do it after the final
axienet_setoptions(), which also writes XAE_FCC, so the negotiated pause
settings are the ones left in place - the same order as in
axienet_open().  Skip it under the same conditions as the queue wake:
axienet_stop() stops phylink, and a resume restarts it.

The link going down and up is visible, which seems right: the MAC has
been reset.  A frame may still leave at the reset-default speed in the
short window before phylink runs; the reset has just dropped everything
that was in flight anyway.

This was reported by the Sashiko AI review bot.

Tested on an AXI Ethernet MAC behind a PCIe endpoint: traffic passes,
including across ten down/up cycles made with traffic running, with this
series applied.  The DMA error path itself was not exercised.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Assisted-by: LLM sparse
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index f16dbfc7dc93..d53f7dc7bd22 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2801,8 +2801,15 @@ static void axienet_dma_err_handler(struct work_struct *work)
 	 * device was detached for suspend: axienet_stop() and axienet_open()
 	 * own the queue state then.
 	 */
-	if (!READ_ONCE(lp->stopping) && netif_device_present(ndev))
+	if (!READ_ONCE(lp->stopping) && netif_device_present(ndev)) {
+		/* The reset also cleared the link speed and pause settings,
+		 * which only axienet_mac_link_up() programs.  Have phylink take
+		 * the link down and up again so that it is called.  This must
+		 * follow the axienet_setoptions() above, which writes XAE_FCC.
+		 */
+		phylink_mac_change(lp->phylink, false);
 		netif_wake_queue(ndev);
+	}
 }
 
 /**
-- 
2.47.0

Re: [PATCH net v2 1/4] net: axienet: start the TX queue in axienet_open()

[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] On resume, the new netif_start_queue() at the end of
  axienet_open() makes the later netif_device_attach() skip its qdisc…

Pre-existing issues:
- [High] axienet_open() throws away the return value of
  axienet_device_reset().
- [High] axienet_resume() ignores the return value of axienet_open().

--

net: axienet: start the TX queue in axienet_open()

This adds a netif_start_queue() call at the end of a successful
axienet_open(). The goal is to clear a __QUEUE_STATE_DRV_XOFF left over
from a close while the queue was stopped. Without that, the interface
could stay unable to transmit after a down/up cycle.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 782f903d318f3..fb26d2e39faca 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1700,6 +1700,11 @@ static int axienet_open(struct net_device *ndev)
>  			goto err_phy;
>  	}
>  
> +	/* Nothing else clears a stop left over from before the last close:
> +	 * the ring is empty, so no TX completion will wake the queue.
> +	 */

[Severity: High]
This isn't a bug introduced by this patch, but this comment and the commit
message ("The ring is reinitialised empty") assume that the reset at the
top of axienet_open() succeeded. Is that guaranteed?

axienet_open() {
	...
	axienet_lock_mii(lp);
	ret = axienet_device_reset(ndev);
	axienet_unlock_mii(lp);

	ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0);
	...
}

The axienet_device_reset() result is overwritten before anything checks it.
On the legacy DMA path, axienet_device_reset() returns early in two cases:
__axienet_device_reset() times out, or axienet_dma_bd_init() fails:

axienet_dma_bd_init() {
	...
	lp->tx_bd_v = dma_alloc_coherent(lp->dev, ...);
	if (!lp->tx_bd_v)
		return -ENOMEM;
	...
out:
	axienet_dma_bd_release(ndev);
	return -ENOMEM;
}

In both cases axienet_dma_start() never runs. tx_bd_v is then either NULL
or a stale pointer, because axienet_dma_bd_release() does not clear it.
The memory it points to was already freed, either by the error path above
or by the previous axienet_stop() when the reset times out on reopen.

axienet_open() still returns 0, and with this patch it also starts the
queue. The next axienet_start_xmit() then reaches
axienet_check_tx_bd_space():

	cur_p = &lp->tx_bd_v[(READ_ONCE(lp->tx_bd_tail) + num_frag) %
			     lp->tx_bd_num];
	if (cur_p->cntrl)

Can this dereference a NULL tx_bd_v? Could it instead write descriptors
into freed coherent memory and kick XAXIDMA_TX_TDESC on a channel that was
never started?

A later ifdown also goes through axienet_stop()->axienet_dma_bd_release(),
which would call dma_free_coherent() on tx_bd_v a second time.

The new netif_start_queue() also clears a stop left over from the previous
close, even though the hardware was never initialised. Should
axienet_open() check the axienet_device_reset() return value before going
on?

> +	netif_start_queue(ndev);

[Severity: Low]
On the resume path, does this stop netif_device_attach() from rescheduling
the qdisc?

axienet_suspend() calls netif_device_detach(), which sets
__QUEUE_STATE_DRV_XOFF. Packets sent while the device is suspended can stay
queued in the qdisc. axienet_resume() then does:

	rtnl_lock();
	axienet_open(ndev);
	rtnl_unlock();

	netif_device_attach(ndev);

axienet_open() now ends with netif_start_queue(), which clears DRV_XOFF
without calling __netif_schedule(). Then
netif_device_attach()->netif_tx_wake_all_queues()->netif_tx_wake_queue()
reschedules only when it clears the bit itself:

netif_tx_wake_queue() {
	...
	if (test_and_clear_bit(__QUEUE_STATE_DRV_XOFF, &dev_queue->state)) {
		...
		__netif_schedule(q);
	...
}

The bit is already clear at that point, so the backlog is not rescheduled.
The commit message says the resume path works "because
netif_device_attach() wakes the queues", but with this change that wake
does nothing.

If the carrier is already up when linkwatch runs (fixed-link, for example),
the queued packets would wait for the next unrelated enqueue. Would using
netif_wake_queue() in axienet_open() avoid this? Another option would be
to leave the resume case to netif_device_attach().

> +
>  	return 0;

[Severity: High]
This is a pre-existing issue, but axienet_open() is also called from
axienet_resume(). What happens if it fails there?

axienet_resume() {
	...
	rtnl_lock();
	axienet_open(ndev);
	rtnl_unlock();

	netif_device_attach(ndev);

	return 0;
}

The return value is ignored. So if phylink_of_phy_connect(), request_irq(),
axienet_init_dmaengine() or axienet_init_legacy_dma() fails, the device is
still attached, its TX queues are woken, and the PM core is told that
resume succeeded.

On legacy DMA, axienet_stop() disabled NAPI during suspend, and the failed
open either never re-enabled it or disabled it again in its error path. A
later ifdown then runs axienet_stop() again:

	napi_disable(&lp->napi_tx);
	napi_disable(&lp->napi_rx);

Can this spin forever in napi_disable_locked() while holding RTNL? The same
path would also call free_irq() on tx_irq and rx_irq, which may not be
requested at that point.

On dmaengine, tx_skb_ring and rx_skb_ring have already been freed and the
channels released. That happened either in axienet_stop() during suspend
or in the axienet_init_dmaengine() error path. Can
axienet_start_xmit_dmaengine() then use the freed tx_skb_ring and the
released tx_chan? Can a later ifdown kfree() the ring entries a second
time?

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

Re: [PATCH net v2 2/4] net: axienet: quiesce the TX path before axienet_stop() tears it down

[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] This patch only works when the previous commit in the series,
  "net: axienet: start the TX queue in axienet_open()", is also applied.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is still
  reachable after the whole series: on the dmaengine backend,…
- [High] This problem wasn't introduced by this patch, but it can undo
  the new TX quiesce and cause use-after-free, oops or hang after an…
- [Medium] This problem wasn't introduced by this patch, but the legacy-
  DMA axienet_stop() leaks outstanding TX skbs and their streaming DMA…

--

net: axienet: quiesce the TX path before axienet_stop() tears it down

This commit adds netif_tx_disable() to the legacy DMA branch of
axienet_stop(). The call comes after TX NAPI is disabled and the error
work is flushed, so a transmit still in progress when axienet_suspend()
runs finishes before the TX ring is freed. The dmaengine path is left
unchanged on purpose.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index fb26d2e39faca..6d448d0b523d3 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1739,6 +1739,14 @@ static int axienet_stop(struct net_device *ndev)
>  
>  		napi_disable(&lp->napi_tx);
>  		napi_disable(&lp->napi_rx);
> +
> +		/* Nothing can wake the queue now: the error work returns early
> +		 * once lp->stopping is set, and TX NAPI is disabled.  Stop it and
> +		 * wait out any transmit in progress before the ring goes away.
> +		 * dev_close() has already done this, but axienet_suspend() calls
> +		 * us directly.
> +		 */
> +		netif_tx_disable(ndev);

[Severity: Low]
This netif_tx_disable() runs on every legacy DMA stop, not only from
axienet_suspend(). That includes the normal dev_close() -> ndo_stop path.
So after each ifdown, __QUEUE_STATE_DRV_XOFF stays set on every TX queue.

In this tree the next open clears it again. That happens through the
netif_start_queue() added by the previous commit, 718829c2b094 ("net:
axienet: start the TX queue in axienet_open()"):

axienet_open() {
    ...
	netif_start_queue(ndev);

	return 0;
    ...
}

Without that commit, nothing in the driver clears DRV_XOFF on reopen, and
__dev_open()/dev_activate() don't clear it either. The ring is empty after
reopen, so axienet_tx_poll() never reaches netif_wake_queue().

This patch has a Fixes: tag, and its hunk context doesn't overlap with the
prerequisite. If it is backported on its own, would TX stay stopped for
good after the first down/up cycle? Could the commit message mention the
dependency on the previous patch, so that stable picks up both?

Related to this, the commit message says:

    On the dev_close() path this is covered by the core:
    dev_deactivate_many() has already quiesced the qdisc and waited for
    in-flight transmits with synchronize_net().

The new comment also says "dev_close() has already done this".
dev_deactivate_many() waits for in-flight transmits but doesn't set
DRV_XOFF. So on the close path the new call does change the queue state.
Could the commit message and the comment say so?

>  	}

[Severity: High]
This isn't a bug introduced by this patch, but the dmaengine branch of
axienet_stop() looks open to the same race from axienet_suspend(). It is
still unchanged at the end of the series.

axienet_suspend() calls netif_device_detach() and then calls
axienet_stop() directly. The dmaengine branch then does this without
netif_tx_disable() or any TX lock barrier:

	dmaengine_terminate_sync(lp->tx_chan);
	...
	for (i = 0; i < TX_BD_NUM_MAX; i++)
		kfree(lp->tx_skb_ring[i]);
	kfree(lp->tx_skb_ring);
	...
	dma_release_channel(lp->tx_chan);

Until the TX channel is terminated, axienet_dma_tx_cb() can still run:

	netif_txq_completed_wake(txq, 1, len,
				 CIRC_SPACE(lp->tx_ring_head, lp->tx_ring_tail, TX_BD_NUM_MAX),
				 2);

That wakes the stopped queue, clears the DRV_XOFF set by
netif_device_detach(), and reschedules the qdisc. On this path the qdisc
is never deactivated, and netif_running() stays true.

Can axienet_start_xmit_dmaengine() then run during or after the teardown?
It does:

	dma_dev = lp->tx_chan->device;
	...
	skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_head);
	...
	sg_init_table(skbuf_dma->sgl, sg_len);

That would write into the freed skbuf_dma and tx_skb_ring. It would also
submit descriptors to a terminated or released channel.

Nothing waits for an xmit that was already past the stopped check when
netif_device_detach() ran, either.

>  
>  	cancel_work_sync(&lp->rx_dim.work);

[Severity: High]
This is a pre-existing issue, but can the resume path undo the TX quiesce
added here? axienet_resume() ignores the return value of axienet_open()
and always attaches:

	rtnl_lock();
	axienet_open(ndev);
	rtnl_unlock();

	netif_device_attach(ndev);

netif_running() is still true, so netif_device_attach() calls
netif_tx_wake_all_queues() even when open failed.

axienet_open() also overwrites the result of the reset:

	ret = axienet_device_reset(ndev);
	axienet_unlock_mii(lp);

	ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0);

axienet_device_reset() fails if __axienet_device_reset() times out. It
also fails if axienet_dma_bd_init() can't allocate and takes its error
path:

out:
	axienet_dma_bd_release(ndev);
	return -ENOMEM;

In either case open keeps going. lp->tx_bd_v is then freed, or still stale
from the suspend-time axienet_dma_bd_release(). Open reaches
netif_start_queue() and returns 0.

Would axienet_start_xmit() then write BDs into a freed or NULL tx_bd_v,
and program a stale tx_bd_p into XAXIDMA_TX_TDESC?

On the dmaengine path, axienet_init_dmaengine() can fail during resume.
Its error path releases the channels and frees tx_skb_ring, or tx_chan is
left holding an ERR_PTR. netif_device_attach() still wakes the queue.
axienet_start_xmit_dmaengine() would then dereference lp->tx_chan->device
and the freed ring.

A later dev_close() -> axienet_stop() would then call
dmaengine_terminate_sync() on the bad channel and kfree() tx_skb_ring a
second time.

On the legacy path, phylink_of_phy_connect() or request_irq() can fail on
resume. In that case napi_tx and napi_rx stay disabled from the suspend.
Would a later dev_close() -> axienet_stop() -> napi_disable() then spin
forever in napi_disable_locked() while holding RTNL?

[Severity: Medium]
This isn't a bug introduced by this patch, but does the legacy DMA stop
path leak outstanding TX skbs and their DMA mappings?

With this change an in-flight axienet_start_xmit() can finish publishing
its BDs. Then axienet_dma_stop() resets the engine, and
axienet_dma_bd_release() only frees the descriptor array:

	dma_free_coherent(lp->dev,
			  sizeof(*lp->tx_bd_v) * lp->tx_bd_num,
			  lp->tx_bd_v,
			  lp->tx_bd_p);

Any BD that axienet_tx_poll() had not reclaimed keeps its
dma_map_single() mapping and cur_p->skb. The next axienet_dma_bd_init()
allocates a fresh ring, so nothing can reach those mappings any more.

axienet_dma_err_handler() does walk the TX BDs, calling dma_unmap_single()
and dev_kfree_skb_irq() for each one. Should axienet_stop() do the same
before it releases the ring?

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

Re: [PATCH net v2 3/4] net: axienet: quiesce the TX queue across a DMA error reset

[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 (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 patch does not fully close the race it sets out to fix:
  axienet_start_xmit() can still run while the TX ring is being torn…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but axienet_open()
  ignores the return value of axienet_device_reset() and can reach…
- [Medium] At this commit, axienet_dma_err_handler() wakes the TX queue
  after a reset that also reset the AXI Ethernet core (per the comment…
- [Medium] This problem wasn't introduced by this patch, but
  axienet_dma_stop() discards the return value of…

--

net: axienet: quiesce the TX queue across a DMA error reset

This adds a netif_tx_disable() call to axienet_dma_err_handler(), right
after TX NAPI is disabled, so no transmit can run while the TX ring is
torn down. Once the reset is complete, the queue is woken again, unless
the interface is stopping or the device has been detached for suspend.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 6d448d0b523d3..f16dbfc7dc933 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2724,6 +2724,11 @@ static void axienet_dma_err_handler(struct work_struct *work)
>  	napi_disable(&lp->napi_tx);
>  	napi_disable(&lp->napi_rx);
>  
> +	/* With TX NAPI disabled nothing else can wake the queue.  Stop it and
> +	 * wait out any transmit in progress, so the ring can be torn down.
> +	 */
> +	netif_tx_disable(ndev);

[Severity: High]
Is the comment accurate that nothing else can wake the queue here? Two
other paths seem able to clear the DRV_XOFF bit set by netif_tx_disable()
while the handler is still running.

The first path is axienet_open(). axienet_init_legacy_dma() calls
request_irq() for tx_irq and rx_irq, so dma_err_task can be scheduled from
that point on. RX DMA is already running with the error interrupt enabled,
via axienet_device_reset() -> axienet_dma_bd_init() -> axienet_dma_start().
After that, axienet_open() does:

	/* Nothing else clears a stop left over from before the last close:
	 * the ring is empty, so no TX completion will wake the queue.
	 */
	netif_start_queue(ndev);

__dev_open() then calls dev_activate() once ndo_open returns. This driver
does not use the netdev instance lock, so the handler's napi_disable() does
not wait for open to finish.

The second path is axienet_resume():

	rtnl_lock();
	axienet_open(ndev);
	rtnl_unlock();

	netif_device_attach(ndev);

When the device was not present and is running, netif_device_attach()
calls netif_tx_wake_all_queues(). Nothing orders this against
dma_err_task.

Suppose either wake lands while the handler sleeps in axienet_dma_stop().
Can axienet_start_xmit() then publish an skb into lp->tx_bd_v[] that the
handler's loop frees with dev_kfree_skb_irq(), and then read skb->len in
netdev_sent_queue()? The same window would also allow a XAXIDMA_TX_TDESC
kick with a tail that the handler is about to rewind to 0.

That would mean the statement in the commit message, "the handler now
owns the queue state for its whole duration", does not hold on the open
and resume paths.

Would it make sense to start the queue in axienet_open() before the DMA
IRQs are requested, and to order the attach in axienet_resume() against
dma_err_task? The later patch in the series, "net: axienet: reprogram the
link settings after a DMA error reset", does not change this code.

> +
>  	axienet_setoptions(ndev, lp->options &
>  			   ~(XAE_OPTION_TXEN | XAE_OPTION_RXEN));
>  
> @@ -2791,6 +2796,13 @@ static void axienet_dma_err_handler(struct work_struct *work)
>  	napi_enable(&lp->napi_rx);
>  	napi_enable(&lp->napi_tx);
>  	axienet_setoptions(ndev, lp->options);
> +
> +	/* Leave the queue stopped if the interface is going down or the
> +	 * device was detached for suspend: axienet_stop() and axienet_open()
> +	 * own the queue state then.
> +	 */
> +	if (!READ_ONCE(lp->stopping) && netif_device_present(ndev))
> +		netif_wake_queue(ndev);

[Severity: Medium]
This is a pre-existing issue, but at this commit the queue is woken
without the link settings being reprogrammed.

The DMA reset in __axienet_device_reset() also resets the AXI Ethernet
core. The handler does not restore the XAE_EMMC link speed. It also writes
XAE_FCC to XAE_FCC_FCRX_MASK, which drops the negotiated TX pause setting.

Only axienet_mac_link_up() programs those registers, and phylink sees no
link change, so it does not call it again. Does transmission then resume
with the MAC at its reset-default speed and pause configuration, for
example on 10/100 Mb/s links?

The final patch in the series, "net: axienet: reprogram the link settings
after a DMA error reset", resolves this. It adds
phylink_mac_change(lp->phylink, false) before netif_wake_queue(), inside
the same condition.

[Severity: Medium]
This isn't a bug introduced by this patch, but the wake here happens
whether or not the reset actually succeeded. axienet_dma_stop() discards
the result of the reset:

	axienet_lock_mii(lp);
	__axienet_device_reset(lp);
	axienet_unlock_mii(lp);

It also continues after the HALTED polling loops expire.
__axienet_device_reset() returns -ETIMEDOUT when XAXIDMA_CR_RESET_MASK
never clears or XAE_INT_PHYRSTCMPLT_MASK is never set.

Even so, axienet_dma_err_handler() unmaps and clears every descriptor,
rewinds the ring indices, calls axienet_dma_start(), re-enables TX/RX and
now wakes the queue.

If the engine never really halted, can outstanding DMA still reach the
buffers that were just unmapped? And would new transmits be posted to an
engine that never completes them?

>  }
>  
>  /**

[Severity: High]
This is a pre-existing issue in axienet_open(), not introduced by this
patch. The same open path is involved in the race above.

The return value of axienet_device_reset() is overwritten before anything
checks it:

	axienet_lock_mii(lp);
	ret = axienet_device_reset(ndev);
	axienet_unlock_mii(lp);

	ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0);

axienet_device_reset() can fail in three ways:

- __axienet_device_reset() times out before axienet_dma_bd_init() runs.
- The tx_bd_v dma_alloc_coherent() fails and leaves it NULL.
- The RX allocation fails. axienet_dma_bd_init() then calls
  axienet_dma_bd_release(), which frees tx_bd_v with dma_free_coherent()
  but does not clear the pointer.

In every case axienet_open() still registers the IRQs, calls
netif_start_queue() and returns 0.

The next transmit then reaches axienet_check_tx_bd_space():

	cur_p = &lp->tx_bd_v[(READ_ONCE(lp->tx_bd_tail) + num_frag) %
			     lp->tx_bd_num];
	if (cur_p->cntrl)

Can this dereference a NULL or freed tx_bd_v? And could a later
axienet_stop() -> axienet_dma_bd_release() call dma_free_coherent() on
tx_bd_v a second time?

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

Re: [PATCH net v2 4/4] net: axienet: reprogram the link settings after a DMA error reset

[netdev-bot+sashiko]

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

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

- [Medium] In axienet_dma_err_handler()
  (drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2804-2811), the new…

--

net: axienet: reprogram the link settings after a DMA error reset

axienet_dma_err_handler() resets the AXI Ethernet core but never restores
the link speed in XAE_EMMC, and it leaves XAE_FCC with only FCRX set. This
patch calls phylink_mac_change(lp->phylink, false) at the end of the
handler, so phylink takes the link down and back up and reruns
axienet_mac_link_up(). The call uses the same condition as the queue wake.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index f16dbfc7dc933..d53f7dc7bd223 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2801,8 +2801,15 @@ static void axienet_dma_err_handler(struct work_struct *work)
>  	 * device was detached for suspend: axienet_stop() and axienet_open()
>  	 * own the queue state then.
>  	 */
> -	if (!READ_ONCE(lp->stopping) && netif_device_present(ndev))
> +	if (!READ_ONCE(lp->stopping) && netif_device_present(ndev)) {
> +		/* The reset also cleared the link speed and pause settings,
> +		 * which only axienet_mac_link_up() programs.  Have phylink take
> +		 * the link down and up again so that it is called.  This must
> +		 * follow the axienet_setoptions() above, which writes XAE_FCC.
> +		 */
> +		phylink_mac_change(lp->phylink, false);

[Severity: Medium]
Can the netif_device_present() check skip the relink during resume, even
though phylink is already running at that point?

The commit message says:

    Skip it under the same conditions as the queue wake:
    axienet_stop() stops phylink, and a resume restarts it.

In axienet_resume(), though, phylink is restarted before the device is
attached:

axienet_resume()
    rtnl_lock();
    axienet_open(ndev);
        axienet_device_reset(ndev);
        phylink_start(lp->phylink);
        axienet_init_legacy_dma(ndev);
            lp->stopping = false;
            request_irq(lp->tx_irq, ...);
            request_irq(lp->rx_irq, ...);
    rtnl_unlock();

    netif_device_attach(ndev);

Consider this sequence inside that window:

1. phylink resolves the link and calls axienet_mac_link_up(). With a
   fixed-link or in-band PCS this can happen quickly.
2. A TX or RX DMA error interrupt schedules dma_err_task.
3. axienet_dma_err_handler() resets the core through axienet_dma_stop(),
   which clears the XAE_EMMC speed. It also rewrites XAE_FCC to FCRX only.
4. The handler reaches the final check before netif_device_attach() has
   run. It sees lp->stopping == false but netif_device_present() == false,
   so it skips phylink_mac_change().

netif_device_attach() then wakes the TX queue, so the queue wake gets a
second chance. Nothing reruns axienet_mac_link_up(), so the MAC stays at
its reset-default speed and pause settings. That is the state this patch
is meant to fix.

phylink already ignores the notification while it is stopped:

phylink_run_resolve()
    if (!pl->phylink_disable_state)
        queue_work(system_power_efficient_wq, &pl->resolve);

axienet_stop() also sets lp->stopping and calls
flush_work(&lp->dma_err_task) before it calls phylink_stop().

Would it be enough to gate phylink_mac_change() on !lp->stopping alone,
and keep the netif_device_present() check only for netif_wake_queue()?

>  		netif_wake_queue(ndev);
> +	}
>  }

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