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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The resume error path incorrectly re-enables the haptics worker while the hardware regulator is disabled, exposing the unpowered device to I2C and GPIO writes.

Pre-existing issues:
- [High] The device's calibration and configuration registers are lost across a suspend/resume cycle because they are never restored after the regulator is re-enabled.
--

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

Input: drv260x: Fix suspend and resume sequencing

This commit fixes system suspend handling for the drv260x haptics driver
by explicitly disabling and draining the force-feedback worker before
entering standby, and conditionally enabling it upon resume. It also
introduces explicit mutex locking and goto-based error unwinding.

Link: https://lore.kernel.org/linux-input/20260829230740.126461F000E9@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..b6499b2fcd8df 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
[ ... ]
>  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)) {
>  		error = regulator_enable(haptics->regulator);
>  		if (error) {
>  			dev_err(dev, "Failed to enable regulator\n");
> -			return error;
> +			goto err_enable_work;
>  		}

[Severity: High]
This is a pre-existing issue, but since the regulator was disabled during
suspend, the DRV260x chip loses all internal volatile registers, including
critical calibration data like rated voltage and overdrive clamp.

Because drv260x_regmap_config uses REGCACHE_NONE, regmap will not
automatically restore these values upon resume.

Does this device need a call to drv260x_init() here to restore its
calibration and configuration settings before attempting to operate?

>  
> +		gpiod_set_value(haptics->enable_gpio, 1);
> +		fsleep(250);
> +
>  		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;
> +			dev_err(dev, "Failed to leave standby mode: %d\n", error);
> +			goto err_disable_regulator;
>  		}
>  
> -		gpiod_set_value(haptics->enable_gpio, 1);
> +		enable_work(&haptics->work);
>  	}
>  
> +	mutex_unlock(&haptics->input_dev->mutex);
>  	return 0;
> +
> +err_disable_regulator:
> +	gpiod_set_value(haptics->enable_gpio, 0);
> +	regulator_disable(haptics->regulator);
> +err_enable_work:
> +	enable_work(&haptics->work);

[Severity: High]
If the system resumes but the hardware initialization fails, does this
unconditionally re-enable the haptics worker while the regulator remains
disabled?

If userspace subsequently sends a force-feedback event, drv260x_worker()
will execute and attempt I2C communication with the unpowered chip.
Can this lead to I2C bus timeouts, lockups, or back-powering the chip
via GPIO/I2C lines?

Additionally, if the system later successfully retries and resumes the
device, the success path will call enable_work() again. Since the count
was already erroneously reset to 0 here, won't this underflow the disable
count and trigger a workqueue kernel warning?

> +	mutex_unlock(&haptics->input_dev->mutex);
> +	return error;
>  }

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