HID: usbhid: get/put around clearing needs_remote_wakeup

Commit Message

Dmitry Torokhov Sept. 8, 2017, 5:43 p.m. UTC

From: Benson Leung <[e-mail]>

usbhid->intf->needs_remote_wakeup is set when a device is opened, and is
cleared when a device is closed.

In usbhid_open, usb_autopm_get_interface is called before setting the
needs_remote_wakeup flag, and usb_autopm_put_interface is called after
hid_start_in. However, when the device is closed in usbhid_close, we
simply reset the flag and the device stays awake even though it could be
suspended.

This patch adds calls to usb_autopm_get_interface and
usb_autopm_put_interface when we reset usbhid->intf->needs_remote_wakeup
flag, giving the system chance to put the device to sleep.

Signed-off-by: Benson Leung <[e-mail]>
Signed-off-by: Dmitry Torokhov <[e-mail]>
---
 drivers/hid/usbhid/hid-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Oliver Neukum Sept. 9, 2017, 7:35 a.m. UTC | #1

Am Freitag, den 08.09.2017, 10:43 -0700 schrieb Dmitry Torokhov:
> From: Benson Leung <[e-mail]>
> 
> usbhid->intf->needs_remote_wakeup is set when a device is opened, and is
> cleared when a device is closed.
> 
> In usbhid_open, usb_autopm_get_interface is called before setting the
> needs_remote_wakeup flag, and usb_autopm_put_interface is called after
> hid_start_in. However, when the device is closed in usbhid_close, we
> simply reset the flag and the device stays awake even though it could be
> suspended.

Hi,

but if the device is asleep, we do not want to wake it just to reset
the flag. Please use the no resume varieties. They did not exist when this
code was written and that is the reason behind the current code.

As it is your patch does more harm than good.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benson Leung Sept. 11, 2017, 8:02 p.m. UTC | #2

Hi Oliver,

On Sat, Sep 09, 2017 at 09:35:52AM +0200, Oliver Neukum wrote:
> Am Freitag, den 08.09.2017, 10:43 -0700 schrieb Dmitry Torokhov:
> > From: Benson Leung <[e-mail]>
> > 
> > usbhid->intf->needs_remote_wakeup is set when a device is opened, and is
> > cleared when a device is closed.
> > 
> > In usbhid_open, usb_autopm_get_interface is called before setting the
> > needs_remote_wakeup flag, and usb_autopm_put_interface is called after
> > hid_start_in. However, when the device is closed in usbhid_close, we
> > simply reset the flag and the device stays awake even though it could be
> > suspended.
> 
> Hi,
> 
> but if the device is asleep, we do not want to wake it just to reset
> the flag. Please use the no resume varieties. They did not exist when this
> code was written and that is the reason behind the current code.
> 

Thanks for the pointer. I'll respin the patch with the no_resume version of
usb_autopm_get_interface and retest.

Benson

Benson Leung Sept. 11, 2017, 11:29 p.m. UTC | #3

Hi Oliver,

On Mon, Sep 11, 2017 at 01:02:52PM -0700, Benson Leung wrote:
> Thanks for the pointer. I'll respin the patch with the no_resume version of
> usb_autopm_get_interface and retest.
> 

I went and tried this patch but with usb_autopm_get_interface_no_resume instead
and the original bug I was trying to fix with a Yubikey hid security key came
back, so something is still wrong here.

I went ahead and filed a bug to track this here:
https://bugs.chromium.org/p/chromium/issues/detail?id=764112

I'll find some time to dig into this deeper and understand why this device
ends up getting stuck in active instead of suspending when all handles are
closed.

Thanks,
Benson

Dmitry Torokhov Sept. 20, 2017, 1:39 a.m. UTC | #4

On Mon, Sep 11, 2017 at 4:29 PM, Benson Leung <[e-mail]> wrote:
> Hi Oliver,
>
> On Mon, Sep 11, 2017 at 01:02:52PM -0700, Benson Leung wrote:
>> Thanks for the pointer. I'll respin the patch with the no_resume version of
>> usb_autopm_get_interface and retest.
>>
>
> I went and tried this patch but with usb_autopm_get_interface_no_resume instead
> and the original bug I was trying to fix with a Yubikey hid security key came
> back, so something is still wrong here.
>
> I went ahead and filed a bug to track this here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=764112
>
> I'll find some time to dig into this deeper and understand why this device
> ends up getting stuck in active instead of suspending when all handles are
> closed.

Meh, it is problem in hidraw that basically does
usb_autopm_put_interface() before letting usbhid_close() to run. Once
I swap  hid_hw_power() and hid_hw_close() it autosuspends properly.

I just sent out a patch.

Thanks.

Patch

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8a9a21..174d87f0e3e6 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -729,6 +729,7 @@  static int usbhid_open(struct hid_device *hid)
 static void usbhid_close(struct hid_device *hid)
 {
 	struct usbhid_device *usbhid = hid->driver_data;
+	int autopm_error;
 
 	/*
 	 * Make sure we don't restart data acquisition due to
@@ -745,8 +746,14 @@  static void usbhid_close(struct hid_device *hid)
 		return;
 
 	hid_cancel_delayed_stuff(usbhid);
+
+	autopm_error = usb_autopm_get_interface(usbhid->intf);
+
 	usb_kill_urb(usbhid->urbin);
 	usbhid->intf->needs_remote_wakeup = 0;
+
+	if (!autopm_error)
+		usb_autopm_put_interface(usbhid->intf);
 }
 
 /*
@@ -1176,13 +1183,18 @@  static int usbhid_start(struct hid_device *hid)
 static void usbhid_stop(struct hid_device *hid)
 {
 	struct usbhid_device *usbhid = hid->driver_data;
+	int autopm_error;
 
 	if (WARN_ON(!usbhid))
 		return;
 
 	if (hid->quirks & HID_QUIRK_ALWAYS_POLL) {
 		clear_bit(HID_IN_POLLING, &usbhid->iofl);
+
+		autopm_error = usb_autopm_get_interface(usbhid->intf);
 		usbhid->intf->needs_remote_wakeup = 0;
+		if (!autopm_error)
+			usb_autopm_put_interface(usbhid->intf);
 	}
 
 	clear_bit(HID_STARTED, &usbhid->iofl);