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

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

On Sat, Sep 19, 2026 at 03:14:20PM +0000, sashiko-bot@kernel.org wrote:
>> @@ -448,12 +526,25 @@ static void w5100_restart(struct net_device *ndev)
>>  {
>>  	struct w5100_priv *priv = netdev_priv(ndev);
>>
>> +	if (!netif_running(ndev) || !netif_device_present(ndev))
>> +		return;
>> +
>> +	disable_irq(priv->irq);
>> +	if (priv->link_irq > 0)
>> +		disable_irq(priv->link_irq);
>> +
>
>[Severity: Medium]
>Is it safe to call disable_irq() here? disable_irq() is a sleepable function,
>but w5100_restart() can be called from atomic context when using memory-mapped
>I/O.
>
>Looking at w5100_tx_timeout(), which executes in atomic context, the timeout
>handler falls back to calling w5100_restart() directly if the device cannot
>sleep:
>
>w5100_tx_timeout() {
>    ...
>    if (priv->ops->may_sleep)
>        schedule_work(&priv->restart_work);
>    else
>        w5100_restart(ndev);
>}
>
>While current in-tree drivers might set may_sleep to true, this code path
>explicitly exists to support may_sleep == false. Could this introduce a
>'scheduling while atomic' panic for non-sleepable devices?

It looks like all the ops in the spi driver are setting may_sleep to
true but indeed disable_irq should not be called from an atomic context.

Would disable_irq_nosync just work here? enable_irq is not affected
from what I understand.

>
>>  	netif_stop_queue(ndev);
>>  	w5100_hw_reset(priv);
>> +	enable_irq(priv->irq);
>>  	w5100_hw_start(priv);
>>  	ndev->stats.tx_errors++;
>>  	netif_trans_update(ndev);
>>  	netif_wake_queue(ndev);
>> +
>> +	if (priv->link_irq > 0) {
>> +		w5500_report_carrier_state(ndev);
>> +		enable_irq(priv->link_irq);
>> +	}
>
>[Severity: Medium]
>Similarly, w5500_report_carrier_state() attempts to acquire a mutex
>(priv->link_lock).
>
>If priv->link_irq > 0 and priv->ops->may_sleep is false, wouldn't taking
>this mutex in the w5100_tx_timeout() -> w5100_restart() call chain also
>result in sleeping in atomic context?
>
>>  }
>

Maybe I could use a spin lock instead of a mutex.

Arthur