HID: usbhid: get/put around clearing needs_remote_wakeup

From: Benson Leung <[email protected]>

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.

---
 drivers/hid/usbhid/hid-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index 089bad8a9a21..174d87f0e3e6 100644
@@ -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);

2.14.1.581.gf28d330327-goog


-- 
Dmitry

Am Freitag, den 08.09.2017, 10:43 -0700 schrieb Dmitry Torokhov:

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

Hi Oliver,

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

Benson

Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]

Hi Oliver,

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

Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]

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.

Dmitry