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

[Chen-Yu Tsai]

On Tue, Jul 21, 2026 at 4:08 PM <sashiko-bot at kernel.org> wrote:
>
> 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 at 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?

I suppose the fsleep() needs to be moved outside the lock region.

So instead of changing _regulator_enable(), make it return the last_on
timestamp, and then act on it in regulator_enable_and_wait() after the
lock is released.

> >       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?

Yes I missed this part when I changed the API to use microseconds
instead of milliseconds. Will fix in the next version.


ChenYu

> 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