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

[Arthur Crepin Leblond]

On Thu, Sep 17, 2026 at 08:08:52PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [Severity: Medium]
> Should the message also mention the teardown changes that apply to all chip
> variants, not just w5500 with a link interrupt?

I can reword a bit the commit message to mention this.

> [Severity: Medium]
> Can a single failed PHYCFGR read leave the interface stuck with the carrier
> off while the PHY is linked?
>
> The negative case here intentionally leaves the carrier untouched, and there
> is no retry, delayed re-read or polling fallback.  The previous value at the
> two initial synchronisation points is off: w5100_probe() calls
> netif_carrier_off() before register_netdev(), and w5100_suspend() forces the
> carrier off.
>
> The errors do reach here.  w5500_spi_read() in w5100-spi.c propagates SPI
> failures:
>
> 	ret = spi_write_then_read(spi, cmd, sizeof(cmd), &data, 1);
>
> 	return ret ? ret : data;
>
> so for example an -ENOMEM from the fallback allocation in
> spi_write_then_read() ends up as a negative return of
> w5500_get_phycfgr_lnk().
>
> The only other refresh comes from LINKLED edges, requested with
> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.  If the link is already up and
> stays up, the pin holds a constant level and no further edge is produced.
>
> So if the read in w5100_open() or w5100_resume() fails once, does the carrier
> stay off until the interface is manually taken down and brought back up,
> with linkwatch keeping the qdisc deactivated in the meantime?  Would
> re-arming a delayed PHYCFGR read on read failure address this?

Yes I agree, an initial SPI read failure will leave the carrier in
such state but adding some polling mechanism in this patch will
complicate it even more.

> [Severity: Medium]
> Making netif_carrier_off() conditional on priv->link_irq here (and the same
> change in w5100_suspend()) looks like it also fixes an existing carrier
> regression.  Should this carry a Fixes: tag for dacf281771a9 ("w5100: remove
> unused gpio link detection")?
>
> dacf281771a9 removed the only netif_carrier_on() calls, which lived in
> w5100_open() and w5100_resume(), but left the unconditional
> netif_carrier_off() in w5100_stop() and w5100_suspend().  On the baseline,
> w5100_open() has no carrier call at all, so:
>
> 	ip link set dev eth0 down
> 	ip link set dev eth0 up
>
> leaves every w5100/w5200/w5500 reporting no carrier, and linkwatch keeps the
> qdisc deactivated.  A suspend/resume cycle has the same effect.
>
> Without a Fixes: tag this fix will not be picked up for stable, so released
> kernels keep the unusable-after-ifdown/ifup behaviour.

This patch is not really a fix, it brings back proper support for the
link detection using an interrupt and reading the device register.

> [Severity: High]
> Can rx_work still run after unregister_netdev() here and hand an skb to
> netif_rx() with a dangling ndev?
>
> w5100_stop(), which runs from dev_close() inside unregister_netdev(), does
> not cancel or flush rx_work:
>
> 	cancel_work_sync(&priv->restart_work);
> 	...
> 	w5100_hw_close(priv);
> 	netif_stop_queue(ndev);
> 	napi_disable(&priv->napi);
>
> so an rx_work item queued just before the close can run afterwards:
>
> w5100_rx_work()
> 	while ((skb = w5100_rx_skb(priv->ndev)))
> 		netif_rx(skb);
>
> 	w5100_enable_intr(priv);
>
> netif_rx() takes no device reference, and unregister_netdev() has already
> done flush_all_backlogs() and netdev_run_todo().  If that skb sits in a
> deferred or RPS-remote backlog, is it dereferenced after the free_netdev() a
> few lines below?
>
> The main interrupt is also still registered while the cancel_work_sync()
> calls run:
>
> w5100_interrupt()
> 	if (ir & S0_IR_RECV) {
> 		w5100_disable_intr(priv);
>
> 		if (priv->ops->may_sleep)
> 			queue_work(priv->xfer_wq, &priv->rx_work);
>
> so an in-flight handler can re-queue rx_work after the cancellation.  That
> late work then touches chip registers concurrently with the w5100_hw_reset()
> above and calls w5100_enable_intr() after the handler has been freed.
>
> Before this patch, destroy_workqueue() drained rx_work while the netdev was
> still registered and after free_irq(), so neither case could happen.  Would
> freeing and synchronizing both interrupts first, then draining all work they
> can produce, and only then resetting the hardware and
> unregistering/freeing the netdev, be the right order?

Right, I'll fix that too. Since unregister is calling ndo_close() I can
handle this there.

> [Severity: Medium]
> Does the new cancel_work_sync(&priv->tx_work) leak priv->tx_skb?
>
> w5100_start_tx() takes ownership of the skb and defers the free entirely to
> the work item:
>
> 	if (priv->ops->may_sleep) {
> 		WARN_ON(priv->tx_skb);
> 		priv->tx_skb = skb;
> 		queue_work(priv->xfer_wq, &priv->tx_work);
>
> and w5100_tx_work() is the only consumer that clears priv->tx_skb and frees
> it via w5100_tx_skb() -> dev_kfree_skb():
>
> 	struct sk_buff *skb = priv->tx_skb;
>
> 	priv->tx_skb = NULL;
>
> 	if (WARN_ON(!skb))
> 		return;
> 	w5100_tx_skb(priv->ndev, skb);
>
> If cancel_work_sync() cancels a work item that has not started yet, nothing
> releases priv->tx_skb, and remove() continues to destroy_workqueue() and
> free_netdev() with it still set.  The previous code reached the same state
> through destroy_workqueue(), which drains queued work instead of cancelling
> it, so the skb was always freed.  Would freeing priv->tx_skb explicitly
> after the cancellation, or flushing tx_work instead, be preferable?

Yes, didn't think of this one.

> [Severity: Medium]
> Should the cancel_work_sync() come before netif_carrier_off() and
> netif_device_detach() rather than after them, as it does in w5100_stop()?
>
> restart_work can be queued at any time on w5500/SPI:
>
> w5100_tx_timeout()
> 	if (priv->ops->may_sleep)
> 		schedule_work(&priv->restart_work);
>
> A work item that is pending or already running when suspend starts still
> passes the new guard in w5100_restart(), because netif_running() stays true
> through the suspend callback and the guard does not test
> netif_device_present():
>
> 	if (!netif_running(ndev))
> 		return;
>
> It then re-runs w5100_hw_reset()/w5100_hw_start(), re-enabling chip
> interrupts and re-opening socket 0, calls netif_wake_queue() which clears the
> __QUEUE_STATE_DRV_XOFF that netif_device_detach() just set, and calls
> w5500_report_carrier_state() which can turn the carrier back on right after
> suspend cleared it.
>
> With the tx queue runnable again and __dev_queue_xmit() not testing
> netif_device_present(), can the stack then reach w5100_start_tx() and issue
> SPI transfers to a chip whose socket w5100_hw_close() has closed, or to an
> already suspended SPI controller?  Would moving the cancellation ahead of the
> state changes, and/or widening the guard in w5100_restart(), close this?

Good point, I'll harden the checks.

Arthur