Re: [PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The integer return value of `regulator_is_enabled()` (which can be a negative error code) is implicitly cast to or bitwise-ANDed with `bool` variables, leading to incorrect evaluations that inadvertently skip required hardware power-on delays.
- [Medium] Shared regulator logic flaw and TOCTOU race allow skipping required power-on delays.

Pre-existing issues:
- [High] Optional regulator supplies missing from the device tree incorrectly cause the prober to fail, contradicting the intended optional behavior.
--

commit 7072efff03a9502c14f9b2eb5a3863213e3992ef
Author: Chen-Yu Tsai <wenst@chromium.org>

i2c: of-prober: skip post-power-on delay if already powered on

This commit modifies the I2C OF prober to check if regulator supplies are
already enabled before applying a post-power-on delay. If the supply is
already active, it skips the delay to avoid unnecessary boot time penalties.

> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index 6a82b03809d4f..f274e260353cd 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c

[ ... ]

> @@ -219,19 +220,25 @@ static void i2c_of_probe_simple_put_supply(struct i2c_of_probe_simple_ctx *ctx)
>  
>  static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
>  {
> +	bool supply_was_on;
>  	int ret;
>  
>  	if (!ctx->supply)
>  		return 0;
>  
> -	dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name);
> +	supply_was_on = regulator_is_enabled(ctx->supply);

[Severity: High]
Could this introduce a regression by skipping required hardware power-on
delays if regulator_is_enabled() fails?

The integer return value of regulator_is_enabled() can be a negative error
code, but it is implicitly cast to the bool variable supply_was_on without
error checking. Any non-zero integer, including negative values, becomes true.

If regulator_is_enabled() returns an error, the code will consider the supply
as already on, causing the device to skip the mandatory post-power-on delay
and fail to probe because it has not finished initializing.

I noticed similar patterns in other drivers:
- drivers/hid/i2c-hid/i2c-hid-of.c:i2c_hid_of_power_up()
- drivers/input/mouse/elan_i2c_core.c:elan_probe()
- drivers/input/mouse/elan_i2c_core.c:elan_resume()

[Severity: Medium]
Is there a potential regression here regarding a shared regulator TOCTOU race
condition?

If two devices share a regulator and probe concurrently, Driver A might enable
the regulator. When Driver B checks regulator_is_enabled(), it returns true.
Driver B will then skip its required power-on delay and attempt to communicate
with the device.

If Driver A enabled the regulator less than the required delay time ago,
Driver B will access its device before the hardware has initialized, leading
to probe failure or I2C communication errors.

> +
> +	dev_dbg(dev, "Enabling regulator supply \"%s\" (was %s)\n", ctx->opts->supply_name,
> +		str_on_off(supply_was_on));
>  
>  	ret = regulator_enable(ctx->supply);
>  	if (ret)
>  		return ret;
>  
> -	if (ctx->opts->post_power_on_delay_ms)
> +	if (!supply_was_on && ctx->opts->post_power_on_delay_ms) {
> +		dev_dbg(dev, "Waiting after enabling regulator\n");
>  		msleep(ctx->opts->post_power_on_delay_ms);
> +	}
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed optional regulator supplies missing
from the device tree incorrectly cause the prober to fail, contradicting the
intended optional behavior.

drivers/i2c/i2c-core-of-prober.c:i2c_of_probe_simple_get_supply() {
    ...
	supply = of_regulator_get_optional(dev, node, supply_name);
	if (IS_ERR(supply)) {
		return dev_err_probe(dev, PTR_ERR(supply),
				     "Failed to get regulator supply \"%s\" from %pOF\n",
				     supply_name, node);
	}
    ...
}

of_regulator_get_optional() returns -ENODEV when the supply is not in the
device tree. The function checks IS_ERR(supply) and propagates the error,
failing the probe unconditionally on hardware platforms where the device tree
omits the optional regulator.

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