Re: [PATCH v2 3/4] regulator: core: Call set_over_voltage_protection() if OVP limit is fixed after setting new voltage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] regulator: core: late fixed_ovp update in _regulator_do_set_voltage()
- [Medium] regulator: core: state desync when set_over_voltage_protection() fails
--

commit 842d424c282842b568e77b33fac660e8e08d0998
Author: Paul Louvel <paul.louvel@bootlin.com>

regulator: core: Call set_over_voltage_protection() if OVP limit is fixed after setting new voltage

This commit introduces a fixed_ovp flag in struct regulator_desc for
regulators whose over-voltage protection threshold is programmed as an
absolute value rather than an offset. When the output voltage changes in
_regulator_do_set_voltage(), it calls set_over_voltage_protection() via
handle_notify_limits() after the slew-rate delay.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 14422cfe80e2d..b1cabe50ac256 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -4014,6 +4014,16 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
>  				     (void *)data);
>  	}
>  
> +	if (old_uV != best_val) {
> +		if (rdev->desc->fixed_ovp && ops->set_over_voltage_protection) {
> +			ret = handle_notify_limits(
> +				rdev, ops->set_over_voltage_protection,
> +				&rdev->constraints->over_voltage_limits);
> +			if (ret == -EOPNOTSUPP)
> +				ret = 0;
> +		}
> +	}

[Severity: High]
When increasing the output voltage (best_val > old_uV) above the previous
over-voltage protection threshold, won't reprogramming the OVP limit after
the voltage write and slew-rate delay trigger a spurious hardware OVP fault?

In _regulator_do_set_voltage(), the new target voltage best_val is written to
hardware first (via _regulator_call_set_voltage(),
_regulator_set_voltage_sel_step(), or _regulator_call_set_voltage_sel()), and
then fsleep(delay) waits for the output voltage to slew to best_val before
handle_notify_limits() is reached:

drivers/regulator/core.c:_regulator_do_set_voltage() {
	...
	/* Insert any necessary delays */
	fsleep(delay);

	if (best_val >= 0) {
		unsigned long data = best_val;

		_notifier_call_chain(rdev, REGULATOR_EVENT_VOLTAGE_CHANGE,
				     (void *)data);
	}

	if (old_uV != best_val) {
		if (rdev->desc->fixed_ovp && ops->set_over_voltage_protection) {
			ret = handle_notify_limits(
				rdev, ops->set_over_voltage_protection,
				&rdev->constraints->over_voltage_limits);
	...
}

Waiting until after the slew-rate delay is necessary when decreasing the
voltage (best_val < old_uV) so the threshold is not lowered before the output
has slewed down. However, when raising the voltage (best_val > old_uV) to a
level above the old threshold (old_uV + lim_uV), the old lower OVP threshold
remains active in hardware while the physical output slews upward during
fsleep(delay), causing the rising output voltage to cross the old OVP limit
before ops->set_over_voltage_protection() is called.

Should _regulator_do_set_voltage() update the OVP threshold before raising
the hardware voltage when best_val > old_uV, and after fsleep(delay) when
best_val < old_uV? Note that because ops->set_over_voltage_protection() only
receives the offset lim_uV and drivers may read back the current hardware
voltage via regulator_get_voltage_rdev(), reprogramming OVP prior to raising
the voltage may also require passing or exposing the target best_val.

[Severity: Medium]
What happens to the regulator state if handle_notify_limits() returns an
error other than -EOPNOTSUPP here?

At this point in _regulator_do_set_voltage(), the hardware voltage has
already been changed to best_val and REGULATOR_EVENT_VOLTAGE_CHANGE has
already been emitted. Overwriting ret with a negative error from
handle_notify_limits() causes regulator_set_voltage_unlocked() ->
regulator_balance_voltage() -> regulator_do_balance_voltage() ->
regulator_set_voltage_rdev() -> _regulator_do_set_voltage() to fail without
reverting the hardware voltage.

In regulator_set_voltage_unlocked(), a negative return rolls back the
consumer's requested voltage range to old_min_uV and old_max_uV even though
the hardware remains at best_val:

drivers/regulator/core.c:regulator_set_voltage_unlocked() {
	...
	/* for not coupled regulators this will just set the voltage */
	ret = regulator_balance_voltage(rdev, state);
	if (ret < 0) {
		voltage->min_uV = old_min_uV;
		voltage->max_uV = old_max_uV;
	}
	...
}

Because voltage->min_uV and voltage->max_uV are reset to old_min_uV and
old_max_uV, a subsequent regulator_set_voltage() call attempting to restore
old_min_uV and old_max_uV will hit the early no-op check:

	if (voltage->min_uV == min_uV && voltage->max_uV == max_uV)
		goto out;

Additionally, in regulator_set_voltage_rdev(), a negative return from
_regulator_do_set_voltage() jumps to out before lowering the parent supply
voltage when supply_change_uV < 0:

drivers/regulator/core.c:regulator_set_voltage_rdev() {
	...
	if (state == PM_SUSPEND_ON)
		ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
	else
		ret = _regulator_do_set_suspend_voltage(rdev, min_uV,
							max_uV, state);
	if (ret < 0)
		goto out;

	if (supply_change_uV < 0) {
		ret = regulator_set_voltage_unlocked(rdev->supply,
				best_supply_uV, INT_MAX, state);
	...
}

Could this leave the software state and parent supply out of sync with the
actual hardware voltage if ops->set_over_voltage_protection() fails (for
example, due to an out-of-range OVP calculation or a register write error)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-paul-lm51772-v2-0-b2a767e34722@bootlin.com?part=3