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

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

New issues:
- [High] The workqueue disable count becomes unbalanced if drv260x_resume() fails, permanently disabling haptic playback on subsequent successful resumes.
- [Medium] Mixing goto-based error handling with scope-based guard() cleanup in the same function.

Pre-existing issues:
- [High] The device loses its register configuration during system suspend because the regulator is disabled, but resume fails to reinitialize the registers.
--

commit 38e8103cb03d4a564c39fb41cb33305800090398
Author: Maurizio Casciano [off-list ref]

Input: drv260x: Fix suspend and resume sequencing

This commit updates the suspend and resume routines to correctly drain and
disable the haptics worker before entering standby. It also adds goto-based
error unwinding and uses the enable GPIO to gate I2C access while enforcing
a startup delay.

Link: https://lore.kernel.org/linux-input/apLD91vzHIrLOPWC@google.com/ (local)

[ ... ]

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

[Severity: Medium]
Does this mix goto-based error handling with scope-based guard() cleanup in
the same function?

The kernel cleanup subsystem guidelines explicitly forbid mixing goto and
guard() because it creates confusing ownership semantics. While the lock is
correctly released at the function return in this specific code structure,
the pattern itself is prohibited by subsystem rules.

[ ... ]

 		error = regulator_disable(haptics->regulator);

[Severity: High]
This is a pre-existing issue, but I noticed the regulator is disabled during
suspend, which completely cuts power to the chip and resets its internal
register state.

When the regulator is re-enabled during drv260x_resume(), the driver only
clears the standby bit and fails to reprogram the operational registers like
rated voltage and overdrive. Because the driver uses REGCACHE_NONE and does
not automatically restore register state, does this leave the hardware
unconfigured and functionally broken after a suspend and resume cycle?

[ ... ]

 static int drv260x_resume(struct device *dev)
 {
 	struct drv260x_data *haptics = dev_get_drvdata(dev);
 	int error;
 
 	guard(mutex)(&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;
 		}

[Severity: High]
If regulator_enable() fails here, we return directly without calling
enable_work(&haptics->work).

Since disable_work_sync() was called in drv260x_suspend(), doesn't this leave
the workqueue's disable count unbalanced? The PM core treats resume errors as
non-fatal to the system, so a subsequent suspend will increment the disable
count again to 2. A future successful resume will only decrement it to 1,
leaving the haptics playback permanently disabled.

[ ... ]

 		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;
 		}

[Severity: High]
Similarly, if we take the err_disable_regulator goto path when leaving
standby fails, enable_work() is skipped.

Does this also permanently leak the workqueue disable count?

[Severity: Medium]
Also, just like in drv260x_suspend(), drv260x_resume() introduces a new
goto path while using guard(mutex) at the start of the function.

Could we refactor this to avoid mixing goto labels and scope-based cleanup?

-		gpiod_set_value(haptics->enable_gpio, 1);
+		enable_work(&haptics->work);
 	}

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