Re: [PATCH v6] Input: drv260x: Fix suspend and resume sequencing

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `work_disabled` flag becomes permanently wedged to `true` if the device is closed after a system resume failure, breaking force-feedback functionality on subsequent opens.
- [Low] Direct return of error code from regmap_bulk_write in a function with multiple failure points.

Pre-existing issues:
- [High] Device configuration is permanently lost if system power is cut during suspend while the input device is closed, leading to hardware misconfiguration upon the next open.
- [High] Uncancelled work item leads to Use-After-Free during device unbind due to asynchronous ff-memless timer.
--

commit 0cbbdf5958f6ed62525dabc219437a115afe42b3
Author: Maurizio Casciano <mauriziocasciano7@gmail.com>

Input: drv260x: Fix suspend and resume sequencing

Force-feedback playback is queued asynchronously, but system suspend can
cut power while the worker is pending. Disable and drain the work item
before entering standby, and keep force-feedback quiesced until resume has
restored communication.

Link: https://lore.kernel.org/linux-input/20260831011526.24AB11F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/20260830143050.03E081F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/20260829230740.126461F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/apLD91vzHIrLOPWC@google.com/
Link: https://www.ti.com/lit/ds/symlink/drv2604.pdf

> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
[ ... ]
> @@ -258,11 +282,29 @@ static int drv260x_haptics_play(struct input_dev *input, void *data,
>  	return 0;
>  }
>  
> +static int drv260x_open(struct input_dev *input)
> +{
> +	struct drv260x_data *haptics = input_get_drvdata(input);
> +
> +	if (haptics->work_disabled)
> +		return -EIO;
> +
> +	gpiod_set_value(haptics->enable_gpio, 1);
> +	/* Data sheet says to wait 250us before trying to communicate */
> +	fsleep(250);
> +
> +	return 0;
> +}

[Severity: High]
This is a pre-existing issue, but will device configuration be permanently
lost if system power is cut during suspend while the input device is closed?

During system suspend, VDD power can be cut. Because drv260x_resume() checks
input_device_enabled(), it skips drv260x_init() if the device was closed.
When userspace later opens the device, drv260x_open() asserts the enable
GPIO but does not reinitialize the hardware registers, leaving the device
in an erratic default state.

>  static void drv260x_close(struct input_dev *input)
>  {
>  	struct drv260x_data *haptics = input_get_drvdata(input);
>  	int error;
>  
> +	/* PM has not restored register access yet. */
> +	if (haptics->work_disabled)
> +		return;

[Severity: High]
Could this early return leave the device permanently wedged if it is closed
after a system resume failure?

If drv260x_resume() fails, it leaves work_disabled = true. If userspace
then closes the device, this early return skips clearing the wedge state. On
the next suspend/resume cycle, drv260x_resume() skips recovery entirely
because input_device_enabled() evaluates to false (since users is 0). The
work_disabled flag would remain stuck to true, returning -EIO on all future
opens.

>  
>  	cancel_work_sync(&haptics->work);

[Severity: High]
This is a pre-existing issue, but does relying on close() for cancellation
risk a use-after-free during device unbind?

When the device is unbound, input_unregister_device() calls drv260x_close()
and then stops the background ff-memless timer. If the asynchronous timer
fires in that window, it can reschedule the work item after
cancel_work_sync() has already returned. Because the work item lacks a
registered devm cleanup action, it remains pending and could execute after
the memory has been freed.

[ ... ]
> @@ -398,6 +440,11 @@ static int drv260x_init(struct drv260x_data *haptics)
>  		return 0;
>  	}
>  
> +	if (haptics->calibration_valid)
> +		return regmap_bulk_write(haptics->regmap, DRV260X_CAL_COMP,
> +					 haptics->calibration_data,
> +					 ARRAY_SIZE(haptics->calibration_data));
> +

[Severity: Low]
Does returning the error code from regmap_bulk_write() directly violate the
explicit failure paths coding style?

The input subsystem maintainer guidelines prefer the expanded error checking
form instead of returning the action directly in functions with multiple
failure points.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831081227.1794986-1-mauriziocasciano7@gmail.com?part=1