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 Mon, Sep 21, 2026 at 11:04:34AM +0200, Arthur Crepin Leblond wrote:
>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

The easiest is probably to always run w5100_restart in worker.

Arthur