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

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

On Thu, Sep 24, 2026 at 01:55:55AM +0000, netdev-bot+sashiko@kernel.org 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