Skip to contentSkip to conversation

Input: s6sy761 - fix error handling in power management paths

PATCH In review Dmitry Torokhov 2 participants 3 messages Sep 30

Subsystems (2)
HID Input drivers
lore
Share & tools
lore

Dmitry Torokhov

author maintainer
#0 lore raw

PATCH 1/1 Input: s6sy761 - fix error handling in power management paths

In s6sy761_power_on(), if any of the I2C operations or status checks fail after enabling the regulators, the function returns an error while leaving the regulators enabled. Conversely, in s6sy761_resume(), if powering on the controller fails, enable_irq() is skipped while the PM core still marks the device as resumed. A subsequent system suspend or driver unbind then invokes s6sy761_power_off() again, attempting to disable already disabled regulators and incrementing the IRQ disable depth a second time, which leaves the interrupt permanently disabled on later resumes. Additionally, if restoring sensing in s6sy761_resume() fails, the controller is left powered on.

Track whether the controller is powered up with a powered_up flag, unwind regulator enablement on error in s6sy761_power_on(), move disable_irq() to s6sy761_suspend() so it is only called during system sleep when the controller is powered up, and power off the controller if restoring sensing fails during resume.

Reported-by [email protected]
Assisted-by LLM
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/input/touchscreen/s6sy761.c | 59 +++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 16 deletions(-)
drivers/input/touchscreen/s6sy761.c +43 -16
index 899c179fb2d3..a770628d2b94 100644
@@ -108,6 +108,8 @@ struct s6sy761_data {
 
 	u16 devid;
 	u8 tx_channel;
+
+	bool powered_up;
 };
 
 /*

@@ -307,32 +309,41 @@ static int s6sy761_power_on(struct s6sy761_data *sdata)
 					    S6SY761_EVENT_SIZE,
 					    buffer);
 	if (ret < 0)
-		return ret;
+		goto err_disable_regulators;
 
 	event = (buffer[0] >> 2) & 0xf;
 
 	if ((event != S6SY761_EVENT_INFO &&
 	     event != S6SY761_EVENT_VENDOR_INFO) ||
 	    buffer[1] != S6SY761_INFO_BOOT_COMPLETE) {
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err_disable_regulators;
 	}
 
 	ret = i2c_smbus_read_byte_data(sdata->client, S6SY761_BOOT_STATUS);
 	if (ret < 0)
-		return ret;
+		goto err_disable_regulators;
 
 	/* for some reasons the device might be stuck in the bootloader */
-	if (ret != S6SY761_BS_APPLICATION)
-		return -ENODEV;
+	if (ret != S6SY761_BS_APPLICATION) {
+		ret = -ENODEV;
+		goto err_disable_regulators;
+	}
 
 	/* enable touch functionality */
 	ret = i2c_smbus_write_word_data(sdata->client,
 					S6SY761_TOUCH_FUNCTION,
 					S6SY761_MASK_TOUCH);
 	if (ret)
-		return ret;
+		goto err_disable_regulators;
 
+	sdata->powered_up = true;
 	return 0;
+
+err_disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(sdata->regulators),
+			       sdata->regulators);
+	return ret;
 }
 
 static int s6sy761_hw_init(struct s6sy761_data *sdata,

@@ -381,9 +392,11 @@ static void s6sy761_power_off(void *data)
 {
 	struct s6sy761_data *sdata = data;
 
-	disable_irq(sdata->client->irq);
-	regulator_bulk_disable(ARRAY_SIZE(sdata->regulators),
-						sdata->regulators);
+	if (!sdata->powered_up)
+		return;
+
+	regulator_bulk_disable(ARRAY_SIZE(sdata->regulators), sdata->regulators);
+	sdata->powered_up = false;
 }
 
 static int s6sy761_probe(struct i2c_client *client)

@@ -492,11 +505,24 @@ static int s6sy761_suspend(struct device *dev)
 {
 	struct s6sy761_data *sdata = dev_get_drvdata(dev);
 
-	s6sy761_power_off(sdata);
+	if (sdata->powered_up) {
+		disable_irq(sdata->client->irq);
+		s6sy761_power_off(sdata);
+	}
 
 	return 0;
 }
 
+static int s6sy761_restore_sense(struct s6sy761_data *sdata)
+{
+	guard(mutex)(&sdata->input->mutex);
+
+	if (!input_device_enabled(sdata->input))
+		return 0;
+
+	return i2c_smbus_write_byte(sdata->client, S6SY761_SENSE_ON);
+}
+
 static int s6sy761_resume(struct device *dev)
 {
 	struct s6sy761_data *sdata = dev_get_drvdata(dev);

@@ -506,15 +532,16 @@ static int s6sy761_resume(struct device *dev)
 	if (err)
 		return err;
 
-	enable_irq(sdata->client->irq);
-
-	guard(mutex)(&sdata->input->mutex);
+	err = s6sy761_restore_sense(sdata);
+	if (err)
+		goto err_power_off;
 
-	if (!input_device_enabled(sdata->input))
-		return 0;
+	enable_irq(sdata->client->irq);
 
-	err = i2c_smbus_write_byte(sdata->client, S6SY761_SENSE_ON);
+	return 0;
 
+err_power_off:
+	s6sy761_power_off(sdata);
 	return err;
 }
Signature · 5 lines
2.56.0.rc1.315.gc6ed9934b7-goog


-- 
Dmitry
#1 lore raw
Quoted 17 lines · Dmitry Torokhov

In s6sy761_power_on(), if any of the I2C operations or status checks fail after enabling the regulators, the function returns an error while leaving the regulators enabled. Conversely, in s6sy761_resume(), if powering on the controller fails, enable_irq() is skipped while the PM core still marks the device as resumed. A subsequent system suspend or driver unbind then invokes s6sy761_power_off() again, attempting to disable already disabled regulators and incrementing the IRQ disable depth a second time, which leaves the interrupt permanently disabled on later resumes. Additionally, if restoring sensing in s6sy761_resume() fails, the controller is left powered on.

Track whether the controller is powered up with a powered_up flag, unwind regulator enablement on error in s6sy761_power_on(), move disable_irq() to s6sy761_suspend() so it is only called during system sleep when the controller is powered up, and power off the controller if restoring sensing fails during resume.

Quoted 7 lines
Reported-by [email protected]
Assisted-by LLM
Signed-off-by Dmitry Torokhov <[email protected]>
---
  drivers/input/touchscreen/s6sy761.c | 59 +++++++++++++++++++++--------
  1 file changed, 43 insertions(+), 16 deletions(-)

+<<<<<<< HEAD

        return s6sy761_power_on(sdata);
+=======
+       return 0;

+
+err_power_off:
+ s6sy761_power_off(sdata);
+ return err;
+>>>>>>> Input: s6sy761 - fix error handling in power management paths

doesn't apply cleanly on linux-next, against which branch was it build? (I don't see anything touch it in input)

Thanks
David

author maintainer
#2 lore raw
Quoted 36 lines · David Heidelberg
Quoted 17 lines · Dmitry Torokhov

In s6sy761_power_on(), if any of the I2C operations or status checks fail after enabling the regulators, the function returns an error while leaving the regulators enabled. Conversely, in s6sy761_resume(), if powering on the controller fails, enable_irq() is skipped while the PM core still marks the device as resumed. A subsequent system suspend or driver unbind then invokes s6sy761_power_off() again, attempting to disable already disabled regulators and incrementing the IRQ disable depth a second time, which leaves the interrupt permanently disabled on later resumes. Additionally, if restoring sensing in s6sy761_resume() fails, the controller is left powered on.

Track whether the controller is powered up with a powered_up flag, unwind regulator enablement on error in s6sy761_power_on(), move disable_irq() to s6sy761_suspend() so it is only called during system sleep when the controller is powered up, and power off the controller if restoring sensing fails during resume.

Quoted 7 lines
Reported-by [email protected]
Assisted-by LLM
Signed-off-by Dmitry Torokhov <[email protected]>
---
  drivers/input/touchscreen/s6sy761.c | 59 +++++++++++++++++++++--------
  1 file changed, 43 insertions(+), 16 deletions(-)

+<<<<<<< HEAD

        return s6sy761_power_on(sdata);

+=======

drivers/input/touchscreen/s6sy761.c +6 -0
+       return 0;
+
+err_power_off:
+       s6sy761_power_off(sdata);
+       return err;
+>>>>>>> Input: s6sy761 - fix error handling in power management paths
Quoted 2 lines

doesn't apply cleanly on linux-next, against which branch was it build? (I don't see anything touch it in input)

It's on top of your change, I just pushed my queue out, should be in next next.

Thanks.

Signature · 1 line
Dmitry