Re: [PATCH v2 2/2] regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropping the runtime PM reference in fp9931_v3p3_enable() allows the PMIC to auto-suspend, physically powering off the active V3P3 regulator and violating framework guarantees.
- [High] Disabling the V3P3 regulator while the PMIC is runtime-suspended causes a spurious 3.3V power-on glitch.
--

--- Patch [2]: [PATCH v2 2/2] regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops ---
commit 195ff0977cf3484ecb06ab9189df30e12979d50c
Author: Robby Cai <[email protected]>

regulator: fp9931: Fix Runtime PM usage count underflow in v3p3 ops

This commit scopes the Runtime PM reference lifetime to the register
access itself in the v3p3 enable and disable callbacks. It pairs
pm_runtime_resume_and_get() and pm_runtime_put_autosuspend() within
each function to fix a usage count underflow issue.

> diff --git a/drivers/regulator/fp9931.c b/drivers/regulator/fp9931.c
> index ff743a8b0dfe8..0d926546221a6 100644
> --- a/drivers/regulator/fp9931.c
> +++ b/drivers/regulator/fp9931.c

[ ... ]

> @@ -214,8 +214,7 @@ static int fp9931_v3p3_enable(struct regulator_dev *rdev)
>  		return ret;
>  
>  	ret = regulator_enable_regmap(rdev);
> -	if (ret < 0)
> -		pm_runtime_put_autosuspend(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);

[Severity: High]
Does dropping the runtime PM reference here allow the PMIC to auto-suspend
while the V3P3 regulator is still logically enabled by consumers?

If a consumer enables V3P3, fp9931_v3p3_enable() will acquire and then
unconditionally release the reference after setting the register. 

When the PMIC subsequently auto-suspends, fp9931_runtime_suspend() could
disable the main vin_reg power supply, causing the V3P3 output to physically
power down and break framework guarantees.

>  
>  	return ret;
>  }
> @@ -225,6 +224,10 @@ static int fp9931_v3p3_disable(struct regulator_dev *rdev)
>  	struct fp9931_data *data = rdev_get_drvdata(rdev);
>  	int ret;
>  
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = regulator_disable_regmap(rdev);

[Severity: High]
Can this resume trigger a spurious 3.3V power-on glitch if the device was
already suspended?

Since the device can now auto-suspend while V3P3 is logically enabled, calling
regulator_disable() could trigger fp9931_v3p3_disable() while the device is
suspended.

In this state, pm_runtime_resume_and_get() wakes the device and
invokes fp9931_runtime_resume(), which calls regcache_sync().

Because the regcache still contains the 'enabled' state for V3P3, wouldn't
regcache_sync() write that state to hardware, physically turning V3P3 ON just
before regulator_disable_regmap() clears the bit and turns it OFF?

>  	pm_runtime_put_autosuspend(data->dev);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2