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

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

On Thu, Sep 17, 2026 at 08:08:52PM +0000, netdev-bot+sashiko@kernel.org 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