Re: [PATCH net 3/3] net: xgmac: disable timeout work before teardown

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 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] xgmac_resume() can deadlock against the timeout work.
- [Medium] The workqueue disable count can get stuck above zero and stay
  that way, so TX-hang recovery is lost for good.
- [Medium] In xgmac_suspend(), netif_device_detach() (line 1887) runs
  before disable_work_sync() (line 1893).
- [Low] The new comments describe the enable/disable pairing wrongly.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits on the
  same stop/failed-open path.

--

net: xgmac: disable timeout work before teardown

This patch disables tx_timeout_work at probe and re-enables it in
xgmac_open() and xgmac_resume(). It also synchronously disables the work
in xgmac_stop(), xgmac_suspend() and xgmac_remove(). The goal is to keep
the TX timeout work from running against freed DMA rings, released IRQs
or unmapped registers.

> diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
> index a2410fba6be2e..9b7aee56f2610 100644
> --- a/drivers/net/ethernet/calxeda/xgmac.c
> +++ b/drivers/net/ethernet/calxeda/xgmac.c
> @@ -1023,6 +1023,9 @@ static int xgmac_open(struct net_device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* xgmac_stop() disables this work before freeing the rings. */
> +	enable_work(&priv->tx_timeout_work);
> +

[Severity: Medium]
Can the disable count on tx_timeout_work get stuck above zero here?

disable_work() and enable_work() nest, so every xgmac_stop() needs a
matching xgmac_open() that succeeds. xgmac_change_mtu() does:

	xgmac_stop(dev);
	WRITE_ONCE(dev->mtu, new_mtu);
	return xgmac_open(dev);

If xgmac_dma_desc_rings_init() fails with -ENOMEM, xgmac_open() returns
before enable_work() and the count stays at 1. A failed ndo_change_mtu
does not close the device, so it stays running.

A later ifdown calls xgmac_stop() again, and disable_work_sync() raises
the count to 2. The next ifup succeeds, but its enable_work() only brings
the count back to 1.

>From then on, schedule_work() from xgmac_tx_timeout() and from the
DMA_STATUS_TPS path in xgmac_interrupt() does nothing until the driver
is rebound. Without this patch, a down/up cycle fully recovered the
device.

Would enabling the work on the open failure path, or tracking whether it
is currently enabled, avoid this?

[Severity: High]
This isn't a bug introduced by this patch, but it is on the same
xgmac_change_mtu() stop/failed-open path. The error labels in
xgmac_dma_desc_rings_init() free the buffers without clearing the
pointers:

err_dma_tx:
	kfree(priv->tx_skbuff);
err_tx_skb:
	dma_free_coherent(priv->device,
			  DMA_RX_RING_SZ * sizeof(struct xgmac_dma_desc),
			  priv->dma_rx, priv->dma_rx_phy);
err_dma_rx:
	kfree(priv->rx_skbuff);
	return -ENOMEM;

When xgmac_change_mtu()->xgmac_open() fails here, the interface stays
running while priv->rx_skbuff, priv->dma_rx and priv->tx_skbuff can
point at freed memory.

The next ndo_stop then goes through
xgmac_stop()->xgmac_free_dma_desc_rings(). There,
xgmac_free_rx_skbufs() walks the freed rx_skbuff[] array, and
dma_free_coherent() on priv->dma_rx and kfree() on
priv->rx_skbuff/priv->tx_skbuff can run a second time.

Could these error labels set the pointers to NULL after freeing them?

>  	/* Enable the MAC Rx/Tx */
>  	xgmac_mac_enable(ioaddr);
>  
> @@ -1046,6 +1049,13 @@ static int xgmac_stop(struct net_device *dev)
>  {
>  	struct xgmac_priv *priv = netdev_priv(dev);
>  
> +	/*
> +	 * The timeout work programs the DMA rings that are freed below, and
> +	 * it toggles NAPI.  Disable it synchronously before touching either;
> +	 * xgmac_open() and xgmac_resume() enable it again.
> +	 */

[Severity: Low]
This isn't a bug, but the comment here and the one added in
xgmac_probe() describe a pairing that doesn't match the code. This
comment says "xgmac_open() and xgmac_resume() enable it again". The
probe comment says the probe-time disable_work() is the matching disable
for "the enable_work() in xgmac_open()/xgmac_resume()".

xgmac_resume() returns early when !netif_running(ndev), and the device
is not running after dev_close()->ndo_stop. So the enable in resume can
never balance a disable done in xgmac_stop() or probe. It is balanced by
the disable_work_sync() in xgmac_suspend().

Could the comments describe the actual pairs, probe/stop with open and
suspend with resume?

> +	disable_work_sync(&priv->tx_timeout_work);
> +

[ ... ]

> @@ -1862,6 +1885,12 @@ static int xgmac_suspend(struct device *dev)
>  		return 0;
>  
>  	netif_device_detach(ndev);
> +	/*
> +	 * The timeout work touches the DMA registers and toggles NAPI.
> +	 * Disable it before NAPI and the interrupts are torn down;
> +	 * xgmac_resume() enables it again.
> +	 */
> +	disable_work_sync(&priv->tx_timeout_work);

[Severity: Medium]
Should disable_work_sync() come before netif_device_detach() here?

If xgmac_tx_timeout_work() is already running when xgmac_suspend() is
called, disable_work_sync() waits for it to finish. The work ends with:

	netif_tx_unlock(priv->dev);
	netif_wake_queue(priv->dev);

That undoes the netif_tx_stop_all_queues() done by netif_device_detach().
Nothing later in xgmac_suspend() stops the queue again. xgmac_stop(), by
contrast, calls netif_tx_disable().

The qdisc path (sch_direct_xmit()->dev_hard_start_xmit()) doesn't check
netif_device_present(). Can xgmac_xmit() then write descriptors and
XGMAC_DMA_TX_POLL while NAPI, interrupts and the MAC/DMA are being shut
down for suspend?

>  	napi_disable(&priv->napi);
>  	writel(0, priv->base + XGMAC_DMA_INTR_ENA);
>  
> @@ -1891,6 +1920,7 @@ static int xgmac_resume(struct device *dev)
>  
>  	/* Enable the MAC and DMA */
>  	xgmac_mac_enable(ioaddr);
> +	enable_work(&priv->tx_timeout_work);
>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_STATUS);
>  	writel(DMA_INTR_DEFAULT_MASK, ioaddr + XGMAC_DMA_INTR_ENA);
>  

[Severity: High]
Can system resume deadlock against the timeout work here?

At this point NAPI is still disabled from xgmac_suspend(). The work is
already enabled, and DMA_INTR_DEFAULT_MASK (which includes TSE) is
unmasked before napi_enable() runs. A "transmit process stopped"
interrupt in this window takes this path in xgmac_interrupt():

	if (intr_status & DMA_STATUS_TPS) {
		...
		schedule_work(&priv->tx_timeout_work);
	}

The work then starts with napi_disable() on a NAPI that is already
disabled:

worker
xgmac_tx_timeout_work()
  napi_disable()
    netdev_lock(n->dev)
    napi_disable_locked()
      loops in usleep_range() while NAPIF_STATE_SCHED is set

resume
xgmac_resume()
  napi_enable()
    netdev_lock(n->dev)    <- blocks on the worker

Only napi_enable_locked() clears SCHED, and it needs the netdev_lock the
worker holds. Neither side can make progress.

This window existed before the patch. However, the suspend hunk is meant
to keep the work from toggling NAPI while it is down, and the
enable_work() placement here leaves it open. xgmac_open() also enables
the work before napi_enable(), but there the DMA interrupts stay masked
until after napi_enable().

Would moving enable_work() and the interrupt unmask after napi_enable()
in xgmac_resume() avoid this?

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