Re: [PATCH v5 1/7] 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:
- [High] regulator_bulk_enable_and_wait incorrectly assigns wait_us only to the first consumer, leaving the rest uninitialized.
- [High] regulator_register accesses hardware state via _regulator_is_enabled and updates rdev->last_on without holding rdev->mutex after the device is exposed.
- [Medium] _regulator_is_enabled() and _regulator_is_enabled_recursive() can return negative error codes, but they are used in boolean contexts where negative values evaluate to true.
--

--- Patch [1]: [PATCH v5 1/7] regulator: core: Add "enable and wait" functions ---
commit 5fd97a59a5c2607c473a3bc31a58415b701f2335
Author: Chen-Yu Tsai <wenst@chromium.org>

regulator: core: Add "enable and wait" functions

This patch introduces "enable and wait" functions to the single and bulk
regulator consumer APIs. It tracks a "last turned on" timestamp to shorten
or skip delays if the regulator is already on. The timestamp is also best
effort populated for regulators left on by hardware or firmware.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 1797929dfe566..f70be7e38e599 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2352,6 +2390,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))
> +		rdev->last_on = ktime_get_boottime();
> +

[Severity: Medium]
_regulator_is_enabled_recursive() can return negative error codes. Since it
is evaluated in a boolean context here, won't negative values evaluate to
true, causing last_on to be incorrectly updated if there is a hardware read
failure?

>  	regulator_unlock_dependent(rdev, &ww_ctx);
>  
>  	if (!do_final_setup)

[ ... ]

> @@ -5372,30 +5444,38 @@ static void regulator_bulk_enable_async(void *data, async_cookie_t cookie)
>  int regulator_bulk_enable_and_wait(int num_consumers,
>  				   struct regulator_bulk_data *consumers,
>  				   unsigned int wait_us)
>  {
>  	ASYNC_DOMAIN_EXCLUSIVE(async_domain);
>  	int i;
>  	int ret = 0;
>  
>  	for (i = 0; i < num_consumers; i++) {
> +		ACCESS_PRIVATE(consumers, wait_us) = wait_us;
>  		async_schedule_domain(regulator_bulk_enable_async,
>  				      &consumers[i], &async_domain);
>  	}

[Severity: High]
Does this loop assign wait_us only to the first element of the array instead
of using the loop counter i? For subsequent consumers where i > 0, the
wait_us field remains uninitialized.

When regulator_bulk_enable_async() executes for these subsequent consumers,
could it read uninitialized memory and pass it to fsleep(), potentially
hanging the kernel thread indefinitely or skipping required delays? Should
this be ACCESS_PRIVATE(&consumers[i], wait_us)?

>  	async_synchronize_full_domain(&async_domain);

[ ... ]

> @@ -6225,6 +6305,13 @@ 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.
> +	 */
> +	if (!rdev->supply_name && !rdev->last_on && _regulator_is_enabled(rdev))
> +		rdev->last_on = ktime_get_boottime();
> +

[Severity: Medium]
Similar to the case in regulator_resolve_supply(), _regulator_is_enabled()
can return negative error codes. Could this cause last_on to be updated
incorrectly if the hardware read fails during probe?

[Severity: High]
Since device_add(&rdev->dev) is called earlier in regulator_register(),
exposing the device globally and allowing concurrent regulator_enable()
calls, is it safe to call _regulator_is_enabled(rdev) and update
rdev->last_on here without holding rdev->mutex? This could lead to a data
race on last_on and unsynchronized hardware callbacks.

>  	rdev_init_debugfs(rdev);
>  
>  	/* try to resolve regulators coupling since a new one was registered */

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