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

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

On Tue, Sep 22, 2026 at 10:53:15AM +0000, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>- [High] net: w5100: race in w5100_suspend due to incorrect detach ordering and missing work flushes
>--
>
>commit 4b50b6b7047dc53f16eedfdba4f4a537ef54c0fd
>Author: Arthur Crepin Leblond <arthur@marmottus.net>
>
>w5100: detect carrier state using link status bit and optional interrupt
>
>This commit adds support for detecting the carrier state on the w5500 Ethernet
>controller using the Link Status bit and an optional interrupt. It also
>refactors the bring-up and teardown operations, including changes to when
>net device registration and work queue cancellations occur.
>
>> 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
>[ ... ]
>> @@ -839,9 +997,20 @@ static int w5100_suspend(struct device *dev)
>>  	struct w5100_priv *priv = netdev_priv(ndev);
>>
>>  	if (netif_running(ndev)) {
>> -		netif_carrier_off(ndev);
>> +		disable_irq(priv->irq);
>> +		if (priv->link_irq > 0)
>> +			disable_irq(priv->link_irq);
>> +
>> +		cancel_work_sync(&priv->restart_work);
>> +
>>  		netif_device_detach(ndev);
>
>[Severity: High]
>Does this code leave a race condition in w5100_suspend() due to the detach
>ordering?
>
>If a TX timeout occurs between cancel_work_sync(&priv->restart_work) and
>netif_device_detach(), wouldn't restart_work be re-scheduled because the
>netdev watchdog is still active?
>
>Also, rx_work, tx_work, setrx_work, and napi do not appear to be flushed or
>disabled in w5100_suspend(), unlike in w5100_stop(). Could this allow them
>to execute concurrently with or after w5100_hw_close(priv) and access
>suspended SPI hardware?
>
>>
>> +		if (priv->link_irq > 0) {
>> +			mutex_lock(&priv->link_lock);
>> +			netif_carrier_off(ndev);
>> +			mutex_unlock(&priv->link_lock);
>> +		}
>> +
>>  		w5100_hw_close(priv);
>>  	}
>>  	return 0;
>
>--
>Sashiko AI review · https://sashiko.dev/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2@marmottus.net?part=3

Probably very low chances to happen but YES, w5100_suspend() could
cleanup the same way as w5100_stop()

Arthur