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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch quietly fixes a user-visible regression that commit
  dacf281771a9 ("w5100: remove unused gpio link detection") introduced.
- [Medium] In w5100_stop(), netif_stop_queue() runs before
  cancel_work_sync(&priv->restart_work).
- [Medium] w5100_open() now calls w5100_hw_reset() but ignores its return
  value.
- [Medium] w5100_suspend() now calls w5100_stop() (disable_irq(irq),
  disable_irq(link_irq), napi_disable()) but leaves __LINK_STATE_START…
- [Low] When all three PHYCFGR reads in w5500_get_phycfgr_lnk() fail,
  w5500_report_carrier_state() leaves carrier unchanged, and…

Pre-existing issues:
- [Low] Claim: making w5100_tx_timeout() always schedule restart_work
  removes the xmit exclusion that dev_watchdog() (tx_global_lock plus…

--

w5100: detect carrier state using link status bit and optional interrupt

This commit reads the w5500 PHYCFGR link status bit to detect carrier. It
adds an optional second interrupt, wired to the LINKLED pin, that triggers
carrier updates. It also reworks bring-up and teardown: register_netdev()
moves to the end of probe, the main irq is only enabled in open, and
suspend/resume now call w5100_stop()/w5100_open().

> On w5100/w5200, which lack this register, get_link() is not implemented
> and the carrier state is not reported (netif_carrier_(on|off) is not
> called).

[Severity: Medium]
Does this patch also fix a regression introduced by commit dacf281771a9
("w5100: remove unused gpio link detection")?

That commit removed the only netif_carrier_on() calls in the driver, in
w5100_open() and w5100_resume(). It kept netif_carrier_off() in
w5100_stop() and w5100_suspend(). At the baseline there is no
netif_carrier_on() anywhere in drivers/net/ethernet/wiznet/.

As a result, on every chip the first ifdown or suspend leaves
__LINK_STATE_NOCARRIER set. The interface then stays NO-CARRIER after the
next ifup or resume.

This patch makes the netif_carrier_off() calls in stop and suspend
conditional on link_irq > 0, which fixes that. However, the commit message
reads as though nothing changes for devices without a link irq, and there
is no Fixes: tag.

Could the commit message describe this as a fix and add:

Fixes: dacf281771a9 ("w5100: remove unused gpio link detection")

That way stable kernels that carry dacf281771a9 would pick it up.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd..dff39ba8103c 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c

[ ... ]

> @@ -345,6 +352,82 @@ static void w5500_memory_configure(struct w5100_priv *priv)

[ ... ]

> +static void w5500_report_carrier_state(struct net_device *ndev)
> +{
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +	int state;
> +
> +	mutex_lock(&priv->link_lock);
> +
> +	state = w5500_get_phycfgr_lnk(ndev);
> +	if (state > 0) {
> +		netif_info(priv, link, ndev, "link is up\n");
> +		netif_carrier_on(ndev);
> +	} else if (state == 0) {
> +		netif_info(priv, link, ndev, "link is down\n");
> +		netif_carrier_off(ndev);
> +	}
> +
> +	mutex_unlock(&priv->link_lock);
> +}
> +
> +static irqreturn_t w5500_detect_link_interrupt(int irq, void *ndev_instance)
> +{
> +	struct net_device *ndev = ndev_instance;
> +
> +	if (netif_running(ndev))
> +		w5500_report_carrier_state(ndev);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Low]
What happens if all three PHYCFGR reads in w5500_get_phycfgr_lnk() fail on
the link-up edge?

In that case the carrier stays off, and w5500_detect_link_interrupt()
returns IRQ_HANDLED without scheduling a retry. A stable link produces no
further LINKLED edge, so the carrier can stay down even after SPI recovers.

w5500_get_link() might then report the link as up through ethtool, but it
doesn't update the carrier. dev_watchdog() also won't start the restart
path while the carrier is off. The only ways to recover seem to be a cable
replug or ifdown/ifup.

These errors can really happen: w5500_spi_read() passes on errors from
spi_write_then_read(), including -ENOMEM from its bounce buffer allocation.

Should a failed read schedule a later re-read of the link state?

[ ... ]

> @@ -448,12 +531,28 @@ static void w5100_restart(struct net_device *ndev)
>  {
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
> +	if (!netif_running(ndev) || !netif_device_present(ndev))
> +		return;
> +
> +	disable_irq(priv->irq);
> +	if (priv->link_irq > 0)
> +		disable_irq(priv->link_irq);
> +
>  	netif_stop_queue(ndev);
> +	w5100_hw_close(priv);
> +	flush_work(&priv->rx_work);
> +	flush_work(&priv->tx_work);
>  	w5100_hw_reset(priv);

[Severity: Low]
This isn't a bug introduced by this patch, but netif_stop_queue() doesn't
wait for an ndo_start_xmit that is already running. Can w5100_start_tx()
still queue tx_work after flush_work(&priv->tx_work) here?

w5100_start_tx() {
	...
	if (priv->ops->may_sleep) {
		WARN_ON(priv->tx_skb);
		priv->tx_skb = skb;
		queue_work(priv->xfer_wq, &priv->tx_work);
	...
}

If it can, tx_work could issue SPI transfers at the same time as
w5100_hw_reset() during tx timeout recovery. The baseline w5100_restart()
had no flush at all, so this patch narrows the window rather than opening
it.

Always scheduling restart_work in w5100_tx_timeout() doesn't change
behaviour for in-tree users. Every w5100_ops in w5100-spi.c already sets
may_sleep = true.

[ ... ]

> @@ -656,9 +753,17 @@ static int w5100_open(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifup, ndev, "enabling\n");
> +	w5100_hw_reset(priv);
> +	enable_irq(priv->irq);
>  	w5100_hw_start(priv);

[Severity: Medium]
Should the return value of w5100_hw_reset() be checked here?

w5100_hw_reset() returns -ENODEV when the RTR read-back doesn't match. This
is its only check that the chip came out of reset and is responding:

	if (w5100_read16(priv, rtr) != RTR_DEFAULT)
		return -ENODEV;

Even if the reset fails, w5100_open() does all of the following and still
returns 0:

- enables the level-low main irq
- calls w5100_hw_start(), which opens the socket and unmasks its interrupt
- enables the link irq and reports carrier from PHYCFGR

w5100_resume() now calls w5100_open() and also ignores its result. So a
failed reset after resume isn't reported either, and netif_device_attach()
still marks the interface as present.

[ ... ]

> @@ -667,13 +772,40 @@ static int w5100_stop(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifdown, ndev, "shutting down\n");
> -	w5100_hw_close(priv);
> -	netif_carrier_off(ndev);
> +
> +	disable_irq(priv->irq);
> +	if (priv->link_irq > 0)
> +		disable_irq(priv->link_irq);
> +
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
> +
> +	cancel_work_sync(&priv->restart_work);

[Severity: Medium]
Can a restart_work that is already running wake the queue again after the
netif_stop_queue() above?

w5100_restart() checks netif_running() and netif_device_present() only on
entry, and it always ends with netif_wake_queue(). w5100_tx_timeout()
schedules restart_work on system_percpu_wq, which isn't freezable:

w5100_suspend()
  netif_device_detach()
  w5100_stop()
    netif_stop_queue()
    cancel_work_sync(&priv->restart_work)   <- waits

w5100_restart() (already past its entry check)
  ...
  netif_wake_queue()                        <- clears XOFF

After suspend returns, the TX queue is awake on a detached device.

The suspend path doesn't deactivate the qdisc, and w5100_start_tx() doesn't
check netif_device_present(). A later transmit can therefore queue tx_work
on xfer_wq. xfer_wq isn't WQ_FREEZABLE, so this would issue SPI transfers
after the suspend callback has completed.

The commit message says w5100_stop() makes sure "there is no race condition
or unfinished work". Should the queue be stopped again after
cancel_work_sync() returns?

[ ... ]

> @@ -839,10 +1011,11 @@ static int w5100_suspend(struct device *dev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	if (netif_running(ndev)) {
> -		netif_carrier_off(ndev);
> +		mutex_lock(&priv->link_lock);
>  		netif_device_detach(ndev);
> +		mutex_unlock(&priv->link_lock);
>  
> -		w5100_hw_close(priv);
> +		w5100_stop(ndev);
>  	}
>  	return 0;
>  }

[Severity: Medium]
What happens if the core calls ndo_stop between this suspend and the
matching resume?

Here w5100_stop() disables both irqs and calls napi_disable(). However,
__LINK_STATE_START stays set, and suspend takes neither rtnl nor the netdev
lock.

A kernel-initiated dev_close() can run during the suspend window, for
example from cleanup_net() on the non-freezable netns workqueue. In that
case w5100_stop() runs a second time:

__dev_close_many()
  w5100_stop()
    disable_irq(priv->irq)          <- disable depth now 2
    napi_disable(&priv->napi)
      napi_disable_locked()         <- SCHED/NPSVC still set from the
                                       first napi_disable()

napi_disable_locked() would then spin forever in usleep_range() while
holding rtnl.

After that, netif_running() is false, so w5100_resume() skips w5100_open().
The irq disable depth stays raised, and a later ifup leaves both irqs
disabled.

Before this patch, suspend only called w5100_hw_close(), which touched
neither NAPI nor the irq disable depth.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-wiznet-link-gpio-v11-0-42a332517b30%40marmottus.net