Re: [PATCH net v3] net: stmmac: request the MDIO reset GPIO only once

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 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] New probe-failure path when the DT MDIO bus is disabled.
- [Low] Conditional-compilation mismatch between the new GPIO/delay
  acquisition and its only consumer.

--

net: stmmac: request the MDIO reset GPIO only once

The patch moves the "snps,reset" GPIO lookup and the "snps,reset-delays-us"
parsing out of stmmac_mdio_reset() and into stmmac_mdio_register(), caching
both in struct stmmac_priv. stmmac_mdio_reset() then only pulses the cached
descriptor, so the devm GPIO request happens once per device instead of once
per reset.

A couple of questions about the new probe-time acquisition below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index afe98ff5bdcb..cf768afd9237 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -384,33 +384,16 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  	struct stmmac_priv *priv = netdev_priv(bus->priv);
>  	unsigned int mii_address = priv->hw->mii.addr;
>  
> -#ifdef CONFIG_OF
> -	if (priv->device->of_node) {
> -		struct gpio_desc *reset_gpio;
> -		u32 delays[3] = { 0, 0, 0 };
> +	if (priv->mdio_reset_delays[0])
> +		msleep(DIV_ROUND_UP(priv->mdio_reset_delays[0], 1000));
>  
> -		reset_gpio = devm_gpiod_get_optional(priv->device,
> -						     "snps,reset",
> -						     GPIOD_OUT_LOW);
> -		if (IS_ERR(reset_gpio))
> -			return PTR_ERR(reset_gpio);
> +	gpiod_set_value_cansleep(priv->mdio_reset_gpio, 1);
> +	if (priv->mdio_reset_delays[1])
> +		msleep(DIV_ROUND_UP(priv->mdio_reset_delays[1], 1000));
>  
> -		device_property_read_u32_array(priv->device,
> -					       "snps,reset-delays-us",
> -					       delays, ARRAY_SIZE(delays));
> -
> -		if (delays[0])
> -			msleep(DIV_ROUND_UP(delays[0], 1000));
> -
> -		gpiod_set_value_cansleep(reset_gpio, 1);
> -		if (delays[1])
> -			msleep(DIV_ROUND_UP(delays[1], 1000));
> -
> -		gpiod_set_value_cansleep(reset_gpio, 0);
> -		if (delays[2])
> -			msleep(DIV_ROUND_UP(delays[2], 1000));
> -	}
> -#endif
> +	gpiod_set_value_cansleep(priv->mdio_reset_gpio, 0);
> +	if (priv->mdio_reset_delays[2])
> +		msleep(DIV_ROUND_UP(priv->mdio_reset_delays[2], 1000));
>  
>  	/* This is a workaround for problems with the STE101P PHY.
>  	 * It doesn't complete its reset until at least one clock cycle

[ ... ]

> @@ -608,6 +591,19 @@ int stmmac_mdio_register(struct net_device *ndev)
>  	if (!mdio_bus_data)
>  		return 0;
>  
> +	if (mdio_bus_data->needs_reset && dev_of_node(priv->device)) {
> +		priv->mdio_reset_gpio =
> +			devm_gpiod_get_optional(priv->device, "snps,reset",
> +						GPIOD_OUT_LOW);
> +		if (IS_ERR(priv->mdio_reset_gpio))
> +			return PTR_ERR(priv->mdio_reset_gpio);
> +
> +		device_property_read_u32_array(priv->device,
> +					       "snps,reset-delays-us",
> +					       priv->mdio_reset_delays,
> +					       ARRAY_SIZE(priv->mdio_reset_delays));
> +	}
> +

[Severity: Low]
Should this new block carry the same conditional compilation as its only
consumer?

The whole body of stmmac_mdio_reset(), including every use of
priv->mdio_reset_gpio and priv->mdio_reset_delays[], sits inside:

drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:stmmac_mdio_reset() {
#if IS_ENABLED(CONFIG_STMMAC_PLATFORM)
	...
#endif
	return 0;
}

but the acquisition added here is not guarded that way. Before the patch the
devm_gpiod_get_optional() call lived inside that guard, so in a build with
CONFIG_STMMAC_PLATFORM=n no GPIO was requested at all.

stmmac_mdio.c is part of the core stmmac.o, and DWMAC_LOONGSON is declared
after "endif # STMMAC_PLATFORM" in drivers/net/ethernet/stmicro/stmmac/Kconfig
and only selects STMMAC_LIBPCI, so it can be built with STMMAC_PLATFORM=n.
loongson_dwmac_dt_config() sets needs_reset on a device that has an of_node:

drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c {
	plat->mdio_node = of_get_child_by_name(np, "mdio");
	if (plat->mdio_node) {
		dev_info(&pdev->dev, "Found MDIO subnode\n");
		plat->mdio_bus_data->needs_reset = true;
	}
}

In that configuration the line is claimed and driven low for the device
lifetime while the pulse code does not exist, and an acquisition error now
aborts stmmac_mdio_register() for a line that can never be used. Would it be
better for both sides to share one condition, either by guarding the
acquisition identically or by dropping the guard from the reset body?

[Severity: Medium]
Can this turn a disabled MDIO bus node into a MAC probe failure?

stmmac_mdio_setup() sets needs_reset from the mere presence of an MDIO child
node, and stmmac_of_get_mdio() uses for_each_child_of_node() /
of_get_child_by_name() without checking of_device_is_available(), so a node
with status = "disabled" still reaches this new block.

The acquisition and its "return PTR_ERR()" run before of_mdiobus_register()
gets a chance to report the disabled node, which stmmac_mdio_register()
currently treats as success:

drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:stmmac_mdio_register() {
	err = of_mdiobus_register(new_bus, mdio_node);
	if (err == -ENODEV) {
		err = 0;
		dev_info(dev, "MDIO bus is disabled\n");
		goto bus_register_fail;
	}
}

and the -ENODEV comes from:

drivers/net/mdio/of_mdio.c:__of_mdiobus_register() {
	/* Do not continue if the node is disabled */
	if (!of_device_is_available(np))
		return -ENODEV;
}

Since bus->reset is only called from __mdiobus_register(), the pre-patch code
never requested the GPIO for a disabled MDIO node, so it could not fail there.
With this patch, a MAC node that still carries snps,reset-gpios whose
acquisition returns -EBUSY (another consumer owns the line) or a permanent
-EPROBE_DEFER aborts the MAC probe even though the reset can never run.

Would it make sense to acquire the descriptor only once the bus is known to be
enabled, still before its reset callback can run, or to let the disabled-bus
path tolerate the acquisition error the way it tolerates -ENODEV?

I could not find an in-tree device tree that combines a disabled mdio child
with a MAC-level snps,reset-gpios that fails to be acquired, so this one is
speculative, but nothing in the code appears to prevent the combination.

>  	stmmac_mdio_bus_config(priv);
>  
>  	new_bus = mdiobus_alloc();

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921015727.2643540-1-xiaolinkui%40126.com