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

[Arthur Crepin Leblond <arthur@marmottus.net>]

On Sat, Sep 05, 2026 at 12:36:29PM +0000, sashiko-bot@kernel.org wrote:
>> @@ -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.

Yes, I'll bring a mutex/lock to the next patch.

>
>> +	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.

I think the best here imho would be to just keep the original behavior
if there is no link irq set and to not touch netif_carrier_on/off at
all.

w5100/w5200 -> no .get_link, no netif_carrier
w5500 -> .get_link but netif_carrier only if link irq is set

Arthur