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

[PATCH net] 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 per consumer, 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 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 returns without resetting the
PHY. Boards needing a hard PHY reset after power down then come back
with a dead or badly negotiated link, and nothing reports it because
both callers ignore the return value.

The descriptor used to be cached in stmmac_mdio_bus_data::reset_gpio so
that it was requested exactly once. Commit ae26c1c6cb9b ("stmmac: fix
PHY reset during resume") relies on that cache to reuse the line on
every call. Dropping the cache removed the protection without replacing
it and reintroduced the very bug it had been fixed for.

Cache the descriptor in struct stmmac_priv instead and only look it up
while it is still unknown. It is devm-managed against the platform
device, so it stays valid for the whole lifetime of the driver.

Fixes: 7e770b252a62 ("net: stmmac: drop the reset GPIO from struct stmmac_mdio_bus_data")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  2 ++
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 20 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..986fb43db45f 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,7 @@ struct stmmac_priv {
 
 	unsigned int pause_time;
 	struct mii_bus *mii;
+	struct gpio_desc *mdio_reset_gpio;
 
 	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..5f99d7db39f7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -389,11 +389,21 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 		struct gpio_desc *reset_gpio;
 		u32 delays[3] = { 0, 0, 0 };
 
-		reset_gpio = devm_gpiod_get_optional(priv->device,
-						     "snps,reset",
-						     GPIOD_OUT_LOW);
-		if (IS_ERR(reset_gpio))
-			return PTR_ERR(reset_gpio);
+		/* Request the reset line only once and reuse the descriptor
+		 * afterwards. A second request of the very same line makes
+		 * gpiolib fail with -EBUSY, which devm_gpiod_get_optional()
+		 * passes through because it only filters out -ENOENT. The
+		 * reset would then abort early and leave the PHY un-reset.
+		 */
+		if (!priv->mdio_reset_gpio) {
+			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);
+		}
+		reset_gpio = priv->mdio_reset_gpio;
 
 		device_property_read_u32_array(priv->device,
 					       "snps,reset-delays-us",
-- 
2.25.1

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

[Maxime Chevallier]

Hello,

> @@ -389,11 +389,21 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  		struct gpio_desc *reset_gpio;
>  		u32 delays[3] = { 0, 0, 0 };
>  
> -		reset_gpio = devm_gpiod_get_optional(priv->device,
> -						     "snps,reset",
> -						     GPIOD_OUT_LOW);
> -		if (IS_ERR(reset_gpio))
> -			return PTR_ERR(reset_gpio);
> +		/* Request the reset line only once and reuse the descriptor
> +		 * afterwards. A second request of the very same line makes
> +		 * gpiolib fail with -EBUSY, which devm_gpiod_get_optional()
> +		 * passes through because it only filters out -ENOENT. The
> +		 * reset would then abort early and leave the PHY un-reset.
> +		 */
> +		if (!priv->mdio_reset_gpio) {
> +			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);
> +		}
> +		reset_gpio = priv->mdio_reset_gpio;

The problem is real, but this isn't the correct approach to solve that.

Instead of requesting the reset gpio on-the-fly at reset time (good example
of when not to use devm_xxx) , let's request the gpio at probe time.

I suggest you move the devm_gpiod_get_optional() in stmmac_mdio_register(),
which is called at probe time. Here, it makes sense to use devm_xxx.

Thanks :)

Maxime

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

[Linkui Xiao]

Hi Maxime,

Thanks for the review.

On 2026/9/18 00:10, Maxime Chevallier wrote:
> Hello,
> 
>> @@ -389,11 +389,21 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>>   		struct gpio_desc *reset_gpio;
>>   		u32 delays[3] = { 0, 0, 0 };
>>   
>> -		reset_gpio = devm_gpiod_get_optional(priv->device,
>> -						     "snps,reset",
>> -						     GPIOD_OUT_LOW);
>> -		if (IS_ERR(reset_gpio))
>> -			return PTR_ERR(reset_gpio);
>> +		/* Request the reset line only once and reuse the descriptor
>> +		 * afterwards. A second request of the very same line makes
>> +		 * gpiolib fail with -EBUSY, which devm_gpiod_get_optional()
>> +		 * passes through because it only filters out -ENOENT. The
>> +		 * reset would then abort early and leave the PHY un-reset.
>> +		 */
>> +		if (!priv->mdio_reset_gpio) {
>> +			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);
>> +		}
>> +		reset_gpio = priv->mdio_reset_gpio;
> 
> The problem is real, but this isn't the correct approach to solve that.
> 
> Instead of requesting the reset gpio on-the-fly at reset time (good example
> of when not to use devm_xxx) , let's request the gpio at probe time.
> 
> I suggest you move the devm_gpiod_get_optional() in stmmac_mdio_register(),
> which is called at probe time. Here, it makes sense to use devm_xxx.
> 

You're right, requesting the GPIO in the reset callback is the wrong
place for devm_xxx.

I'll send a v2 that moves the devm_gpiod_get_optional() call to
stmmac_mdio_register(), which runs at probe time. stmmac_mdio_reset()
will then just use the descriptor stored in stmmac_priv.

Thanks,
Linkui

> Thanks :)
> 
> Maxime
>