Re: [PATCH net-next v1 2/2] net: dsa: realtek: rtl83xx: add support for enabling supplies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Potential back-powering of unpowered chip during device removal or probe failure.
--

--- Patch [2]: [PATCH net-next v1 2/2] net: dsa: realtek: rtl83xx: add support for enabling supplies ---
commit bb86fcef91dead11293d5508eb30d8736c1afc82
Author: Ahmad Fatoum <a.fatoum@pengutronix.de>

net: dsa: realtek: rtl83xx: add support for enabling supplies

This commit extends the Realtek DSA driver to optionally enable power
supplies specified in the device tree. The supplies are enabled before
the reset line is driven to prevent forcing current into unpowered pins.

> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 35df809a59518..fb6aaaa60aa35 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.c
> +++ b/drivers/net/dsa/realtek/rtl83xx.c

[ ... ]

> @@ -195,17 +215,22 @@ rtl83xx_probe(struct device *dev,
>  	priv->leds_disabled = of_property_read_bool(dev->of_node,
>  						    "realtek,disable-leds");
>  
> -	/* TODO: if power is software controlled, set up any regulators here */
> +	/* Enable the supplies before the RESET line is requested and driven,
> +	 * so the chip is powered before current is forced into its pins.
> +	 */
> +	num_supplies = rtl83xx_enable_supplies(dev, var->supplies);
> +	if (num_supplies < 0)
> +		return ERR_PTR(num_supplies);
> +
>  	priv->reset_ctl = devm_reset_control_get_optional(dev, NULL);
>  	if (IS_ERR(priv->reset_ctl))
>  		return dev_err_cast_probe(dev, priv->reset_ctl,
>  					  "failed to get reset control\n");
>  
>  	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);

[Severity: High]
Does this allocation order cause the unpowered chip to be back-powered
by the SoC during driver unbind or probe failure?

Because the devm regulators are enabled before the devm reset controls are
allocated, devres will clean them up in the reverse order. The reset
control and GPIO will be released first, followed by the regulators being
disabled.

Releasing a GPIO via gpiod_put() or a reset control doesn't automatically
assert it or change it to a high-impedance input state; they remain driven
to their last active value (deasserted). Then devres disables the
regulators, leaving the switch unpowered while the SoC continues to actively
drive the reset pin.

> -	if (IS_ERR(priv->reset)) {
> -		dev_err(dev, "failed to get RESET GPIO\n");
> -		return ERR_CAST(priv->reset);
> -	}
> +	if (IS_ERR(priv->reset))
> +		return dev_err_cast_probe(dev, priv->reset,
> +					  "failed to get RESET GPIO\n");
>  
>  	dev_set_drvdata(dev, priv);
>  
> @@ -214,10 +239,13 @@ rtl83xx_probe(struct device *dev,
>  		dev_dbg(dev, "asserted RESET\n");
>  		msleep(REALTEK_HW_STOP_DELAY);
>  		rtl83xx_reset_deassert(priv);
> -		msleep(REALTEK_HW_START_DELAY);
>  		dev_dbg(dev, "deasserted RESET\n");
>  	}

Is a custom devm action (like devm_add_action_or_reset) needed here to
safely assert the reset line before the regulators are disabled on teardown?

>  
> +	/* Wait for the chip to come up only if we powered and/or reset it. */
> +	if (priv->reset_ctl || priv->reset || num_supplies)
> +		msleep(REALTEK_HW_START_DELAY);
> +
>  	return priv;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806140239.3518906-1-o.rempel@pengutronix.de?part=2