Re: [PATCH net-next v8 3/3] w5100: detect carrier state using link status bit and optional interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: w5100: teardown race condition due to inverted cancel/free order
--

commit a7c6f85022e2b47daf620a6b97b050a3490bd3ef
Author: Arthur Crepin Leblond <arthur@marmottus.net>

w5100: detect carrier state using link status bit and optional interrupt

This commit adds carrier state detection using the Link Status bit on the
w5500. It introduces an optional interrupt for link status changes and ensures
the netdev is registered last during probe.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c

[ ... ]

> @@ -820,14 +953,22 @@ void w5100_remove(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
> -	w5100_hw_reset(priv);
> +	unregister_netdev(ndev);
> +
> +	cancel_work_sync(&priv->rx_work);
> +	cancel_work_sync(&priv->tx_work);
> +	cancel_work_sync(&priv->setrx_work);
> +	cancel_work_sync(&priv->restart_work);
> +
> +	if (priv->link_irq > 0)
> +		free_irq(priv->link_irq, ndev);
> +
>  	free_irq(priv->irq, ndev);

[Severity: High]
Does this code introduce a teardown race by canceling the work queues before
freeing the interrupts?

If the threaded interrupt handler (w5100_interrupt) is running concurrently
during w5100_remove(), it could queue a new rx_work or tx_work immediately
after cancel_work_sync() returns.

Later in the teardown, destroy_workqueue() drains and forcibly executes all
pending works. Since w5100_hw_reset() is called prior to destroy_workqueue(),
could these newly queued works execute against a reset and uninitialized
hardware state, potentially causing I/O corruption or kernel crashes?

Should free_irq() be called before cancel_work_sync() to guarantee the
interrupt handler has completed and no new works will be queued?

> +	w5100_hw_reset(priv);
>  
> -	flush_work(&priv->setrx_work);
> -	flush_work(&priv->restart_work);
>  	destroy_workqueue(priv->xfer_wq);
> +	mutex_destroy(&priv->link_lock);
>  
> -	unregister_netdev(ndev);
>  	free_netdev(ndev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-wiznet-link-gpio-v8-0-d173622474cc@marmottus.net?part=3