Re: [PATCH net-next v7 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 2 potential issue(s) to consider:
- [High] Interface becomes permanently stuck in NO-CARRIER if brought up without a cable and link_irq is not provided.
- [Medium] Race condition in w5100_open() and w5100_resume() causes permanent incorrect carrier state due to stale read.
--

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

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

This commit adds link status detection for w5500 devices using the PHY
Configuration register. It also introduces an optional interrupt to track
subsequent link state changes.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..ecb99621d0a1e 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c

[ ... ]

> @@ -659,6 +711,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 (w5100_get_link(ndev))
> +		netif_carrier_on(ndev);

[Severity: Medium]
Could this introduce a race condition where a stale link state overwrites
a newer one? If the physical link state changes exactly after
w5100_get_link() reads the hardware register but before this thread
evaluates the result, the newly unmasked edge-triggered interrupt handler
might run first. The handler would read the new state and update the carrier,
but then the main thread would resume and apply its stale read, overwriting
the correct state. Since the interrupt is edge-triggered, no further
interrupts would correct this until the link changes again.

> +	else
> +		netif_carrier_off(ndev);

[Severity: High]
Does this leave the interface permanently stuck without carrier if the
link_irq is not provided? If the interface is brought up while the cable is
disconnected, this explicitly calls netif_carrier_off(). Without the optional
interrupt (which is the case for most legacy device trees), there appears
to be no polling mechanism to ever turn the carrier back on when a cable is
subsequently plugged in.

> +
>  	return 0;
>  }

[ ... ]

> @@ -857,6 +952,12 @@ static int w5100_resume(struct device *dev)
>  		w5100_hw_start(priv);
>  
>  		netif_device_attach(ndev);
> +
> +		if (priv->link_irq >= 0)
> +			enable_irq(priv->link_irq);
> +
> +		if (w5100_get_link(ndev))
> +			netif_carrier_on(ndev);

[Severity: Medium]
Could this face the same race condition as w5100_open()? The edge-triggered
interrupt is enabled before the local state read, so a concurrent physical
link change could trigger the interrupt handler, update the carrier, and
then be overwritten by the stale state read here.

>  	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f@marmottus.net?part=3