Re: [PATCH net-next v9 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 1 potential issue(s) to consider:
- [Medium] net: w5100: sleep in atomic context in w5100_restart()
--

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

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

This commit introduces carrier state detection for the w5500 Ethernet
controller via the PHY Configuration register. It uses an optional interrupt
wired to the LINKLED pin to handle link status changes, and refactors several
aspects of the driver's initialization and teardown sequences.

> 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

[ ... ]

> @@ -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?

>  	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?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-wiznet-link-gpio-v9-0-5536f24d8d6d@marmottus.net?part=3