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

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

On Tue, Sep 22, 2026 at 03:43:26PM +0000, netdev-bot+sashiko@kernel.org wrote:
>[Severity: Medium]
> ...
>What is the intended split here - should w5100_tx_timeout() always queue
>restart_work now that w5100_restart() can sleep, or should the irq
>masking and the carrier re-check be done only in the may_sleep path?

schedule_work() will always be called (see v10).

>
>[Severity: Medium]
>The link irq is requested below with IRQF_NO_AUTOEN and
>IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, so it stays masked until this
>enable_irq(). PHYCFGR is sampled first and edge detection is only turned on
>afterwards:
>
>	w5500_report_carrier_state(ndev);	/* reads PHYCFGR */
>	enable_irq(priv->link_irq);		/* edge detection starts */
>
>If the PHY link comes up between those two steps, can that transition be
>lost for good? On the BCM2835 controller used in the binding's own example,
>masking disables edge detection and also discards latched events:
>
>drivers/pinctrl/bcm/pinctrl-bcm2835.c:
>	bcm2835_gpio_irq_config(pc, gpio, false);
>	/* Clear events that were latched prior to clearing event sources */
>	bcm2835_gpio_set_bit(pc, GPEDS0, gpio);
>
>so unmasking does not replay the missed edge, and with a now stable LINKLED
>level the driver keeps reporting carrier off until the next physical link
>change.
>
>w5100_restart() and w5100_resume() use the same order, and restart follows a
>chip reset with PHY re-negotiation. The changelog lists this ordering as
>intentional ("enable the link irq after checking PHYCFGR") - would enabling
>the irq first and then reading PHYCFGR close the window?

Probably very unlikely to happen and more of an edge case but I can
enable link irq first.

>[Severity: High]
>flush_work(&priv->rx_work) now runs while socket 0 is still open, because
>w5100_hw_close() is called after it.
>
>w5100_rx_work() drains without a budget or a shutdown test:
>
>	while ((skb = w5100_rx_skb(priv->ndev)))
>		netif_rx(skb);
>
>and w5100_rx_skb() only returns NULL once the hardware receive buffer is
>observed empty:
>
>	u16 rx_buf_len = w5100_read16(priv, W5100_S0_RX_RSR(priv));
>
>	if (rx_buf_len == 0)
>		return NULL;
>
>disable_irq(priv->irq) masks the host interrupt but does not stop the
>controller from filling its receive buffer. Each frame is drained with
>several synchronous SPI transfers, which is slower than line rate, so can
>sustained incoming traffic keep RX_RSR non-zero and make this flush_work()
>never return?
>
>ndo_stop runs with rtnl held, so an ip link set down would then stall all
>network configuration in an unkillable wait. w5100_remove() reaches the same
>path through unregister_netdev().
>
>Would calling w5100_hw_close() before flushing rx_work avoid this?
>
>> -	netif_carrier_off(ndev);
>> +
>> +	if (priv->link_irq > 0)  {
>> +		mutex_lock(&priv->link_lock);
>> +		netif_carrier_off(ndev);
>> +		mutex_unlock(&priv->link_lock);
>> +	}
>> +
>>  	netif_stop_queue(ndev);
>>  	napi_disable(&priv->napi);
>>  	return 0;
>>  }
>

Agreed, w5100_hw_close() should be called earlier.

>[Severity: Medium]
>The changelog states:
>
>  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 cancels restart_work here. setrx_work (queued with
>schedule_work() from w5100_set_rx_mode()), rx_work (queued on priv->xfer_wq
>from w5100_interrupt()) and tx_work (queued from w5100_start_tx()) are
>neither cancelled nor flushed, while w5100_stop() drains all four. Should
>the changelog be narrowed, or the suspend path made to match stop()?
>
>[Severity: High]
>Independently of the changelog wording, can these work items still touch the
>hardware after suspend has closed it?
>
>disable_irq(priv->irq) waits for the threaded handler w5100_interrupt() to
>finish, but any rx_work it already queued is still pending, and
>priv->xfer_wq is created with WQ_MEM_RECLAIM | WQ_PERCPU (no WQ_FREEZABLE),
>so it keeps running during suspend. w5100_rx_work() does SPI register
>accesses (S0_RX_RD writes and S0_CR_RECV) and ends with:
>
>	w5100_enable_intr(priv);
>
>which re-arms the chip interrupt masks that w5100_hw_close() cleared, while
>the host irq stays masked.
>
>setrx_work runs on system_wq and calls w5100_hw_start(), re-issuing
>S0_CR_OPEN and w5100_enable_intr() after suspend closed socket 0. tx_work
>can still issue S0_CR_SEND. Any of these transfers issued after the suspend
>callback returns are rejected once the SPI controller itself is suspended.
>
>There is also a re-queue window: restart_work is cancelled here, before
>netif_device_detach() below. In between, dev_watchdog() can still fire
>(netif_device_present(), netif_running() and netif_carrier_ok() all hold,
>and carrier_ok() is permanently true on w5100/w5200 and on a W5500 without a
>link irq):
>
>w5100_tx_timeout()
>	if (priv->ops->may_sleep)
>		schedule_work(&priv->restart_work);
>
>The re-queued w5100_restart() then passes its own netif_running() /
>netif_device_present() test and runs w5100_hw_reset() plus w5100_hw_start()
>against the device suspend is closing.
>
>Should netif_device_detach() come before the cancel, and should setrx_work,
>rx_work and tx_work be drained here as they are in w5100_stop()?

suspend() and resume() will call w5100_stop() and w5100_open()
directly.
And yes, netif_device_detach() will be called before.

Arthur