Re: [PATCH net-next v2 02/14] net: phy: qcom: qca808x: Add regulator management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The PHY regulators are enabled during `probe()` without re-triggering a hardware reset, violating power-on sequencing, and the code incorrectly repurposes the MDIO `reset_assert_delay` as a post-power-on stabilization delay.
--

commit 7b9b83105850db948e3794f9c1593ea2fbeac999
Author: Daniel Thompson <daniel@riscstar.com>

    net: phy: qcom: qca808x: Add regulator management

This commit adds optional regulator management to the QCA8081 PHY driver.
It attempts to enable the regulators during the probe sequence.

> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index 8eb51b1a006c4..fc3f2cf2e55d0 100644
> --- a/drivers/net/phy/qcom/qca808x.c
> +++ b/drivers/net/phy/qcom/qca808x.c

[ ... ]

> @@ -178,15 +179,44 @@ static void qca808x_fill_possible_interfaces(struct phy_device *phydev)
>  		__set_bit(PHY_INTERFACE_MODE_2500BASEX, possible);
>  }
>  
> +static int qca808x_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF
> +	static const char * const regulator_names[] = {
> +		"avdd18", "vdd", "vdd18", "vdd125"
> +	};
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 count = 0;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(regulator_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, regulator_names[i]);
> +		if (!ret)
> +			count++;
> +		else if (ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	if (count)
> +		fsleep(phydev->mdio.reset_assert_delay);

[Severity: High]
Does this apply power to the PHY without re-asserting the hardware reset
line?

The Linux PHY framework pulses the hardware reset line (asserting and then
deasserting via phy_device_reset()) during device registration, which happens
before the driver's probe callback is invoked. By enabling the regulators
here inside probe(), power is applied while the reset line is already
deasserted, which violates standard hardware power-on sequencing and can
leave the PHY in an undefined or unstable state.

Additionally, is it safe to repurpose the MDIO reset_assert_delay for this
settling time?

This uses the reset-assert-us device tree property (meant to dictate the
minimum active pulse width of the reset signal) as a post-power-on settling
delay. If a board's device tree lacks a reset-assert-us property,
reset_assert_delay will be 0, causing fsleep(0) to return immediately. The
lack of a proper stabilization delay could cause subsequent MDIO accesses to
fail.

> +#endif
> +	return 0;
> +}
> +
>  static int qca808x_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
>  	struct qca808x_priv *priv;
> +	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	ret = qca808x_power_on(phydev);
> +	if (ret)
> +		return ret;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605010022.968612-1-elder@riscstar.com?part=2