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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device loses custom configuration and DT-provided calibration data after system suspend because power is cut and registers are not restored upon resume.
- [High] Unbalanced regulator disable when driver is unbound after a resume failure.
- [High] I2C register access during suspend and close fails because the enable GPIO is not asserted when no effects have been played, causing system suspend to abort.
--

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

    Input: drv260x: Fix suspend and resume sequencing
    
This commit addresses race conditions between the asynchronous force-feedback
playback worker and system suspend. It manually stops the worker and sequences
the regulator, GPIO, and standby mode transitions, attempting to gracefully
unwind on failure.

Link: https://lore.kernel.org/linux-input/20260829230740.126461F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/20260830143050.03E081F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-input/apLD91vzHIrLOPWC@google.com/

> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index 6c5c4c53753b1..f261bb32fdc08 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c

[ ... ]

> @@ -569,18 +593,22 @@ static int drv260x_probe(struct i2c_client *client)
>  static int drv260x_suspend(struct device *dev)
>  {
>  	struct drv260x_data *haptics = dev_get_drvdata(dev);
> -	int error;
> +	bool restore_work = false;
> +	int error, restore_error;
>  
> -	guard(mutex)(&haptics->input_dev->mutex);
> +	mutex_lock(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> +		restore_work = !haptics->work_disabled;
> +		drv260x_disable_work(haptics);
> +
>  		error = regmap_update_bits(haptics->regmap,
>  					   DRV260X_MODE,
>  					   DRV260X_STANDBY_MASK,
>  					   DRV260X_STANDBY);

[Severity: High]
This is a pre-existing issue, but will this I2C register access fail if the
enable GPIO is not asserted?

If a user opens and then closes the device, drv260x_close() sets the enable
GPIO to 0. Because there is no open() callback, reopening the device leaves
the GPIO at 0. If the system then suspends before any effects are played, the
chip is disabled.

Will the resulting I2C NACK cause regmap_update_bits() to return an error and
abort the entire system suspend?

>  		if (error) {
>  			dev_err(dev, "Failed to set standby mode\n");
> -			return error;
> +			goto err_enable_work;
>  		}

[ ... ]

> @@ -603,28 +645,40 @@ static int drv260x_resume(struct device *dev)
>  	struct drv260x_data *haptics = dev_get_drvdata(dev);
>  	int error;
>  
> -	guard(mutex)(&haptics->input_dev->mutex);
> +	mutex_lock(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> +		drv260x_disable_work(haptics);
> +
>  		error = regulator_enable(haptics->regulator);
>  		if (error) {
>  			dev_err(dev, "Failed to enable regulator\n");
> -			return error;
> +			goto err_unlock;
>  		}
>  
> +		gpiod_set_value(haptics->enable_gpio, 1);
> +		fsleep(250);
> +
>  		error = regmap_update_bits(haptics->regmap,
>  					   DRV260X_MODE,
>  					   DRV260X_STANDBY_MASK, 0);

[Severity: High]
This isn't a bug introduced by this patch, but does the device lose its custom
configuration here?

Because the regulator was disabled during suspend, power to the device was cut
and the registers were wiped. Since the driver uses REGCACHE_NONE for its
regmap configuration, clearing the standby mode won't automatically reapply the
device settings.

Are critical initialization settings like rated voltage, overdrive voltage, and
calibration data lost after a suspend/resume cycle?

>  		if (error) {
> -			dev_err(dev, "Failed to unset standby mode\n");
> -			regulator_disable(haptics->regulator);
> -			return error;
> +			dev_err(dev, "Failed to leave standby mode: %d\n", error);
> +			goto err_disable_regulator;
>  		}
>  
> -		gpiod_set_value(haptics->enable_gpio, 1);
> +		drv260x_enable_work(haptics);
>  	}
>  
> +	mutex_unlock(&haptics->input_dev->mutex);
>  	return 0;
> +
> +err_disable_regulator:
> +	gpiod_set_value(haptics->enable_gpio, 0);
> +	regulator_disable(haptics->regulator);
> +err_unlock:
> +	mutex_unlock(&haptics->input_dev->mutex);
> +	return error;
>  }

[Severity: High]
This is a pre-existing issue, but does this error unwinding break the invariant
expected by devm cleanup?

If regulator_enable() fails (goto err_unlock) or if regmap_update_bits() fails
(goto err_disable_regulator), drv260x_resume() exits with the regulator count
at 0.

If the device is subsequently unbound after this resume failure (for example,
the driver module is removed), the devm cleanup action unconditionally calls
regulator_disable() again.

Will this cause an unbalanced regulator disable, triggering a kernel WARN and
prematurely cutting power to other hardware sharing the same regulator?

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