[PATCH] net: stmmac: avoid repeated devm_gpiod_get_optional in mdio reset on resume

[PATCH] net: stmmac: avoid repeated devm_gpiod_get_optional in mdio reset on resume

[Liu Xiang]

During system resume, stmmac_resume() calls stmmac_mdio_reset().
This results in repeated calls to devm_gpiod_get_optional() for the same
GPIO. The second invocation returns an error since the GPIO descriptor has
already been obtained.

Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h   |  2 ++
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c  | 18 +++++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 33667a267..34ebc6f3a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -372,6 +372,8 @@ struct stmmac_priv {
 	struct bpf_prog *xdp_prog;
 
 	struct devlink *devlink;
+
+	struct gpio_desc *reset_gpio;
 };
 
 enum stmmac_state {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index a7c2496b3..811ded1c5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -386,15 +386,15 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 
 #ifdef CONFIG_OF
 	if (priv->device->of_node) {
-		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);
-
+		if (!priv->reset_gpio) {
+			priv->reset_gpio = devm_gpiod_get_optional(priv->device,
+								   "snps,reset",
+								 GPIOD_OUT_LOW);
+			if (IS_ERR(priv->reset_gpio))
+				return PTR_ERR(priv->reset_gpio);
+		}
 		device_property_read_u32_array(priv->device,
 					       "snps,reset-delays-us",
 					       delays, ARRAY_SIZE(delays));
@@ -402,11 +402,11 @@ int stmmac_mdio_reset(struct mii_bus *bus)
 		if (delays[0])
 			msleep(DIV_ROUND_UP(delays[0], 1000));
 
-		gpiod_set_value_cansleep(reset_gpio, 1);
+		gpiod_set_value_cansleep(priv->reset_gpio, 1);
 		if (delays[1])
 			msleep(DIV_ROUND_UP(delays[1], 1000));
 
-		gpiod_set_value_cansleep(reset_gpio, 0);
+		gpiod_set_value_cansleep(priv->reset_gpio, 0);
 		if (delays[2])
 			msleep(DIV_ROUND_UP(delays[2], 1000));
 	}
-- 
2.34.1

Re: net: stmmac: avoid repeated devm_gpiod_get_optional in mdio reset on resume

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: avoid repeated devm_gpiod_get_optional in mdio reset on resume

This commit prevents repeated calls to devm_gpiod_get_optional() during
system resume by moving the reset GPIO descriptor to struct stmmac_priv
and only fetching it once on the first call.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 7a66edba8f66..a97ec8d18b40 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -381,6 +381,8 @@ struct stmmac_priv {
>  	struct bpf_prog *xdp_prog;
>
>  	struct devlink *devlink;
> +
> +	struct gpio_desc *reset_gpio;
>  };
>
>  enum stmmac_state {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index afe98ff5bdcb..da1b256bbb6e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -386,15 +386,15 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>
>  #ifdef CONFIG_OF
>  	if (priv->device->of_node) {
> -		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);
> -
> +		if (!priv->reset_gpio) {
                    ^^^^^^^^^^^^^^^

What happens if devm_gpiod_get_optional() fails and stores ERR_PTR in
priv->reset_gpio on the first call?

On subsequent resumes, this check evaluates to false since ERR_PTR is
not NULL, so the allocation block gets skipped. The code then passes
ERR_PTR to gpiod_set_value_cansleep() which returns errors that are
silently ignored, causing the MDIO reset sequence to be skipped.

First resume (acquisition fails):

    stmmac_mdio_reset() {
        if (!priv->reset_gpio) {  // true, priv->reset_gpio is NULL
            priv->reset_gpio = devm_gpiod_get_optional(...);
            if (IS_ERR(priv->reset_gpio))
                return PTR_ERR(priv->reset_gpio);  // returns error
        }
        // priv->reset_gpio now contains ERR_PTR(-errno)
    }

Second resume (reset silently skipped):

    stmmac_mdio_reset() {
        if (!priv->reset_gpio) {  // false, ERR_PTR is not NULL
            // skipped
        }
        gpiod_set_value_cansleep(priv->reset_gpio, 1);  // ERR_PTR passed
        gpiod_set_value_cansleep(priv->reset_gpio, 0);  // ERR_PTR passed
        return 0;  // success despite reset being skipped
    }

Should this use IS_ERR_OR_NULL() instead, or set priv->reset_gpio to
NULL on error?

> +			priv->reset_gpio = devm_gpiod_get_optional(priv->device,
> +								   "snps,reset",
> +							 GPIOD_OUT_LOW);
> +			if (IS_ERR(priv->reset_gpio))
> +				return PTR_ERR(priv->reset_gpio);
> +		}
>  		device_property_read_u32_array(priv->device,
>  					       "snps,reset-delays-us",
>  					       delays, ARRAY_SIZE(delays));
> @@ -402,11 +402,11 @@ int stmmac_mdio_reset(struct mii_bus *bus)
>  		if (delays[0])
>  			msleep(DIV_ROUND_UP(delays[0], 1000));
>
> -		gpiod_set_value_cansleep(reset_gpio, 1);
> +		gpiod_set_value_cansleep(priv->reset_gpio, 1);
>  		if (delays[1])
>  			msleep(DIV_ROUND_UP(delays[1], 1000));
>
> -		gpiod_set_value_cansleep(reset_gpio, 0);
> +		gpiod_set_value_cansleep(priv->reset_gpio, 0);
>  		if (delays[2])
>  			msleep(DIV_ROUND_UP(delays[2], 1000));
>  	}