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

[Arthur Crepin Leblond]

On Tue, Sep 22, 2026 at 03:43:26PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [Severity: Medium]
> ...
> What is the intended split here - should w5100_tx_timeout() always queue
> restart_work now that w5100_restart() can sleep, or should the irq
> masking and the carrier re-check be done only in the may_sleep path?

schedule_work() will always be called (see v10).

> [Severity: Medium]
> The link irq is requested below with IRQF_NO_AUTOEN and
> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, so it stays masked until this
> enable_irq(). PHYCFGR is sampled first and edge detection is only turned on
> afterwards:
>
> 	w5500_report_carrier_state(ndev);	/* reads PHYCFGR */
> 	enable_irq(priv->link_irq);		/* edge detection starts */
>
> If the PHY link comes up between those two steps, can that transition be
> lost for good? On the BCM2835 controller used in the binding's own example,
> masking disables edge detection and also discards latched events:
>
> drivers/pinctrl/bcm/pinctrl-bcm2835.c:
> 	bcm2835_gpio_irq_config(pc, gpio, false);
> 	/* Clear events that were latched prior to clearing event sources */
> 	bcm2835_gpio_set_bit(pc, GPEDS0, gpio);
>
> so unmasking does not replay the missed edge, and with a now stable LINKLED
> level the driver keeps reporting carrier off until the next physical link
> change.
>
> w5100_restart() and w5100_resume() use the same order, and restart follows a
> chip reset with PHY re-negotiation. The changelog lists this ordering as
> intentional ("enable the link irq after checking PHYCFGR") - would enabling
> the irq first and then reading PHYCFGR close the window?

Probably very unlikely to happen and more of an edge case but I can
enable link irq first.

> [Severity: High]
> flush_work(&priv->rx_work) now runs while socket 0 is still open, because
> w5100_hw_close() is called after it.
>
> w5100_rx_work() drains without a budget or a shutdown test:
>
> 	while ((skb = w5100_rx_skb(priv->ndev)))
> 		netif_rx(skb);
>
> and w5100_rx_skb() only returns NULL once the hardware receive buffer is
> observed empty:
>
> 	u16 rx_buf_len = w5100_read16(priv, W5100_S0_RX_RSR(priv));
>
> 	if (rx_buf_len == 0)
> 		return NULL;
>
> disable_irq(priv->irq) masks the host interrupt but does not stop the
> controller from filling its receive buffer. Each frame is drained with
> several synchronous SPI transfers, which is slower than line rate, so can
> sustained incoming traffic keep RX_RSR non-zero and make this flush_work()
> never return?
>
> ndo_stop runs with rtnl held, so an ip link set down would then stall all
> network configuration in an unkillable wait. w5100_remove() reaches the same
> path through unregister_netdev().
>
> Would calling w5100_hw_close() before flushing rx_work avoid this?
>
> > -	netif_carrier_off(ndev);
> > +
> > +	if (priv->link_irq > 0)  {
> > +		mutex_lock(&priv->link_lock);
> > +		netif_carrier_off(ndev);
> > +		mutex_unlock(&priv->link_lock);
> > +	}
> > +
> >  	netif_stop_queue(ndev);
> >  	napi_disable(&priv->napi);
> >  	return 0;
> >  }

Agreed, w5100_hw_close() should be called earlier.

> [Severity: Medium]
> The changelog states:
>
>  w5100_remove(), w5100_stop() and w5100_suspend() call
>  cancel_work_sync()/flush_work() to make sure there is no pending
>  work
>
> but w5100_suspend() only cancels restart_work here. setrx_work (queued with
> schedule_work() from w5100_set_rx_mode()), rx_work (queued on priv->xfer_wq
> from w5100_interrupt()) and tx_work (queued from w5100_start_tx()) are
> neither cancelled nor flushed, while w5100_stop() drains all four. Should
> the changelog be narrowed, or the suspend path made to match stop()?
>
> [Severity: High]
> Independently of the changelog wording, can these work items still touch the
> hardware after suspend has closed it?
>
> disable_irq(priv->irq) waits for the threaded handler w5100_interrupt() to
> finish, but any rx_work it already queued is still pending, and
> priv->xfer_wq is created with WQ_MEM_RECLAIM | WQ_PERCPU (no WQ_FREEZABLE),
> so it keeps running during suspend. w5100_rx_work() does SPI register
> accesses (S0_RX_RD writes and S0_CR_RECV) and ends with:
>
> 	w5100_enable_intr(priv);
>
> which re-arms the chip interrupt masks that w5100_hw_close() cleared, while
> the host irq stays masked.
>
> setrx_work runs on system_wq and calls w5100_hw_start(), re-issuing
> S0_CR_OPEN and w5100_enable_intr() after suspend closed socket 0. tx_work
> can still issue S0_CR_SEND. Any of these transfers issued after the suspend
> callback returns are rejected once the SPI controller itself is suspended.
>
> There is also a re-queue window: restart_work is cancelled here, before
> netif_device_detach() below. In between, dev_watchdog() can still fire
> (netif_device_present(), netif_running() and netif_carrier_ok() all hold,
> and carrier_ok() is permanently true on w5100/w5200 and on a W5500 without a
> link irq):
>
> w5100_tx_timeout()
> 	if (priv->ops->may_sleep)
> 		schedule_work(&priv->restart_work);
>
> The re-queued w5100_restart() then passes its own netif_running() /
> netif_device_present() test and runs w5100_hw_reset() plus w5100_hw_start()
> against the device suspend is closing.
>
> Should netif_device_detach() come before the cancel, and should setrx_work,
> rx_work and tx_work be drained here as they are in w5100_stop()?

suspend() and resume() will call w5100_stop() and w5100_open()
directly.
And yes, netif_device_detach() will be called before.

Arthur