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

[Paolo Abeni <pabeni@redhat.com>]

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));
>  	}