Message ID | 20170908174337.GA13616@dtor-ws (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Am Freitag, den 08.09.2017, 10:43 -0700 schrieb Dmitry Torokhov: > From: Benson Leung <bleung@chromium.org> > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <bleung@chromium.org> > > > > 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
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
On Mon, Sep 11, 2017 at 4:29 PM, Benson Leung <bleung@google.com> 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.
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);