Re: [PATCH v5 5/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper

[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() fails to set wait_us for all consumers except the first one, leading to uninitialized memory reads
- [High] _regulator_is_enabled(rdev) is called locklessly in regulator_register() after the device is exposed, causing data races
- [Medium] Unconditional 100ms boot penalty in i2c_of_probe_simple_cleanup when no I2C component is found
--

--- Patch [5]: [PATCH v5 5/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper ---
commit 227ad2dc27d8706adbb141c1fb7f00da090435f9
Author: Chen-Yu Tsai <wenst@chromium.org>
Subject: i2c: of-prober: Defer regulator_disable() on successful probe in simple helper

This commit delays dropping the regulator enable refcount for 100 ms when the
cleanup function is called. This gives asynchronous drivers time to probe
and increase the refcount themselves, avoiding the need to wait for hardware
re-initialization if the regulator was disabled just prior to probing.

> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index f9f3c0ef93ff5..68c929b16b066 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c
[ ... ]
> @@ -235,11 +235,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
>  	return 0;
>  }
>  
> -static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> +static void i2c_of_probe_simple_disable_regulator(struct device *dev,
> +						  struct i2c_of_probe_simple_ctx *ctx,
> +						  bool defer_disable)
>  {
>  	if (!ctx->supply)
>  		return;
>  
> +	/*
> +	 * Wait a bit of time for async drivers to probe and increase the
> +	 * regulator enable count. This allows the drivers to check and
> +	 * skip waiting for re-initialization.
> +	 */
> +	if (defer_disable) {
> +		dev_dbg(dev, "Deferring regulator disable\n");
> +		msleep(100);
> +	}
> +

[Severity: Medium]
Will this unconditionally penalize boot time by 100ms when no I2C component is
found?

If the prober finishes without finding any responding devices, it falls back
to calling the cleanup function. Since i2c_of_probe_simple_cleanup() hardcodes
defer_disable to true, this sleep is triggered even when there is no device
present to probe asynchronously.

[ ... ]
> @@ -401,7 +413,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
>  	i2c_of_probe_simple_disable_gpio(dev, ctx);
>  	i2c_of_probe_simple_put_gpiod(ctx);
>  
> -	i2c_of_probe_simple_disable_regulator(dev, ctx);
> +	i2c_of_probe_simple_disable_regulator(dev, ctx, true);
>  	i2c_of_probe_simple_put_supply(ctx);
>  }

While reviewing this series, I also noticed regressions introduced in
patch 1 of this series.

commit 5fd97a59a5c2607c473a3bc31a58415b701f2335
Author: Chen-Yu Tsai <wenst@chromium.org>
Subject: regulator: core: Add "enable and wait" functions

This patch introduces a "last turned on" timestamp field to the regulator
device and adds "enable and wait" functions to the single and bulk regulator
consumer APIs. The timestamp is updated each time the regulator is actually
turned on, allowing drivers to shorten or skip post-power-on delays.

> 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
[ ... ]
> @@ -5372,30 +5444,38 @@ static void regulator_bulk_enable_async(void *data, async_cookie_t cookie)
>  {
>  	struct regulator_bulk_data *bulk = data;
>  
> -	bulk->ret = regulator_enable(bulk->consumer);
> +	bulk->ret = regulator_enable_and_wait(bulk->consumer, ACCESS_PRIVATE(bulk, wait_us));
>  }
>  
>  /**
> - * regulator_bulk_enable - enable multiple regulator consumers
> + * regulator_bulk_enable_and_wait - enable multiple regulator consumers and
> + *				    wait for time passed after regulators are
> + *				    actually enabled
[ ... ]
> -int regulator_bulk_enable(int num_consumers,
> -			  struct regulator_bulk_data *consumers)
> +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 code fail to set wait_us for all consumers except the first one?

Because consumers is an array pointer, ACCESS_PRIVATE(consumers, wait_us)
expands to consumers[0].wait_us, completely ignoring the loop index i.

This leaves consumers[1..N].wait_us uninitialized. When the async worker later
calls regulator_enable_and_wait(), it will read uninitialized memory. This can
cause long boot delays if the garbage value is large, or hardware
synchronization failures if it happens to be zero.

[ ... ]
> @@ -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();
> +
>  	rdev_init_debugfs(rdev);

[Severity: High]
Can this cause data races or hardware state corruption?

This calls _regulator_is_enabled(rdev) locklessly, and potentially writes to
rdev->last_on, after device_add() has already exposed the device to consumers
and userspace sysfs.

If userspace reads sysfs state attributes concurrently, or a driver calls
regulator_enable(), it could invoke the driver's is_enabled hardware callback
without synchronization.

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