Re: [PATCH net-next v5 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 3 potential issue(s) to consider:
- [High] W5500 devices without the optional link interrupt can become permanently stuck with the carrier off if initialized while disconnected.
- [High] Reordering unregister_netdev() before flush_work() leaves the hardware active and unmasked after driver removal.
- [Medium] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition in link state initialization.
--

--- Patch [3]: [PATCH net-next v5 3/3] w5100: detect carrier state using link status bit and optional interrupt ---
commit d746a0d78f3b21d66ca77ce59de3867ee0ba026e
Author: Arthur Crepin Leblond <arthur@marmottus.net>

w5100: detect carrier state using link status bit and optional interrupt
    
Detect the link status on the w5500 using the Link Status bit from the
PHY Configuration register. Add an optional interrupt to detect link
status changes and read the PHYCFGR register. Make sure the netdev is
registered last in the probe function.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..b5976d5e6c60c 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c
[ ... ]
> @@ -659,6 +705,15 @@ static int w5100_open(struct net_device *ndev)
>  	w5100_hw_start(priv);
>  	napi_enable(&priv->napi);
>  	netif_start_queue(ndev);
> +
> +	if (priv->link_irq >= 0)
> +		enable_irq(priv->link_irq);
> +
> +	if (priv->ops->chip_id != W5500 || w5500_get_phycfgr_lnk(ndev))
> +		netif_carrier_on(ndev);
> +	else
> +		netif_carrier_off(ndev);
> +

[Severity: Medium]
Could this introduce a race condition during link state initialization?

If the physical link changes state right after enable_irq() is called but
before the manual PHY state check:

CPU1                                      CPU2
w5100_open()
   enable_irq()
                                          w5100_detect_link()
                                             w5100_get_link() -> link up
                                             netif_carrier_on()
   w5500_get_phycfgr_lnk() -> link down
   netif_carrier_off()

Will this leave the interface permanently stuck in a no-carrier state despite
having a valid physical link?

[Severity: High]
Does this code permanently break connectivity for setups without a link
interrupt?

If the device tree does not provide the optional link_irq, and the driver is
initialized while the physical cable is disconnected, netif_carrier_off() is
called here.

Since there is no polling mechanism to detect a subsequent link-up event, how
will the carrier ever be turned back on once the cable is plugged in?

>  	return 0;
>  }
[ ... ]
> @@ -820,6 +902,13 @@ void w5100_remove(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
> +	unregister_netdev(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);
>  
> @@ -827,7 +916,6 @@ void w5100_remove(struct device *dev)
>  	flush_work(&priv->restart_work);
>  	destroy_workqueue(priv->xfer_wq);
>  
> -	unregister_netdev(ndev);
>  	free_netdev(ndev);
>  }

[Severity: High]
Can this reordering leave the hardware active after the driver is removed?

unregister_netdev() was moved to the top of w5100_remove(), which gracefully
stops the hardware early via ndo_stop(). 

However, flush_work() is called later in the teardown process:

w5100_remove()
   unregister_netdev()
      ...
         w5100_stop()
            w5100_hw_close()
   ...
   flush_work(&priv->setrx_work)
      w5100_setrx_work()
         w5100_hw_start()

If setrx_work or restart_work are pending in the workqueue, they will execute
and call w5100_hw_start(), which re-enables hardware interrupts and opens the
socket after the OS interrupt handler has been freed. Is it safe to execute
flush_work() after unregister_netdev()?

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