Re: [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on
From: sashiko-bot@kernel.org
Date: 2026-07-03 12:07:19
Also in:
linux-devicetree
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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