Re: [PATCH v2 1/2] w5100: restore GPIO-based link detection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The devm-managed link IRQ outlives the manually freed network device, leading to a Use-After-Free in the interrupt handler during driver removal.
- [Medium] The threaded interrupt handler uses the non-sleepable `gpiod_get_value()` instead of `gpiod_get_value_cansleep()`, which will trigger a kernel WARNING if the GPIO chip can sleep.
- [Medium] Race condition between the initial link state check in `w5100_open` and the edge-triggered link IRQ, leading to a desynchronized carrier state.
--

--- Patch [1]: [PATCH v2 1/2] w5100: restore GPIO-based link detection ---
commit 48dd7a12d64614af31aae8617e2d9b17300b7e8a
Author: Arthur Crépin Leblond <arthur@marmottus.net>

w5100: restore GPIO-based link detection

Commit dacf281771a9 ("w5100: remove unused gpio link detection")
dropped the link_gpio/link_irq handling on the grounds that no
devicetree user passed a "link" GPIO at the time.

This isn't a plain revert of that removal. The link GPIO is now
bound from the device tree link-gpios property using the resource
managed GPIO descriptors.

The link IRQ is also requested with devm_request_threaded_irq()
instead of request_any_context_irq(), so it is released automatically
on driver removal or probe failure.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..1410a9fcfa921 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c
[ ... ]
> @@ -616,6 +628,24 @@ static irqreturn_t w5100_interrupt(int irq, void *ndev_instance)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t w5100_detect_link(int irq, void *ndev_instance)
> +{
> +	struct net_device *ndev = ndev_instance;
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +
> +	if (netif_running(ndev)) {
> +		if (gpiod_get_value(priv->link_gpio) != 0) {

[Severity: Medium]
Will this trigger a warning if the GPIO controller can sleep?

Since w5100_detect_link() is registered as a threaded IRQ handler, it is
allowed to sleep. If the underlying GPIO controller is sleepable (like an I2C
or SPI expander), using the non-sleepable gpiod_get_value() will trigger a
WARN_ON. Should this use gpiod_get_value_cansleep() instead?

> +			netif_info(priv, link, ndev, "link is up\n");
> +			netif_carrier_on(ndev);
> +		} else {
> +			netif_info(priv, link, ndev, "link is down\n");
> +			netif_carrier_off(ndev);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
[ ... ]
> @@ -659,6 +689,14 @@ static int w5100_open(struct net_device *ndev)
>  	w5100_hw_start(priv);
>  	napi_enable(&priv->napi);
>  	netif_start_queue(ndev);
> +
> +	if (priv->link_gpio) {
> +		if (gpiod_get_value_cansleep(priv->link_gpio) != 0)
> +			netif_carrier_on(ndev);
> +		else
> +			netif_carrier_off(ndev);
> +	}
> +

[Severity: Medium]
Can this race with the w5100_detect_link() IRQ handler?

If a physical link change triggers an interrupt right after the GPIO value
is read but before netif_carrier_on/off is called, the IRQ handler will run
and correctly update the carrier state.

Once the IRQ handler completes, w5100_open() will continue and overwrite the
carrier state with the stale value it read earlier. Because the link IRQ is
edge-triggered, no further interrupts will correct this desynchronization.

>  	return 0;
>  }
[ ... ]
> @@ -803,8 +849,29 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  	if (err)
>  		goto err_hw;
>  
> +	if (priv->link_gpio) {
> +		int link_irq = gpiod_to_irq(priv->link_gpio);
> +
> +		if (link_irq < 0) {
> +			err = dev_err_probe(dev, link_irq,
> +					    "No corresponding irq for link gpio\n");
> +			goto err_gpio;
> +		}
> +
> +		err = devm_request_threaded_irq(dev, link_irq, NULL,
> +						w5100_detect_link,
> +						IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"w5100-link", priv->ndev);

[Severity: High]
Does this introduce a Use-After-Free during driver removal?

Since devm_request_threaded_irq() is used, the IRQ is automatically released
by devres after the driver's remove function returns.

However, w5100_remove() manually frees the network device:

drivers/net/ethernet/wiznet/w5100.c:w5100_remove() {
    ...
	unregister_netdev(ndev);
	free_netdev(ndev);
}

If a link interrupt fires after free_netdev() but before devres cleans up
the IRQ, w5100_detect_link() will be called and dereference the freed ndev
pointer.

> +		if (err < 0)
> +			goto err_gpio;
> +	}
> +
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-wiznet-link-gpio-v2-0-3b1d0c870f35@marmottus.net?part=1