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

["Paul Louvel" <paul.louvel@bootlin.com>]

On Wed Oct 21, 2026 at 12:46 PM CEST, sashiko-bot wrote:
> 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?

Yes, any idea why there is a state instead of turning off the gpio instead ?

>
>> +
>>  /**
>>   * 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?

Yes probably.
What was on my mind mind is that the boot_force_off property has higher
priority. But this is not documented / I don't know if it is an acceptable
behavior.

>
> [ ... ]
>
>> 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?





-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com