[PATCH] HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

[PATCH] HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

[Andrew Duggan]

On at least one platform the kernel prints a warning on resume saying
"Unbalanced IRQ 37 wake disable". This is because enable_irq_wake in the
suspend function is failing. This patch checks to see if the call to
enable_irq_wake succeeded before calling disable_irq_wake.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
This patch based on Jiri's for-4.3/i2c-hid and needs Gabriele Mazzotta's
patch which moves where the HID driver's suspend callback call to apply
cleanly.

 drivers/hid/i2c-hid/i2c-hid.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9ed69b5..1605fbe 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -149,6 +149,8 @@ struct i2c_hid {
 	int			irq;
 
 	struct i2c_hid_platform_data pdata;
+
+	bool			irq_wake_enabled;
 };
 
 static int __i2c_hid_command(struct i2c_client *client,
@@ -1096,8 +1098,8 @@ static int i2c_hid_suspend(struct device *dev)
 		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
 
 	disable_irq(ihid->irq);
-	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(ihid->irq);
+	if (device_may_wakeup(&client->dev) && !enable_irq_wake(ihid->irq))
+		ihid->irq_wake_enabled = true;
 
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
@@ -1117,8 +1119,10 @@ static int i2c_hid_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (device_may_wakeup(&client->dev))
+	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
 		disable_irq_wake(ihid->irq);
+		ihid->irq_wake_enabled = false;
+	}
 
 	if (hid->driver && hid->driver->reset_resume) {
 		ret = hid->driver->reset_resume(hid);
-- 
2.1.4

[PATCH] HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

[Andrew Duggan]

On at least one platform the kernel prints a warning on resume saying
"Unbalanced IRQ 37 wake disable". This is because enable_irq_wake in the
suspend function is failing. This patch checks to see if the call to
enable_irq_wake succeeded before calling disable_irq_wake.

Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
This patch based on Jiri's for-4.3/i2c-hid and needs Gabriele Mazzotta's
patch which moves where the HID driver's suspend callback call to apply
cleanly.

 drivers/hid/i2c-hid/i2c-hid.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9ed69b5..1605fbe 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -149,6 +149,8 @@ struct i2c_hid {
 	int			irq;
 
 	struct i2c_hid_platform_data pdata;
+
+	bool			irq_wake_enabled;
 };
 
 static int __i2c_hid_command(struct i2c_client *client,
@@ -1096,8 +1098,8 @@ static int i2c_hid_suspend(struct device *dev)
 		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
 
 	disable_irq(ihid->irq);
-	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(ihid->irq);
+	if (device_may_wakeup(&client->dev) && !enable_irq_wake(ihid->irq))
+		ihid->irq_wake_enabled = true;
 
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
@@ -1117,8 +1119,10 @@ static int i2c_hid_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (device_may_wakeup(&client->dev))
+	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
 		disable_irq_wake(ihid->irq);
+		ihid->irq_wake_enabled = false;
+	}
 
 	if (hid->driver && hid->driver->reset_resume) {
 		ret = hid->driver->reset_resume(hid);
-- 
2.1.4

Re: [PATCH] HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

[Mika Westerberg]

On Mon, Jul 27, 2015 at 05:08:06PM -0700, Andrew Duggan wrote:
> On at least one platform the kernel prints a warning on resume saying
> "Unbalanced IRQ 37 wake disable". This is because enable_irq_wake in the
> suspend function is failing. This patch checks to see if the call to
> enable_irq_wake succeeded before calling disable_irq_wake.

While this looks okay, I'm wondering if this can also be fixed from the
irq chip side by setting IRQCHIP_SKIP_SET_WAKE to .flags, given that the
driver does not provide such function?

Re: [PATCH] HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

[Andrew Duggan]

On 07/28/2015 04:33 AM, Mika Westerberg wrote:
> On Mon, Jul 27, 2015 at 05:08:06PM -0700, Andrew Duggan wrote:
>> On at least one platform the kernel prints a warning on resume saying
>> "Unbalanced IRQ 37 wake disable". This is because enable_irq_wake in the
>> suspend function is failing. This patch checks to see if the call to
>> enable_irq_wake succeeded before calling disable_irq_wake.
> While this looks okay, I'm wondering if this can also be fixed from the
> irq chip side by setting IRQCHIP_SKIP_SET_WAKE to .flags, given that the
> driver does not provide such function?

Yes, setting IRQCHIP_SKIP_SET_WAKE does fix the problem. After looking 
into it a bit more it turns out commit 5613570 already fixed the issue. 
My test system was running an older kernel which didn't include it. 
Applying 5613570 fixed this particular issue. Thanks for pointing that 
out, now I have a better understanding of why enable_irq_wake was 
failing in this instance.

For this patch, I think it is still a good idea to check the return code 
of enable_irq_wake. In theory, an irqchip which does have a irq_set_wake 
callback set could return an error. I can update the commit comment, add 
a warning, and submit a v2.

Thanks,
Andrew

Re: [PATCH] HID: i2c-hid: Only disable irq wake if it was successfully enabled during suspend

[Andrew Duggan]

On 07/28/2015 04:33 AM, Mika Westerberg wrote:
> On Mon, Jul 27, 2015 at 05:08:06PM -0700, Andrew Duggan wrote:
>> On at least one platform the kernel prints a warning on resume saying
>> "Unbalanced IRQ 37 wake disable". This is because enable_irq_wake in the
>> suspend function is failing. This patch checks to see if the call to
>> enable_irq_wake succeeded before calling disable_irq_wake.
> While this looks okay, I'm wondering if this can also be fixed from the
> irq chip side by setting IRQCHIP_SKIP_SET_WAKE to .flags, given that the
> driver does not provide such function?

Yes, setting IRQCHIP_SKIP_SET_WAKE does fix the problem. After looking 
into it a bit more it turns out commit 5613570 already fixed the issue. 
My test system was running an older kernel which didn't include it. 
Applying 5613570 fixed this particular issue. Thanks for pointing that 
out, now I have a better understanding of why enable_irq_wake was 
failing in this instance.

For this patch, I think it is still a good idea to check the return code 
of enable_irq_wake. In theory, an irqchip which does have a irq_set_wake 
callback set could return an error. I can update the commit comment, add 
a warning, and submit a v2.

Thanks,
Andrew