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

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 [off-list ref]

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.

[ ... ]

[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