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

[Arthur Crepin Leblond]

On Mon, Sep 28, 2026 at 10:23:18AM +0000, netdev-bot+sashiko@xxxxxxxxxx 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