Re: [PATCH v3 1/9] regulator: core: Add "enable and wait" functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The prototype for `regulator_enable_and_wait` in `consumer.h` incorrectly names the delay parameter `ms` instead of `wait_us`, inviting developers to pass millisecond values that will be incorrectly interpreted as microseconds.
- [High] Sleeping for the consumer delay while holding the recursive `ww_mutex` for the regulator and its entire parent supply chain severely degrades concurrency, blocking shared regulators and serializing async bulk enablement.
--

--- Patch [1]: [PATCH v3 1/9] regulator: core: Add "enable and wait" functions ---
commit 73793dc0bdc3050ff22074a860d438d5d9cc0b49
Author: Chen-Yu Tsai <wenst@chromium.org>

regulator: core: Add "enable and wait" functions

This commit introduces new "enable and wait" functions to the regulator core,
allowing consumers to ensure a minimum amount of time has passed since a
regulator was actually turned on. It adds a last_on timestamp to the
regulator device and updates single and bulk enable APIs to accept a wait time.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 1797929dfe566..d14ce86d8f7bb 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c

[ ... ]

> @@ -3159,13 +3162,24 @@ 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 {
> +			/* regulator already enabled somehow, but timestamp might be invalid */
> +			if (!rdev->last_on)
> +				rdev->last_on = ktime_get_boottime();
>  		}
> -		/* Fallthrough on positive return values - already enabled */
>  	}
>  
>  	if (regulator->enable_count == 1)
>  		rdev->use_count++;
>  
> +	if (wait_us) {
> +		ktime_t end = ktime_add_us(rdev->last_on, wait_us);
> +		s64 remaining = ktime_us_delta(end, ktime_get_boottime());
> +
> +		if (remaining > 0)
> +			fsleep(remaining);
> +	}
> +

[Severity: High]
Does sleeping here while holding the recursive ww_mutex for the regulator and
its parent supply chain cause concurrency issues?

Since regulator_enable_and_wait() acquires these locks via
regulator_lock_dependent() before calling _regulator_enable_and_wait(), this
sleep happens while the locks are held.

In cases like regulator_bulk_enable_and_wait() where async workers are created
to enable regulators in parallel, won't this serialize the wait times for any
consumers sharing a parent supply, defeating the purpose of the async design?

>  	return 0;
>  
>  err_consumer_disable:

[ ... ]

> diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
> index 56fe2693d9b22..a69157c9b5b51 100644
> --- a/include/linux/regulator/consumer.h
> +++ b/include/linux/regulator/consumer.h

[ ... ]

> @@ -192,7 +193,7 @@ int devm_regulator_bulk_register_supply_alias(struct device *dev,
>  					      int num_id);
>  
>  /* regulator output control and status */
> -int __must_check regulator_enable(struct regulator *regulator);
> +int __must_check regulator_enable_and_wait(struct regulator *regulator, unsigned int ms);

[Severity: High]
Could the naming of this parameter cause consumers to inadvertently pass
millisecond values that get interpreted as microseconds?

The prototype here defines the parameter as ms, but the implementation and
the parameter passed to ktime_add_us() inside _regulator_enable_and_wait()
treat it strictly as microseconds.

If a driver developer passes 10 expecting a 10-millisecond delay based on this
header, the core will interpret it as 10 microseconds. Could this lead to
hardware initialization failures due to premature access to unstable power
supplies?

>  int regulator_disable(struct regulator *regulator);
>  int regulator_force_disable(struct regulator *regulator);
>  int regulator_is_enabled(struct regulator *regulator);

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