Re: [PATCH net-next v6 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] Teardown order and race condition in `w5100_remove`
--

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

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

This commit detects the link status on the w5500 using the Link Status
bit from the PHY Configuration register. It also adds an optional
interrupt wired to the LINKLED pin of the w5500 to detect link status
changes, and registers the netdev last in the probe function.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..d91881ecd797c 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c
[ ... ]
> @@ -667,6 +722,10 @@ static int w5100_stop(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifdown, ndev, "shutting down\n");
> +
> +	if (priv->link_irq >= 0)
> +		disable_irq(priv->link_irq);

[Severity: High]
In w5100_stop() (drivers/net/ethernet/wiznet/w5100.c), can w5100_remove()
and w5100_stop() race here?

If an interface shutdown (ndo_stop) executes concurrently with w5100_remove(),
w5100_stop() could evaluate priv->link_irq >= 0 to true, get preempted, and
then call disable_irq() after w5100_remove() has already freed the interrupt
descriptor.

[ ... ]
> @@ -820,14 +902,20 @@ void w5100_remove(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
> +	if (priv->link_irq >= 0) {
> +		free_irq(priv->link_irq, ndev);
> +		priv->link_irq = -ENODEV;
> +	}
> +
>  	w5100_hw_reset(priv);
>  	free_irq(priv->irq, ndev);
>  
> +	unregister_netdev(ndev);

[Severity: High]
In w5100_remove() (drivers/net/ethernet/wiznet/w5100.c), does this expose
the driver to concurrent operations on reset hardware?

unregister_netdev() is called after w5100_hw_reset() and free_irq(), which
means the network stack might still be actively transmitting or calling into
the driver while the hardware is in a reset state.

This is a pre-existing issue, but this change exacerbates it by introducing
the link_irq race described above.

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

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