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

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

On Mon, Sep 28, 2026 at 10:23:18AM +0000, netdev-bot+sashiko@kernel.org wrote:
>> +static irqreturn_t w5500_detect_link_interrupt(int irq, void *ndev_instance)
>> +{
>> +	struct net_device *ndev = ndev_instance;
>> +
>> +	if (netif_running(ndev))
>> +		w5500_report_carrier_state(ndev);
>> +
>> +	return IRQ_HANDLED;
>> +}
>
>[Severity: Low]
>What happens if all three PHYCFGR reads in w5500_get_phycfgr_lnk() fail on
>the link-up edge?

The driver will log the error but I do not think this patch can much
more than that.

>[ ... ]
>
>> @@ -448,12 +531,28 @@ 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);
>> +
>>  	netif_stop_queue(ndev);
>> +	w5100_hw_close(priv);
>> +	flush_work(&priv->rx_work);
>> +	flush_work(&priv->tx_work);
>>  	w5100_hw_reset(priv);
>
>[Severity: Low]
>This isn't a bug introduced by this patch, but netif_stop_queue() doesn't
>wait for an ndo_start_xmit that is already running. Can w5100_start_tx()
>still queue tx_work after flush_work(&priv->tx_work) here?
>
>w5100_start_tx() {
>	...
>	if (priv->ops->may_sleep) {
>		WARN_ON(priv->tx_skb);
>		priv->tx_skb = skb;
>		queue_work(priv->xfer_wq, &priv->tx_work);
>	...
>}
>
>If it can, tx_work could issue SPI transfers at the same time as
>w5100_hw_reset() during tx timeout recovery. The baseline w5100_restart()
>had no flush at all, so this patch narrows the window rather than opening
>it.

True, the low risk still exists.

>[ ... ]
>
>> @@ -667,13 +772,40 @@ static int w5100_stop(struct net_device *ndev)
>>  	struct w5100_priv *priv = netdev_priv(ndev);
>>
>>  	netif_info(priv, ifdown, ndev, "shutting down\n");
>> -	w5100_hw_close(priv);
>> -	netif_carrier_off(ndev);
>> +
>> +	disable_irq(priv->irq);
>> +	if (priv->link_irq > 0)
>> +		disable_irq(priv->link_irq);
>> +
>>  	netif_stop_queue(ndev);
>>  	napi_disable(&priv->napi);
>> +
>> +	cancel_work_sync(&priv->restart_work);
>
>[Severity: Medium]
>Can a restart_work that is already running wake the queue again after the
>netif_stop_queue() above?
>

With the correct timing, technically yes it could.

>[ ... ]
>
>> @@ -839,10 +1011,11 @@ static int w5100_suspend(struct device *dev)
>>  	struct w5100_priv *priv = netdev_priv(ndev);
>>
>>  	if (netif_running(ndev)) {
>> -		netif_carrier_off(ndev);
>> +		mutex_lock(&priv->link_lock);
>>  		netif_device_detach(ndev);
>> +		mutex_unlock(&priv->link_lock);
>>
>> -		w5100_hw_close(priv);
>> +		w5100_stop(ndev);
>>  	}
>>  	return 0;
>>  }
>
>[Severity: Medium]
>What happens if the core calls ndo_stop between this suspend and the
>matching resume?
>
>Here w5100_stop() disables both irqs and calls napi_disable(). However,
>__LINK_STATE_START stays set, and suspend takes neither rtnl nor the netdev
>lock.
>
>A kernel-initiated dev_close() can run during the suspend window, for
>example from cleanup_net() on the non-freezable netns workqueue. In that
>case w5100_stop() runs a second time:
>
>__dev_close_many()
>  w5100_stop()
>    disable_irq(priv->irq)          <- disable depth now 2
>    napi_disable(&priv->napi)
>      napi_disable_locked()         <- SCHED/NPSVC still set from the
>                                       first napi_disable()
>
>napi_disable_locked() would then spin forever in usleep_range() while
>holding rtnl.
>
>After that, netif_running() is false, so w5100_resume() skips w5100_open().
>The irq disable depth stays raised, and a later ifup leaves both irqs
>disabled.
>
>Before this patch, suspend only called w5100_hw_close(), which touched
>neither NAPI nor the irq disable depth.
>

Taking rtnl_lock/unlock in suspend/resume could close this gap
but it is again probably something very unlikely to happen.

Arthur