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

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

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

stmmac_mdio_reset() calls devm_gpiod_get_optional() every time it runs.
A GPIO line can only be requested once, so from the second call on
gpiod_request_commit() returns -EBUSY. devm_gpiod_get_optional() only
turns -ENOENT into NULL, hence the error is passed straight back and
stmmac_mdio_reset() bails out before pulsing "snps,reset" and before
running the STE101P MDC workaround.

The first call, made by of_mdiobus_register(), succeeds, so the failure
is only visible later on: every resume that does not use WoL goes
through stmmac_resume() -> stmmac_mdio_reset(), and that caller ignores
the return value, so the PHY silently stays un-reset.

The descriptor used to be requested exactly once: stmmac_mdio_reset()
resolved "snps,reset-gpio" itself and cached the GPIO number in
stmmac_mdio_bus_data::reset_gpio, and commit ae26c1c6cb9b ("stmmac: fix
PHY reset during resume") relies on that cache to reuse the line on
every call. commit 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in
stmmac_mdio_reset") replaced it with a devm_gpiod_get_optional() that
caches nothing, so the request is repeated on every call and fails from
the second one on.

Parse the whole reset description, the GPIO and "snps,reset-delays-us",
in stmmac_mdio_register() at probe time, and keep it in struct
stmmac_priv. This is where devm-gpiod is meant to be used: the line is
acquired with the device and released with it, and any failure to
acquire it is reported during probe instead of being ignored by
stmmac_resume(). stmmac_mdio_reset() then only pulses the cached line,
with the delays that were read once and for all at probe time.

The request and the delays are still gated on
mdio_bus_data->needs_reset, which is the condition that installs the
reset callback, and on the device using DT. stmmac_mdio_reset()
itself no longer looks at the device tree at all: where the
description is absent the cached descriptor is NULL and the delays
are all zero, so the pulse is a no-op, just as the of_node check used
to make it.

Fixes: 7c86f20d15b7 ("net: stmmac: use GPIO descriptors in stmmac_mdio_reset")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v3:
- Read "snps,reset-delays-us" in stmmac_mdio_register() as well, so that
  the whole reset description is parsed in one place. (Lorenzo Bianconi)
- Drop the CONFIG_OF/of_node check from stmmac_mdio_reset(), which no
  longer needs it now that nothing is parsed there: the callback does not
  look at the device tree at all.

Changes in v2:
- Do not request the GPIO from stmmac_mdio_reset(); request it once in
  stmmac_mdio_register() at probe time instead. (Maxime Chevallier)

 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  3 ++
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 46 +++++++++----------
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..2bb68e8b6274 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -25,6 +25,7 @@
 #include <net/xdp.h>
 #include <uapi/linux/bpf.h>
 
+struct gpio_desc;
 struct stmmac_pcs;
 
 struct stmmac_resources {
@@ -287,6 +288,8 @@ struct stmmac_priv {
 
 	unsigned int pause_time;
 	struct mii_bus *mii;
+	struct gpio_desc *mdio_reset_gpio;
+	u32 mdio_reset_delays[3];
 
 	struct stmmac_pcs *integrated_pcs;
 
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));
+	}
+
 	stmmac_mdio_bus_config(priv);
 
 	new_bus = mdiobus_alloc();
-- 
2.25.1

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

[netdev-bot+sashiko]

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

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

[Jakub Kicinski]

On Mon, 21 Sep 2026 09:57:27 +0800 Linkui Xiao wrote:
> +struct gpio_desc;
>  struct stmmac_pcs;
>  
>  struct stmmac_resources {
> @@ -287,6 +288,8 @@ struct stmmac_priv {
>  
>  	unsigned int pause_time;
>  	struct mii_bus *mii;
> +	struct gpio_desc *mdio_reset_gpio;
> +	u32 mdio_reset_delays[3];

Someone already marked it as changes requested but also no need to
forward declare structs unless first use is as function arguemnt.