Re: [PATCH v7 1/8] regulator: core: Add "enable and wait" functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] regulator: core: unlocked _regulator_is_enabled_recursive() in early resolve
- [Medium] regulator: core: stale last_on timestamp after disable skips enable wait
- [Medium] regulator: core: uninitialized last_on in set_machine_constraints()
--

commit 10272782c962e94533f9c00c5f660ea31ea2ca13
Author: Chen-Yu Tsai <wenst@chromium.org>

regulator: core: Add "enable and wait" functions

This commit adds a last_on timestamp field to struct regulator_dev and
introduces regulator_enable_and_wait() and regulator_bulk_enable_and_wait()
to wait for the remaining turn-on delay after a regulator is actually
enabled.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 14422cfe80e2d..071f9f8185f63 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c

[ ... ]

> @@ -2188,6 +2188,45 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
>  	return ERR_PTR(-ENODEV);
>  }
>  
> +/**
> + * _regulator_is_enabled_recursive - is the regulator output enabled all
> + *				     the way to the root supply
> + * @rdev: regulator device
> + *
> + * Only intended for checking regulators that have been left on by
> + * hardware default or firmware.
> + *
> + * Assumes supplies have been resolved. Must be called with dependent
> + * locks held.
> + *
> + * Return: Positive if the regulator output backing the source/client,
> + *	   and all of its upstream supplies, have requested that their
> + *	   respective device be enabled, zero if any of them hasn't,
> + *	   else a negative error number.
> + */
> +static int _regulator_is_enabled_recursive(struct regulator_dev *rdev)
> +{
> +	int ret;
> +
> +	ret = _regulator_is_enabled(rdev);
> +	if (ret <= 0)
> +		return ret;
> +
> +	/*
> +	 * If .last_on was set, then this rdev was either enabled through
> +	 * _regulator_do_enable(), or had been checked before as the target
> +	 * of regulator_resolve_supply().
> +	 */
> +	if (rdev->last_on)
> +		return ret;
> +
> +	/* This is the root supply; we can only assume it actually has power */
> +	if (!rdev->supply)
> +		return ret;
> +
> +	return _regulator_is_enabled_recursive(rdev->supply->rdev);
> +}
> +
>  static int regulator_resolve_supply(struct regulator_dev *rdev)
>  {
>  	struct regulator_dev *r;
> @@ -2373,6 +2412,14 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
>  		}
>  		rdev->constraints_pending = false;
>  	}
> +
> +	/*
> +	 * regulator was left on but not enabled with .always_on or .boot_on
> +	 * constraints, and thus .last_on timestamp is still invalid.
> +	 */
> +	if (!rdev->last_on && _regulator_is_enabled_recursive(rdev) > 0)
> +		rdev->last_on = ktime_get_boottime();
> +
>  	regulator_unlock_dependent(rdev, &ww_ctx);

[Severity: Medium]
Does this call _regulator_is_enabled_recursive(rdev) without holding any
regulator locks during the early supply resolution path in
regulator_register()?

In drivers/regulator/core.c:regulator_register(), when the initial call to
set_machine_constraints(rdev, false) returns -EPROBE_DEFER,
regulator_resolve_supply(rdev) is called before regulator_init_coupling(rdev)
has initialized rdev->coupling_desc.n_coupled:

drivers/regulator/core.c:regulator_register() {
    ...
	ret = set_machine_constraints(rdev, false);
	if (ret == -EPROBE_DEFER) {
		rdev_dbg(rdev, "will resolve supply early: %s\n",
			 rdev->supply_name);
		ret = regulator_resolve_supply(rdev);
		if (!ret)
			ret = set_machine_constraints(rdev, false);
		else
			rdev_dbg(rdev, "unable to resolve supply early: %pe\n",
				 ERR_PTR(ret));
		tried_supply_resolve = true;
	}
    ...
	ret = regulator_init_coupling(rdev);
    ...
}

At that point, rdev->coupling_desc.n_coupled is still 0, so
regulator_lock_dependent() -> regulator_lock_recursive() performs zero loop
iterations:

drivers/regulator/core.c:regulator_lock_recursive() {
    ...
	for (i = 0; i < rdev->coupling_desc.n_coupled; i++) {
		c_rdev = rdev->coupling_desc.coupled_rdevs[i];
        ...
	}
    ...
}

Previously, rdev->constraints_pending was false during that early call, so
nothing ran inside the regulator_lock_dependent() section. With this change,
_regulator_is_enabled_recursive(rdev) runs outside the
if (rdev->constraints_pending) block and walks rdev and its live upstream
supply chain without holding rdev->mutex or any supply mutexes.

[ ... ]

> @@ -3180,13 +3231,26 @@ static int _regulator_enable(struct regulator *regulator)
>  		} else if (ret < 0) {
>  			rdev_err(rdev, "is_enabled() failed: %pe\n", ERR_PTR(ret));
>  			goto err_consumer_disable;
> +		} else {
> +			/*
> +			 * Either the regulator was already enabled somehow, or this regulator
> +			 * itself has no on/off control. Either way our timestamp cannot be
> +			 * earlier than its supply's timestamp. supply_last_on == 0 if there
> +			 * is no supply.
> +			 */
> +			if (rdev->last_on < supply_last_on)
> +				rdev->last_on = supply_last_on;
> +			if (!rdev->last_on)
> +				rdev->last_on = ktime_get_boottime();
>  		}
> -		/* Fallthrough on positive return values - already enabled */
>  	}

[Severity: Medium]
Can rdev->last_on hold a stale timestamp from before a previous disable,
causing regulator_enable_and_wait() to skip the wait_us delay?

When a regulator is disabled in drivers/regulator/core.c:_regulator_do_disable(),
rdev->last_off is updated if off_on_delay is set, but rdev->last_on is never
reset to 0:

drivers/regulator/core.c:_regulator_do_disable() {
    ...
	if (rdev->desc->off_on_delay)
		rdev->last_off = ktime_get_boottime();

	trace_regulator_disable_complete(rdev_get_name(rdev));

	return 0;
}

This can leave a stale rdev->last_on in two ways:

1. If a regulator is enabled at T1 (setting rdev->last_on = T1), disabled via
_regulator_do_disable() at T2, and later re-enabled in hardware while
rdev->use_count == 0 (for example by firmware across suspend/resume or via
regulator_hardware_enable()), a subsequent regulator_enable_and_wait() call
sees rdev->use_count == 0 and _regulator_is_enabled(rdev) > 0. In
_regulator_enable(), if rdev has no supply or its supply remained on since
T1 (so supply_last_on <= T1), the check if (!rdev->last_on) is false because
rdev->last_on is still T1. As a result, rdev->last_on is not updated to
ktime_get_boottime() and regulator_enable_and_wait() skips the required wait.

2. In _regulator_is_enabled_recursive(), if (rdev->last_on) short-circuits
the walk up the supply chain. In a supply chain root -> mid -> leaf where
root and mid are initially on (setting root->last_on and mid->last_on) and
root is later disabled while mid->use_count == 0 and
_regulator_is_enabled(mid) > 0 (such as a passive/fixed regulator without an
is_enabled() op), mid->last_on remains non-zero. When leaf resolves its
supply in regulator_resolve_supply(), _regulator_is_enabled_recursive(mid)
returns positive without checking root, setting leaf->last_on =
ktime_get_boottime() while root is actually off.

Should rdev->last_on be cleared when a regulator or its upstream supply is
disabled?

[ ... ]

> @@ -6251,6 +6341,15 @@ regulator_register(struct device *dev,
>  			goto del_cdev_and_bdev;
>  	}
>  
> +	/*
> +	 * If no supply was given, then the last_on timestamp could not have
> +	 * been updated in regulator_resolve_supply(). Check it here.
> +	 */
> +	regulator_lock(rdev);
> +	if (!rdev->supply_name && !rdev->last_on && _regulator_is_enabled(rdev) > 0)
> +		rdev->last_on = ktime_get_boottime();
> +	regulator_unlock(rdev);

[Severity: Medium]
What happens to rdev->last_on when set_machine_constraints() enables a
regulator that has no direct enable operation (!rdev->ena_pin &&
!rdev->desc->ops->enable)?

In drivers/regulator/core.c:set_machine_constraints(), when always_on or
boot_on is set on a regulator without enable ops, _regulator_do_enable(rdev)
returns -EINVAL before reaching rdev->last_on = ktime_get_boottime(), and
set_machine_constraints() ignores -EINVAL and increments rdev->use_count for
always_on:

drivers/regulator/core.c:set_machine_constraints() {
    ...
		ret = _regulator_do_enable(rdev);
		if (ret < 0 && ret != -EINVAL) {
			rdev_err(rdev, "failed to enable: %pe\n", ERR_PTR(ret));
			if (supply_enabled)
				regulator_disable(rdev->supply);
			return ret;
		}

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

This appears to leave rdev->last_on as 0 in two cases:

1. For a regulator with a supply (rdev->supply_name != NULL), no enable ops,
and boot_on set whose supply is initially off, the first
set_machine_constraints(rdev, false) call in regulator_register() returns
-EPROBE_DEFER while rdev->constraints_pending is still false. Early supply
resolution in regulator_resolve_supply(rdev) then runs
_regulator_is_enabled_recursive(rdev) while rdev->supply is still off,
leaving rdev->last_on as 0. Next, regulator_register() calls
set_machine_constraints(rdev, false) a second time, which enables
rdev->supply, gets -EINVAL from _regulator_do_enable(rdev), and leaves
rdev->last_on as 0. Because tried_supply_resolve is true and
rdev->supply_name is non-NULL, neither the second regulator_resolve_supply()
call nor the !rdev->supply_name check above runs, leaving rdev->last_on as 0
after registration completes.

2. For a supply-less always_on regulator without enable ops,
set_machine_constraints() sets rdev->use_count = 1 with rdev->last_on = 0,
and regulator_register() publishes the regulator via
set_consumer_device_supply() and device_add(&rdev->dev) before reaching the
!rdev->supply_name check above. If a consumer probes concurrently on another
thread and calls regulator_enable_and_wait() in that window,
_regulator_enable() skips the rdev->use_count == 0 branch, reads
last_on == 0, and returns without waiting for wait_us.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103043734.2179858-1-wenst@chromium.org?part=1