Re: [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of `regulator_is_enabled()` is implicitly cast to `bool`, causing negative error codes to evaluate as true and incorrectly bypassing the required power-on delay.
- [High] Skipping the power-on delay for shared regulators is susceptible to unsafe power domain assumptions and TOCTOU races, leading to bypassed hardware initialization.
--

commit 62ef124436c7ca613cd0f8d922094f42cdd2a0b8
Author: Chen-Yu Tsai <wenst@chromium.org>

Input: elan_i2c - Wait for initialization after enabling regulator supply

This commit adds proper delays after enabling the regulator supply for Elan
trackpad controllers, which require time for hardware and firmware to
initialize. It attempts to avoid boot time impact by skipping the delay if
the regulator was already enabled.

> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index fee1796da3d08..110c662cef96f 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c

[ ... ]

> @@ -1187,6 +1189,7 @@ static int elan_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct elan_tp_data *data;
>  	unsigned long irqflags;
> +	bool supply_was_enabled;
>  	int error;
>  
>  	if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
> @@ -1218,6 +1221,8 @@ static int elan_probe(struct i2c_client *client)
>  	if (IS_ERR(data->vcc))
>  		return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
>  
> +	supply_was_enabled = regulator_is_enabled(data->vcc);

[Severity: High]
In elan_probe(), regulator_is_enabled() returns an int, which can include
negative error codes like -EPROBE_DEFER or -EREMOTEIO. Will assigning this to
the bool supply_was_enabled cause negative error codes to evaluate as true?

If this happens, it looks like it would incorrectly bypass the power-on delay
and lead to a permanent -ENXIO probe failure later rather than allowing a
proper deferral.

> +
>  	error = regulator_enable(data->vcc);
>  	if (error) {
>  		dev_err(dev, "Failed to enable regulator: %d\n", error);
> @@ -1231,6 +1236,9 @@ static int elan_probe(struct i2c_client *client)
>  		return error;
>  	}
>  
> +	if (!supply_was_enabled)
> +		msleep(ETP_POWER_ON_DELAY);

[Severity: High]
Is it safe to assume that a regulator that is already enabled has been powered
on for a sufficient amount of time?

If the trackpad shares a regulator with another device (like a touchscreen)
that just finished its own probe milliseconds earlier, the regulator would
already be enabled, but the trackpad hardware wouldn't be fully initialized.

Could this cause elan_probe() to skip the required hardware initialization
delay and fail the subsequent i2c_smbus_read_byte() check?

[ ... ]

> @@ -1374,11 +1382,16 @@ static int elan_resume(struct device *dev)
>  	int error;
>  
>  	if (!device_may_wakeup(dev)) {
> +		bool supply_was_enabled = regulator_is_enabled(data->vcc);

[Severity: High]
In elan_resume(), could the same boolean cast issue happen if
regulator_is_enabled() encounters a transient failure during system resume,
causing the resume delay to be skipped?

> +
>  		error = regulator_enable(data->vcc);
>  		if (error) {
>  			dev_err(dev, "error %d enabling regulator\n", error);
>  			goto err;
>  		}
> +
> +		if (!supply_was_enabled)
> +			msleep(ETP_POWER_ON_DELAY);

[Severity: High]
Could the same shared-regulator timing race occur during the system resume
path, skipping the initialization delay if another device on the same shared
power domain resumed just before the trackpad?

>  	}
>  
>  	error = elan_set_power(data, true);

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