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

[netdev-bot+sashiko@kernel.org]

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