Re: [PATCH net-next v10 3/3] w5100: detect carrier state using link status bit and optional interrupt
From: Arthur Crepin Leblond
Date: Thu Sep 24 2026 - 04:32:41 EST
On Thu, Sep 24, 2026 at 01:55:55AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
[Severity: Medium]
What happens to the carrier state when the PHYCFGR read fails here?
w5500_get_phycfgr_lnk() returns a negative errno on SPI failure, but
w5500_report_carrier_state() only acts on state > 0 and state == 0, and it
returns void, so the error is neither acted on nor propagated.
The carrier is forced off beforehand (netif_carrier_off() in w5100_probe()
for the link_irq case, and in w5100_stop()/w5100_suspend()), and
w5100_open() samples the state exactly once:
if (priv->link_irq > 0) {
w5500_report_carrier_state(ndev);
enable_irq(priv->link_irq);
}
return 0;
Since w5100_open() still returns 0 and the link IRQ is edge triggered
(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING), no further event arrives while
the link stays up. Can a single failed read at open (or resume, or restart)
therefore leave the netdev permanently without carrier, with dev_activate()
refusing to leave noop_qdisc and dev_watchdog() skipping the device, until
the cable is toggled or the interface is cycled? Meanwhile w5500_get_link()
re-reads the register and keeps reporting "Link detected: yes".
I'll add a retry loop.
[Severity: Low]
This isn't a bug as such, since the commit message documents the intent, but
for a w5500 board whose DT has no second (LINKLED) interrupt the two link
uAPIs end up permanently disagreeing.
w5100_probe() installs the new ops for every w5500:
ndev->ethtool_ops = ops->chip_id == W5500 ? &w5500_ethtool_ops :
&w5100_ethtool_ops;
so ethtool reports the real PHYCFGR bit, while every netif_carrier_on|off()
call in this patch (including the netif_carrier_off() in probe) sits inside
an if (priv->link_irq > 0) block.
With the carrier bit never touched, netif_carrier_ok() stays true (nothing
in net/core sets __LINK_STATE_NOCARRIER at registration). Would it be
better to only publish .get_link when the driver can also maintain the
carrier, so that ip link and ETHTOOL_GLINK cannot report IFF_LOWER_UP and
"Link detected: no" at the same time?
No but that's a fair point, .get_link() should probably be
implemented only on w5500 with a link irq.
[ ... ]
@@ -656,9 +744,16 @@ static int w5100_open(struct net_device *ndev)
struct w5100_priv *priv = netdev_priv(ndev);
netif_info(priv, ifup, ndev, "enabling\n");
- w5100_hw_start(priv);
napi_enable(&priv->napi);
+ enable_irq(priv->irq);
+ w5100_hw_start(priv);
netif_start_queue(ndev);
+
+ if (priv->link_irq > 0) {
+ w5500_report_carrier_state(ndev);
+ enable_irq(priv->link_irq);
+ }
+
return 0;
}
[Severity: Medium]
Can a link transition be missed here because PHYCFGR is sampled before the
edge triggered IRQ is enabled?
The same read-then-enable order is used in w5100_restart() and
w5100_resume(). The IRQ is requested with
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
IRQF_NO_AUTOEN
so at open the enable_irq() is the initial irq_startup(): the flow handler
has never run, no IRQS_PENDING state exists in the irq_desc for the core to
resend, and replay of an edge that happened while the line was masked
depends on the irqchip latching it (GPIO controllers such as bcm2835 clear
the per-line edge enable bits on mask, so nothing is recorded).
A transition inside the window between the read and enable_irq() would then
be seen by neither, leaving the carrier stale until the next physical
change. Would enabling the IRQ first and reading PHYCFGR afterwards be
safe, given that w5500_report_carrier_state() is idempotent and serialized
by priv->link_lock? The v7 changelog entry ("Enable the link IRQ before
reading the PHYCFGR bit") suggests this order was intentional at some
point.
I'll enable the irq before reporting the state.
@@ -667,13 +762,38 @@ static int w5100_stop(struct net_device *ndev)
struct w5100_priv *priv = netdev_priv(ndev);
netif_info(priv, ifdown, ndev, "shutting down\n");
+
+ disable_irq(priv->irq);
+ if (priv->link_irq > 0)
+ disable_irq(priv->link_irq);
+
+ cancel_work_sync(&priv->restart_work);
+ cancel_work_sync(&priv->setrx_work);
+ flush_work(&priv->rx_work);
+ flush_work(&priv->tx_work);
+
w5100_hw_close(priv);
[Severity: High]
Can flush_work(&priv->rx_work) block here indefinitely, since it now runs
before w5100_hw_close() stops the producer of receive traffic?
disable_irq() stops interrupt handling, but socket 0 is still open and the
chip keeps filling its 16 KB RX buffer. The worker drains without a budget
or a shutdown check:
drivers/net/ethernet/wiznet/w5100.c:w5100_rx_work() {
while ((skb = w5100_rx_skb(priv->ndev)))
netif_rx(skb);
w5100_enable_intr(priv);
}
and w5100_rx_skb() only returns NULL once the hardware buffer reads empty:
u16 rx_buf_len = w5100_read16(priv, W5100_S0_RX_RSR(priv));
if (rx_buf_len == 0)
return NULL;
The SPI drain rate is well below the wire rate, so an adjacent sender
flooding the link can keep RSR non-zero for as long as it likes.
ndo_stop runs under RTNL, so would "ip link set dev down" (and
unregister_netdev() from w5100_remove(), which now runs first) stall with
RTNL held, blocking all other network configuration? Closing the hardware
before flushing rx_work, as the previous code did, would avoid this.
Yeah, I've had this suggestion when running Sashiko locally too. This
patch is a Pandora box or race conditions! But hopefully I figured it
out and it's fixed in v11.
I am really learning a lot along the way regarding the synchronization
of ndos and how the calls are made.
- netif_carrier_off(ndev);
+
+ if (priv->link_irq > 0) {
+ mutex_lock(&priv->link_lock);
+ netif_carrier_off(ndev);
+ mutex_unlock(&priv->link_lock);
+ }
+
[Severity: Medium]
Making this netif_carrier_off() conditional, together with the new carrier
sampling in w5100_open()/w5100_resume(), looks like it also repairs an
existing user visible breakage. Should that be split out with a Fixes: tag?
In the baseline tree, dacf281771a9 ("w5100: remove unused gpio link
detection") removed w5100_get_link()/w5100_detect_link() and both
netif_carrier_on() call sites, but kept the unconditional
netif_carrier_off() in w5100_stop() and w5100_suspend(). git grep
netif_carrier_on on the baseline returns no match in this driver.
A freshly registered netdev has __LINK_STATE_NOCARRIER clear, so the first
ifup works, but after the first ifdown (or suspend cycle) the bit is set and
nothing ever clears it again, and dev_activate() takes the "Delay activation
until next carrier-on event" path, so the interface cannot transmit until
the module is reloaded.
As written, the fix is buried in a feature patch that also rewrites probe
ordering, IRQ enable/disable and work cancellation, which makes it hard to
pick up for stable.
It still don't think it is a fix. The patch is bringing back the
interrupt and improves it but it is too much of a change to be called
a fix.
[Severity: Medium]
The commit message says:
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 drains restart_work. w5100_stop() and
w5100_remove() drain all four (restart_work, setrx_work, rx_work, tx_work).
Could the changelog be adjusted, or the suspend path made to match?
[Severity: High]
Does leaving setrx_work, rx_work and tx_work pending here allow the chip to
be re-armed after w5100_hw_close()?
disable_irq()/synchronize_irq() waits for the threaded handler only, not for
work already queued on priv->xfer_wq:
w5100_interrupt() -> queue_work(priv->xfer_wq, &priv->rx_work)
w5100_start_tx() -> queue_work(priv->xfer_wq, &priv->tx_work)
w5100_set_rx_mode() -> schedule_work(&priv->setrx_work)
A pending w5100_rx_work() ends with w5100_enable_intr(), re-arming
W5500_SIMR / W5100_IMR; w5100_setrx_work() calls w5100_hw_start(), which
issues S0_CR_OPEN and enables interrupts again; w5100_tx_work() can program
S0_TX_WR and issue S0_CR_SEND.
Any of these can run concurrently with, or after, the w5100_hw_close() a few
lines below, so the controller can stay open with socket interrupts armed
for the whole suspend while the CPU IRQ is disabled, and the SPI transfers
are issued after the device was marked absent. All in-tree ops set
may_sleep = true, so this is the live path. Would replicating the
w5100_stop() sequence here be appropriate?
[Severity: Medium]
Can restart_work be re-queued after this cancel_work_sync(), because the
cancel happens before netif_device_detach()?
In that window the device is still running, still present, and (for the
link IRQ case) still carrier-ok, and the tx watchdog timer is not lowered on
the suspend path. On the dev_close() path, dev_deactivate_many() calls
netdev_watchdog_down() before ndo_stop, which is what makes the same
ordering safe in w5100_stop().
dev_watchdog() only requires:
net/sched/sch_generic.c:dev_watchdog() {
if (netif_device_present(dev) &&
netif_running(dev) &&
netif_carrier_ok(dev)) {
...
}
and w5100_tx_timeout() is now unconditional:
schedule_work(&priv->restart_work);
The re-queued w5100_restart() would then pass its
if (!netif_running(ndev) || !netif_device_present(ndev))
return;
test, since nothing serializes that check with netif_device_detach(), and
proceed to w5100_hw_reset(), enable_irq(priv->irq), w5100_hw_start() and the
PHYCFGR read, concurrently with the w5100_hw_close() below. Would detaching
first and cancelling afterwards close this?
I reworked all those in the upcoming v11. I really tried to figure out
all the edge cases.
Arthur