Re: [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bitwise AND assignment (`&=`) between a `bool` and the `int` return value of `regulator_is_enabled()` causes pseudo-random evaluation based on the least significant bit (LSB) of the integer, potentially skipping mandatory delays.
- [High] Skipping the post-power-on delay based solely on the current state of `regulator_is_enabled()` introduces a race condition for devices sharing a power rail.
--

commit 14324865d2e3b5ccaf90d6ff025e907befb17313
Author: Chen-Yu Tsai <wenst@chromium.org>

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

This patch checks if the regulator supplies for an i2c-hid device are
already enabled during probe. If all supplies are already on, it skips
the post-power-on delay to save boot time.

> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -46,8 +47,12 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
>  {
>  	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
>  	struct device *dev = &ihid_of->client->dev;
> +	bool supply_was_enabled = true;
>  	int ret;
>  
> +	for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
> +		supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);

[Severity: High]
Could this bitwise operation cause a regression by inadvertently evaluating
the least significant bit of the return value?

The function regulator_is_enabled() returns an int (positive state, 0, or 
a negative error). In C, performing a bitwise AND (&=) between a bool 
(promoted to integer 1) and an int isolates the least significant bit.

For example, if regulator_is_enabled() returns a negative error code like 
-ENODEV (-19), the least significant bit is 1, causing supply_was_enabled 
to evaluate to true despite the error. If it returns an even positive 
state like 2, the least significant bit is 0, making it false.

[ ... ]

>  	if (ret) {
>  		dev_warn(dev, "Failed to enable supplies: %d\n", ret);
>  		return ret;
>  	}
>  
> -	if (ihid_of->post_power_delay_ms)
> +	dev_dbg(dev, "supply was %s.\n", str_on_off(supply_was_enabled));
> +	if (!supply_was_enabled && ihid_of->post_power_delay_ms)
>  		msleep(ihid_of->post_power_delay_ms);

[Severity: High]
Is it safe to skip this delay based solely on whether the regulator is
currently enabled?

If two devices (e.g. a touchscreen and a trackpad) share the same 
non-always-on regulator and probe asynchronously:

Device 1 starts probing, calls regulator_bulk_enable(), and begins
waiting its post-power delay.

Device 2 concurrently probes in i2c_hid_of_power_up(), sees 
regulator_is_enabled() is true, and skips its post_power_delay_ms here.

Device 2 would then immediately issue I2C transactions to a newly powered
device that might not have finished initializing, potentially resulting
in NACKs and a probe regression. 

Does the regulator framework provide a way to ensure the rail has been on
for the required duration, rather than just currently enabled?

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