Re: [PATCH 3/5] regulator: core: Add mechanism to force disable a regulator at boot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] regulator: core: missing mutual exclusion for boot_force_off in set_machine_constraints()
- [High] regulator: core: boot_force_off is a no-op for GPIO-controlled regulators
- [Medium] regulator: core: boot_force_off works around consumer API misuse
- [Low] regulator: missing kernel-doc for boot_force_off in struct regulation_constraints
--

commit 934802b5e041e184ae3df8fc8a423b7e086bf66c
Author: Paul Louvel <paul.louvel@bootlin.com>

regulator: core: Add mechanism to force disable a regulator at boot

This commit adds the regulator-boot-force-off device tree property and the
corresponding boot_force_off constraint flag to disable a regulator in
set_machine_constraints() during registration.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 14422cfe80e2d..b45048ec084a1 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1450,6 +1450,81 @@ static int handle_notify_limits(struct regulator_dev *rdev,
>  
>  	return ret;
>  }
> +

[ ... ]

> +static int _regulator_do_disable(struct regulator_dev *rdev)
> +{
> +	int ret;
> +
> +	trace_regulator_disable(rdev_get_name(rdev));
> +
> +	if (rdev->ena_pin) {
> +		if (rdev->ena_gpio_state) {
> +			ret = regulator_ena_gpio_ctrl(rdev, false);
> +			if (ret < 0)
> +				return ret;
> +			rdev->ena_gpio_state = 0;
> +		}
> +
> +	} else if (rdev->desc->ops->disable) {
> +		ret = rdev->desc->ops->disable(rdev);
> +		if (ret != 0)
> +			return ret;
> +	}
> +
> +	if (rdev->desc->off_on_delay)
> +		rdev->last_off = ktime_get_boottime();
> +
> +	trace_regulator_disable_complete(rdev_get_name(rdev));
> +
> +	return 0;
> +}

[Severity: High]
Does boot_force_off work for regulators that use an enable gpio
(rdev->ena_pin)?

During regulator_register(), rdev is zero-allocated and
regulator_ena_gpio_request() attaches rdev->ena_pin without setting
rdev->ena_gpio_state. When regulator_register() -> set_machine_constraints()
calls _regulator_do_disable(rdev) for boot_force_off (with always_on and
boot_on unset), rdev->ena_gpio_state is still 0.

In _regulator_do_disable(), when rdev->ena_pin is present and
rdev->ena_gpio_state is 0, the code skips regulator_ena_gpio_ctrl(rdev, false)
and also skips the else-if branch for rdev->desc->ops->disable(rdev).

Won't this leave gpio-controlled regulators enabled in hardware at boot?

> +
>  /**
>   * set_machine_constraints - sets regulator constraints
>   * @rdev: regulator source
> @@ -1701,6 +1776,12 @@ static int set_machine_constraints(struct regulator_dev *rdev,
>  		rdev->last_off = ktime_get();
>  	}
>  
> +	if (rdev->constraints->boot_force_off) {
> +		ret = _regulator_do_disable(rdev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	if (!rdev->constraints->pw_budget_mW)
>  		rdev->constraints->pw_budget_mW = INT_MAX;
>  

[Severity: High]
What happens in set_machine_constraints() if boot_force_off is set together
with always_on or boot_on?

Neither of_get_regulation_constraints() nor set_machine_constraints() prevents
boot_force_off from being active at the same time as always_on or boot_on
(including when set_machine_constraints() implicitly sets
rdev->constraints->always_on = true when !rdev->ena_pin && !ops->enable).

Right before this boot_force_off check, set_machine_constraints() handles
always_on and boot_on:

drivers/regulator/core.c:set_machine_constraints() {
	if (rdev->constraints->always_on || rdev->constraints->boot_on) {
		...
		if (rdev->supply &&
		    (rdev->constraints->always_on ||
		     !regulator_is_enabled(rdev->supply))) {
			ret = (is_locked
			       ? _regulator_enable(rdev->supply)
			       : regulator_enable(rdev->supply));
			...
			supply_enabled = true;
		}

		ret = _regulator_do_enable(rdev);
		...
		if (rdev->constraints->always_on)
			rdev->use_count++;
	}
}

Because boot_force_off is checked in a separate conditional afterwards,
set_machine_constraints() will enable rdev->supply and rdev, and then
immediately call _regulator_do_disable(rdev).

If _regulator_do_disable(rdev) fails after rdev->supply was enabled,
set_machine_constraints() returns the error without disabling rdev->supply,
and regulator_register() jumps to the wash cleanup path:

drivers/regulator/core.c:regulator_register() {
wash:
	regulator_put(rdev->supply);
}

which triggers the warning in _regulator_put() and leaks the supply enable
count:

drivers/regulator/core.c:_regulator_put() {
	/* Docs say you must disable before calling regulator_put() */
	WARN_ON(regulator->enable_count);
}

And if _regulator_do_disable(rdev) succeeds when always_on or boot_on is set,
the hardware output is disabled while rdev->use_count remains 1 (for
always_on, preventing subsequent regulator_enable() calls from re-enabling the
hardware) and rdev->supply remains enabled with an unbalanced enable_count.

Should boot_force_off be mutually exclusive with always_on and boot_on?

[ ... ]

> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
> index 09e26febfc834..337629be58ca7 100644
> --- a/drivers/regulator/of_regulator.c
> +++ b/drivers/regulator/of_regulator.c
> @@ -132,6 +132,7 @@ static int of_get_regulation_constraints(struct device *dev,
>  	constraints->always_on = of_property_read_bool(np, "regulator-always-on");
>  	if (!constraints->always_on) /* status change should be possible. */
>  		constraints->valid_ops_mask |= REGULATOR_CHANGE_STATUS;
> +	constraints->boot_force_off = of_property_read_bool(np, "regulator-boot-force-off");
>  
>  	constraints->pull_down = of_property_read_bool(np, "regulator-pull-down");
>  	constraints->system_critical = of_property_read_bool(np,

[Severity: Medium]
Is a new device tree property the right way to handle consumers that want to
disable a boot-enabled regulator during probe?

In the regulator framework, regulator_enable() and regulator_disable() calls
on a standard consumer handle must be balanced because regulator_get()
initializes regulator->enable_count to 0 regardless of hardware state.

For consumers that have exclusive control of a supply and need to turn off a
regulator that was left enabled at boot without a prior regulator_enable(),
regulator_get_exclusive() already syncs use_count and enable_count with the
hardware state in _regulator_get_common():

drivers/regulator/core.c:_regulator_get_common() {
	if (get_type == EXCLUSIVE_GET) {
		rdev->exclusive = 1;

		ret = _regulator_is_enabled(rdev);
		if (ret > 0) {
			rdev->use_count = 1;
			regulator->enable_count = 1;
}

Could the affected consumer driver use regulator_get_exclusive() (or
devm_regulator_get_exclusive()) instead of adding regulator-boot-force-off to
device tree?

> diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
> index 1fc440c5c4c7e..35ab9de96e5c7 100644
> --- a/include/linux/regulator/machine.h
> +++ b/include/linux/regulator/machine.h
> @@ -227,7 +227,8 @@ struct regulation_constraints {
>  	unsigned int active_discharge;
>  
>  	/* constraint flags */
> -	unsigned always_on:1;	/* regulator never off when system is on */
> +	unsigned always_on:1;		/* regulator never off when system is on */
> +	unsigned boot_force_off:1;	/* force regulator off at boot */
>  	unsigned boot_on:1;	/* bootloader/firmware enabled regulator */
>  	unsigned apply_uV:1;	/* apply uV constraint if min == max */
>  	unsigned ramp_disable:1; /* disable ramp delay */

[Severity: Low]
Struct regulation_constraints in include/linux/regulator/machine.h has a
kernel-doc comment block above the definition describing its fields. Should an
@boot_force_off entry be added there as well to avoid a kernel-doc warning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-paul-lm51772-v1-0-25f0ffbffa8d@bootlin.com?part=3