[PATCH] HID: i2c-hid: Call device suspend callback before disabling irq

[PATCH] HID: i2c-hid: Call device suspend callback before disabling irq

[Gabriele Mazzotta]

The irq is most likely required by the suspend callback, so disable it
only after the callback had been executed.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index f77469d..9ed69b5 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -1092,13 +1092,13 @@ static int i2c_hid_suspend(struct device *dev)
 	struct hid_device *hid = ihid->hid;
 	int ret = 0;
 
+	if (hid->driver && hid->driver->suspend)
+		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
+
 	disable_irq(ihid->irq);
 	if (device_may_wakeup(&client->dev))
 		enable_irq_wake(ihid->irq);
 
-	if (hid->driver && hid->driver->suspend)
-		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
-
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
 
-- 
2.1.4

Re: [PATCH] HID: i2c-hid: Call device suspend callback before disabling irq

[Benjamin Tissoires]

On Jul 07 2015 or thereabouts, Gabriele Mazzotta wrote:
> The irq is most likely required by the suspend callback, so disable it
> only after the callback had been executed.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---

Just in case Jiri missed it:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks!

Benjamin

>  drivers/hid/i2c-hid/i2c-hid.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index f77469d..9ed69b5 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -1092,13 +1092,13 @@ static int i2c_hid_suspend(struct device *dev)
>  	struct hid_device *hid = ihid->hid;
>  	int ret = 0;
>  
> +	if (hid->driver && hid->driver->suspend)
> +		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
> +
>  	disable_irq(ihid->irq);
>  	if (device_may_wakeup(&client->dev))
>  		enable_irq_wake(ihid->irq);
>  
> -	if (hid->driver && hid->driver->suspend)
> -		ret = hid->driver->suspend(hid, PMSG_SUSPEND);
> -
>  	/* Save some power */
>  	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>  
> -- 
> 2.1.4
> 

Re: [PATCH] HID: i2c-hid: Call device suspend callback before disabling irq

[Jiri Kosina]

On Tue, 7 Jul 2015, Gabriele Mazzotta wrote:

> The irq is most likely required by the suspend callback, so disable it
> only after the callback had been executed.

It would be nice to have a more verbose changelog here -- i.e. why we want 
to do such change and what could go wrong if IRQ is disabled too early 
(i.e. what bug is this exactly fixing).

> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>

I'd see this as a 4.2 material still. Please let me know if you'd rather 
want to wait for 4.3.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: i2c-hid: Call device suspend callback before disabling irq

[Gabriele Mazzotta]

On Tuesday 07 July 2015 22:49:30 Jiri Kosina wrote:
> On Tue, 7 Jul 2015, Gabriele Mazzotta wrote:
> 
> > The irq is most likely required by the suspend callback, so disable it
> > only after the callback had been executed.
> 
> It would be nice to have a more verbose changelog here -- i.e. why we want 
> to do such change and what could go wrong if IRQ is disabled too early 
> (i.e. what bug is this exactly fixing).
> 
> > 
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> 
> I'd see this as a 4.2 material still. Please let me know if you'd rather 
> want to wait for 4.3.
> 
> Thanks,

I would have given an instance showing the problem, but AFAIK there
isn't one yet as there are no drivers doing something with IRQs on
suspend. I encounterd problems while modifying hid-rmi.c, but since
the changes aren't there yet, I couldn't mention them. Still, I could
have written something better.

What about something like the following? I can resend the patch with
the new message if it's OK.
---

i2c-hid takes care of requesting and handling IRQs for HID devices
which in turns might expect them to be always active when working
in normal conditions. Hence, disabling IRQs before calling the suspend
callbacks can potentially cause problems since device drivers might
try to perform operations needing them.

Fix this by disabling IRQs only after the suspend callbacks had been
executed.

Re: [PATCH] HID: i2c-hid: Call device suspend callback before disabling irq

[Jiri Kosina]

On Wed, 8 Jul 2015, Gabriele Mazzotta wrote:

> i2c-hid takes care of requesting and handling IRQs for HID devices
> which in turns might expect them to be always active when working
> in normal conditions. Hence, disabling IRQs before calling the suspend
> callbacks can potentially cause problems since device drivers might
> try to perform operations needing them.
> 
> Fix this by disabling IRQs only after the suspend callbacks had been
> executed.

That's much nicer changelog, thanks. I've now applied the patch with this 
changelog to for-4.3/i2c-hid.

-- 
Jiri Kosina
SUSE Labs