[PATCH net] net: axienet: restart the TX queue after a DMA error reset

[PATCH net] net: axienet: restart the TX queue after a DMA error reset

[Sagi Maimon]

axienet_dma_err_handler() resets the DMA engine, frees every TX
descriptor and sets lp->tx_bd_ci and lp->tx_bd_tail back to 0, but it
never restarts the transmit queue.

If the queue was stopped by axienet_start_xmit() because the ring was
full - __QUEUE_STATE_DRV_XOFF - it stays stopped.  The only
queue-state clearing the handler does is netdev_reset_queue(), which
touches __QUEUE_STATE_STACK_XOFF and does nothing at all when
CONFIG_BQL is disabled.  The remaining wake in axienet_tx_poll() is
reached only when axienet_free_tx_chain() returns a non-zero packet
count, which cannot happen after the handler has cleared the status
word of every descriptor, and the wake in axienet_start_xmit() cannot
run while the queue is stopped.

The transmit timestamp is not refreshed either, so on a kernel with a
TX timeout handler the watchdog condition stays true and the reset can
be retried indefinitely.  axienet_device_reset(), the driver's other
reset path, already calls netif_trans_update() for this reason.

Wake the queue and refresh the timestamp at the end of the handler.
The descriptor ring has just been reinitialised at that point, so it is
ready to accept transmits.

Fixes: 8a3b7a252dca ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
Compile-tested only; the board I found this on is not available to me
for about a month.  The BQL/STACK_XOFF case was exercised on hardware on
2026-08-26 and did recover, which is why the gap went unnoticed - see the
discussion on the TX timeout patch.  The DRV_XOFF case is reasoned from
the code, not measured.

axienet_dma_err_handler() also tears down the TX ring without excluding
axienet_start_xmit().  That is a separate pre-existing problem and needs
a separate patch; I would rather send it once I can test it.
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 782f903d318f..b6440c06f260 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2778,6 +2778,15 @@ 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);
+
+	/* The ring is empty again, so let the stack transmit.  The queue may
+	 * have been stopped by axienet_start_xmit(); netdev_reset_queue()
+	 * above clears only __QUEUE_STATE_STACK_XOFF, and nothing at all
+	 * without CONFIG_BQL.  Refresh the transmit timestamp first, or the
+	 * watchdog fires again on the next tick.
+	 */
+	netif_trans_update(ndev);
+	netif_wake_queue(ndev);
 }
 
 /**
-- 
2.47.0

Re: [PATCH net] net: axienet: restart the TX queue after a DMA error reset

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (2) · Medium: 1 · Low: 0 (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 netif_wake_queue() added at the end of
  axienet_dma_err_handler() is not serialized against…
- [High] The unconditional netif_wake_queue() added at the end of
  axienet_dma_err_handler() can undo the TX stop that axienet_suspend()…
- [Medium] The added in-code comment in axienet_dma_err_handler() and the
  commit message assert a TX-watchdog failure that cannot happen in this…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  axienet_dma_err_handler() unmaps, frees and clears every TX descriptor…
- [High] This problem wasn't introduced by this patch, but
  axienet_dma_err_handler() performs a full AXI Ethernet core reset…
- [Low] Concern that the newly added unconditional netif_wake_queue() in
  axienet_dma_err_handler() re-enables transmission even when the…

--

net: axienet: restart the TX queue after a DMA error reset

The patch adds netif_trans_update() and netif_wake_queue() at the end of
axienet_dma_err_handler(), so that a queue stopped by axienet_start_xmit()
is restarted after the DMA error reset has reinitialised the descriptor
ring.  No Link: tags are present in the commit header.

A few questions below about the new wake and about the surrounding recovery
sequence.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..814d92fcb4fda 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2778,6 +2778,15 @@ 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);
> +
> +	/* The ring is empty again, so let the stack transmit.  The queue may
> +	 * have been stopped by axienet_start_xmit(); netdev_reset_queue()
> +	 * above clears only __QUEUE_STATE_STACK_XOFF, and nothing at all
> +	 * without CONFIG_BQL.  Refresh the transmit timestamp first, or the
> +	 * watchdog fires again on the next tick.
> +	 */

[Severity: Medium]
Can the watchdog actually fire for this device?  The comment says:

	 * without CONFIG_BQL.  Refresh the transmit timestamp first, or the
	 * watchdog fires again on the next tick.

and the commit message states it as a fact:

    The transmit timestamp is not refreshed either, so on a kernel with a
    TX timeout handler the watchdog condition stays true and the reset can
    be retried indefinitely.

Neither axienet_netdev_ops nor axienet_netdev_dmaengine_ops installs
.ndo_tx_timeout, and the driver never assigns ndev->watchdog_timeo
(a grep for both symbols in drivers/net/ethernet/xilinx/ matches only
xilinx_emaclite.c).  The core refuses to arm the timer in that case:

net/sched/sch_generic.c:netdev_watchdog_up() {
	if (!dev->netdev_ops->ndo_tx_timeout)
		return;
	...
}

So dev_watchdog() never runs for axienet and netif_trans_update() is a
timestamp store with no in-tree consumer.  The note below the --- line
mentions this depends on a separate TX timeout patch, but that note does
not reach git history, and the Fixes: tag invites a stable backport of a
watchdog retry loop no in-tree axienet can produce.  Could the comment and
the commit message be reworded to say the timestamp refresh is preparation
for a TX timeout handler rather than a fix for observed behaviour?

> +	netif_trans_update(ndev);
> +	netif_wake_queue(ndev);

[Severity: High]
Can this wake be lost, leaving the queue stopped for good?

axienet_dma_err_handler() runs in process context and never calls
netif_tx_disable() or netif_tx_lock_bh(), and it sleeps for tens of
milliseconds inside axienet_dma_stop().  An axienet_start_xmit() that
entered before the teardown can therefore finish after the worker has
reset the indices:

axienet_dma_err_handler()
	lp->tx_bd_ci = 0;
	lp->tx_bd_tail = 0;
	...
	netif_wake_queue(ndev);

axienet_start_xmit() then runs its own stop protocol:

	if (axienet_check_tx_bd_space(lp, MAX_SKB_FRAGS + 1)) {
		netif_stop_queue(ndev);

		/* Matches barrier in axienet_tx_poll */
		smp_mb();

If that netif_stop_queue() lands after the worker's netif_wake_queue(),
__QUEUE_STATE_DRV_XOFF is set again with nothing left to clear it.  The
only other wake is inside the 'if (packets)' branch of axienet_tx_poll():

		if (!axienet_check_tx_bd_space(lp, MAX_SKB_FRAGS + 1))
			netif_wake_queue(ndev);

and the commit message already explains axienet_free_tx_chain() cannot
return a non-zero packet count once the handler has zeroed every status
word.

The existing queue-state protocol is a two-party smp_mb() handshake
between axienet_start_xmit() and axienet_tx_poll(); this adds a third
writer from a different context that does not take part in it.  Would
quiescing the transmit path at the top of the handler (netif_tx_disable())
and waking at the end make the wake unloseable?

Related: lp->tx_bd_ci and lp->tx_bd_tail are written with plain stores by
the worker while axienet_start_xmit() reads them unlocked
(orig_tail_ptr = lp->tx_bd_tail) and WRITE_ONCEs them, so is the comment's
premise that "the ring is empty again" guaranteed to hold at the instant
of the wake?

[Severity: High]
Can this wake undo the transmit stop that suspend installed?

The worker checks lp->stopping only at its entry.  If a suspend starts
after that check:

axienet_suspend()
	netif_device_detach(ndev);

	rtnl_lock();
	axienet_stop(ndev);
	rtnl_unlock();

net/core/dev.c:netif_device_detach() {
	if (test_and_clear_bit(__LINK_STATE_PRESENT, &dev->state) &&
	    netif_running(dev)) {
		netif_tx_stop_all_queues(dev);
	}
}

axienet_stop() then sets lp->stopping and blocks:

		WRITE_ONCE(lp->stopping, true);
		flush_work(&lp->dma_err_task);
		...
		axienet_dma_bd_release(ndev);

The already-running worker reaches the new netif_wake_queue(), clearing
the DRV_XOFF that detach set and scheduling the qdisc.  Suspend calls
axienet_stop() directly without dev_deactivate(), so the qdisc is still
active, and the transmit path does not consult netif_device_present().
Can backlogged traffic then enter axienet_start_xmit() after flush_work()
returns, while axienet_stop() proceeds through napi_disable(),
axienet_dma_stop(), free_irq() and axienet_dma_bd_release(), so that the
in-flight transmit writes into lp->tx_bd_v as it is being dma_free'd and
kicks XAXIDMA_TX_TDESC on a torn-down engine?

Before this patch the detach-installed DRV_XOFF stayed set, so no xmit
could run.  Would re-checking lp->stopping (or netif_device_present())
before the wake close this?

[Severity: Low]
This isn't a bug introduced by this patch, but the hardware reset result
is discarded on this path.  __axienet_device_reset() returns -ETIMEDOUT
when the DMA reset bit never clears or PhyRstCmplt never arrives:

	if (ret) {
		dev_err(lp->dev, "%s: timeout waiting for PhyRstCmplt\n", __func__);
		goto out;
	}

but axienet_dma_stop() drops it:

	/* Do a reset to ensure DMA is really stopped */
	axienet_lock_mii(lp);
	__axienet_device_reset(lp);
	axienet_unlock_mii(lp);

so the handler cannot tell a recovered device from one still in reset, and
it unconditionally re-enables XAE_OPTION_TXEN via
axienet_setoptions(ndev, lp->options) just above the new wake.  The added
netif_wake_queue() does not make this worse - transmits already reached the
possibly-unreset engine in the common case where the queue was not stopped
when the error hit - but should axienet_dma_stop() propagate the reset
failure instead of returning void?

[Severity: High]
This is a pre-existing issue, but the ring teardown earlier in this same
handler has no exclusion against axienet_start_xmit(), which the commit
message acknowledges below the --- line.  The handler does, for every TX
descriptor:

		if (cur_p->skb)
			dev_kfree_skb_irq(cur_p->skb);
		cur_p->phys = 0;
		...
		cur_p->skb = NULL;

while the transmitter publishes the skb and then dereferences it:

	cur_p->skb = skb;
	...
	WRITE_ONCE(lp->tx_bd_tail, new_tail_ptr);
	netdev_sent_queue(ndev, skb->len);

Disabling TX NAPI excludes completion processing, not ndo_start_xmit.  If
the worker frees that skb between the two, is the skb->len read a
use-after-free, and does the following tail publication plus
XAXIDMA_TX_TDESC kick submit descriptors whose mappings the worker already
released?

[Severity: High]
This isn't a bug introduced by this patch, but is the MAC fully
reprogrammed at the point where the queue is now woken?

axienet_dma_stop() runs __axienet_device_reset(), which resets the
Ethernet core, and the recovery sequence restores RCW1, IS/IE, FCC (FCRX
only), the MAC address, the multicast filter and lp->options.  It never
rewrites XAE_EMMC_OFFSET.  The only writer of the LINKSPEED field and of
the negotiated pause bits is axienet_mac_link_up():

	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
	...
	axienet_iow(lp, XAE_EMMC_OFFSET, emmc_reg);

and phylink will not call it again because the link state has not changed
(axienet_mac_link_down() is a no-op and the handler does not re-trigger
phylink).  axienet_options[] has no XAE_EMMC_OFFSET entry either, so
axienet_setoptions() cannot repair it.

On a 10/100 link, does the MAC keep its reset-default link speed after
this recovery while software still believes the negotiated rate is in
effect, so that the frames the new netif_wake_queue() lets through are
clocked at the wrong rate until an unrelated link flap re-runs
axienet_mac_link_up()?  The open path escapes this only because
axienet_open() runs phylink_start() after axienet_device_reset().

>  }
>  
>  /**

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