Re: [PATCH] Input: drv260x - switch to using fsleep()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] I2C communication failure in drv260x_resume due to incorrect GPIO sequence.
- [High] Lack of synchronization between haptic workqueue and system suspend leads to I2C transfers to a powered-off device.
- [Medium] Incorrect timeout handling in drv260x_init loop may cause spurious calibration failures.
--

commit 00b4c6fa5b7a9a1b697cc946bec3e6c6ae2eff9c
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

    Input: drv260x - switch to using fsleep()
    
    Switch from udelay() to fsleep() in drv260x_worker() as it runs in a
    sleepable workqueue context where busy-waiting is unnecessary.

> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index 6c5c4c53753b1..477ec84ea09b8 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
> @@ -222,7 +222,7 @@ static void drv260x_worker(struct work_struct *work)
>  
>  	gpiod_set_value(haptics->enable_gpio, 1);
>  	/* Data sheet says to wait 250us before trying to communicate */
> -	udelay(250);
> +	fsleep(250);

[Severity: High]
This is a pre-existing issue, but does switching to fsleep() widen the race
window between drv260x_worker() and system suspend?

If a haptic effect is playing during system suspend, drv260x_suspend() disables
the regulator without canceling haptics->work:

drivers/input/misc/drv260x.c:drv260x_suspend() {
    ...
	gpiod_set_value(haptics->enable_gpio, 0);

	error = regulator_disable(haptics->regulator);
    ...
}

Since fsleep() voluntarily yields the CPU, could this allow the suspend thread
to run and power down the device, resulting in regmap_write() attempting I2C
transfers to a powered-off chip when the worker resumes?

>  
>  	error = regmap_write(haptics->regmap,
>  			     DRV260X_MODE, DRV260X_RT_PLAYBACK);

[Severity: High]
This isn't a bug introduced by this patch, but can I2C communication fail in
drv260x_resume() due to the GPIO enable sequence?

In drv260x_resume(), regmap_update_bits() is called before setting the
enable_gpio pin high:

drivers/input/misc/drv260x.c:drv260x_resume() {
    ...
		error = regmap_update_bits(haptics->regmap,
					   DRV260X_MODE,
					   DRV260X_STANDBY_MASK, 0);
		if (error) {
			dev_err(dev, "Failed to unset standby mode\n");
			regulator_disable(haptics->regulator);
			return error;
		}

		gpiod_set_value(haptics->enable_gpio, 1);
    ...
}

Since the hardware disables the I2C interface when the EN pin is low, won't
this transaction NACK and cause the resume sequence to abort?

[Severity: Medium]
This isn't a bug introduced by this patch, but could the timeout handling in
drv260x_init() cause spurious calibration failures?

In drv260x_init(), the loop checks for a timeout unconditionally after reading
the register:

drivers/input/misc/drv260x.c:drv260x_init() {
    ...
		if (time_after(jiffies, timeout)) {
			dev_err(&haptics->client->dev,
				"Calibration timeout. The device cannot be used.\n");
			return -ETIMEDOUT;
		}
	} while (cal_buf == DRV260X_GO_BIT);
    ...
}

If the timeout expires on the exact same iteration that the calibration
successfully completes (the GO bit is cleared), will this erroneously return
-ETIMEDOUT instead of successfully exiting the loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/apLOXaWFZ8ueIEAm@google.com?part=1