| Message ID | 1415909806-23848-1-git-send-email-bleung@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Thu, 13 Nov 2014, Benson Leung wrote: > usbhid->intf->needs_remote_wakeup is set when a device is > opened, and is cleared when a device is closed. > > When a usbhid device that does not support remote wake > ( i.e. !device_can_wakeup() ) is closed, we fail out of > autosuspend_check() because the autosuspend check is called > before the flag is cleared as a result of usb_kill_urb(usbhid->urbin); > > The result is that a device that may otherwise autosuspend will > fail to enter suspend again after all handles to it are 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, the same > protection isn't there when clearing needs_remote_wakeup. This will > add that to usbhid_close as well as usbhid_stop. usbhid_stop probably doesn't need it. And it should be possible to fix usbhid_close more easily just by interchanging the two lines: - usb_kill_urb(usbhid->urbin); usbhid->intf->needs_remote_wakeup = 0; + usb_kill_urb(usbhid->urbin); Have you tried this? Alan Stern -- 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
On Thu, Nov 13, 2014 at 12:41 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > usbhid_stop probably doesn't need it. And it should be possible to fix > usbhid_close more easily just by interchanging the two lines: > > - usb_kill_urb(usbhid->urbin); > usbhid->intf->needs_remote_wakeup = 0; > + usb_kill_urb(usbhid->urbin); > > Have you tried this? Yes, I tried that as well, and that does work. I used the get/put because that's the way it was done in other drivers, for example in synusb_close() in drivers/input/mouse/synaptics_usb.c
On Thu, 13 Nov 2014, Benson Leung wrote: > On Thu, Nov 13, 2014 at 12:41 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > usbhid_stop probably doesn't need it. And it should be possible to fix > > usbhid_close more easily just by interchanging the two lines: > > > > - usb_kill_urb(usbhid->urbin); > > usbhid->intf->needs_remote_wakeup = 0; > > + usb_kill_urb(usbhid->urbin); > > > > Have you tried this? > > Yes, I tried that as well, and that does work. > > I used the get/put because that's the way it was done in other > drivers, for example in synusb_close() in > drivers/input/mouse/synaptics_usb.c In the patch description, you wrote: > When a usbhid device that does not support remote wake > ( i.e. !device_can_wakeup() ) is closed, we fail out of > autosuspend_check() because the autosuspend check is called > before the flag is cleared as a result of usb_kill_urb(usbhid->urbin); If you interchange the two lines then the flag _will_ be cleared before usb_kill_urb() and autosuspend_check() can run. This means your patch description is bogus. Alan Stern -- 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
On Thu, Nov 13, 2014 at 1:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > If you interchange the two lines then the flag _will_ be cleared before > usb_kill_urb() and autosuspend_check() can run. This means your patch > description is bogus. Gotcha. v2 posted. Thanks for the direction.
On Thu, 13 Nov 2014, Benson Leung wrote: > On Thu, Nov 13, 2014 at 1:18 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > If you interchange the two lines then the flag _will_ be cleared before > > usb_kill_urb() and autosuspend_check() can run. This means your patch > > description is bogus. > > Gotcha. v2 posted. Thanks for the direction. Wait a minute -- in your previous email you said this approach didn't work. So does it work or doesn't it? Alan Stern -- 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 Alan, On Thu, Nov 13, 2014 at 2:11 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > Wait a minute -- in your previous email you said this approach didn't > work. So does it work or doesn't it? Sorry for the confusion. The approach *does* work. That was actually my original idea to fix the problem, but I saw other places in the kernel where it was done with a get/put.
On Thu, 2014-11-13 at 12:16 -0800, Benson Leung wrote: > 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, the same > protection isn't there when clearing needs_remote_wakeup. This will > add that to usbhid_close as well as usbhid_stop. Interesting, but this has the side effect of waking devices that are asleep just to remove the flag. Regards Oliver
On Thu, 13 Nov 2014, Benson Leung wrote: > Hi Alan, > > On Thu, Nov 13, 2014 at 2:11 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > Wait a minute -- in your previous email you said this approach didn't > > work. So does it work or doesn't it? > > Sorry for the confusion. The approach *does* work. > > That was actually my original idea to fix the problem, but I saw other > places in the kernel where it was done with a get/put. The reason for the get/put is to force a call to autosuspend_check(). But in this case, if killing the interrupt URB causes autosuspend_check() to run then the get/put isn't needed. On the other hand, I don't see why killing the interrupt URB would cause autosuspend_check() to run. Can you explain that? Alan Stern -- 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 Alan, On Fri, Nov 14, 2014 at 7:17 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > The reason for the get/put is to force a call to autosuspend_check(). > But in this case, if killing the interrupt URB causes > autosuspend_check() to run then the get/put isn't needed. > > On the other hand, I don't see why killing the interrupt URB would > cause autosuspend_check() to run. Can you explain that? Sorry for the delay in my response. I did some more checking of my particular failure, and my commit message is incorrect. The usb_kill_urb is actually not the cause of this problem. It does not result in autosuspend_check() itself, and is only serving to add some delay. hidraw_release() in hidraw.c calls drop_ref(), which calls the following in sequence upon clearing the last reader : /* close device for last reader */ hid_hw_power(hidraw->hid, PM_HINT_NORMAL); hid_hw_close(hidraw->hid); hid_hw_power results in a usb_autopm_put_interface. In this case, the reference count is decremented to 0, and a delayed autosuspend request is attempted. hid_hw_close leads to usbhid_close, which clears needs_remote_wakeup. However, there's no guarantee that the clear of needs_remote_wakeup will occur before the delayed work ( runtime_idle() -> autosuspend_check() ) runs. Moving usbhid->intf->needs_remote_wakeup = 0 to before the usb_kill_urb(usbhid->urbin) only serves to reduce the amount of time between these events and makes this particular failure less likely. The correct solution is to put get/put around each change of needs_remote_wakeup, as that will correctly trigger another delayed autosuspend_check(), whose result is affected by the state of needs_remote_wakeup. Since autosuspend_check() occurs as delayed work, I think it is appropriate to add get/put around the clear in usbhid_stop as well.
On Fri, Nov 14, 2014 at 1:08 AM, Oliver Neukum <oneukum@suse.de> wrote: > On Thu, 2014-11-13 at 12:16 -0800, Benson Leung wrote: > >> 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, the same >> protection isn't there when clearing needs_remote_wakeup. This will >> add that to usbhid_close as well as usbhid_stop. > > Interesting, but this has the side effect of waking devices > that are asleep just to remove the flag. > > Regards If devices are already asleep with this flag enabled, that means that they are presently configured for remote wake. Waking the device in the case of a close() is appropriate because it also has the effect of re-suspending the device with the capability disabled, as it is no longer necessary.
On Fri, 21 Nov 2014, Benson Leung wrote: > Sorry for the delay in my response. I did some more checking of my > particular failure, and my commit message is incorrect. The > usb_kill_urb is actually not the cause of this problem. It does not > result in autosuspend_check() itself, and is only serving to add some > delay. > > hidraw_release() in hidraw.c calls drop_ref(), which calls the > following in sequence upon clearing the last reader : > /* close device for last reader */ > hid_hw_power(hidraw->hid, PM_HINT_NORMAL); > hid_hw_close(hidraw->hid); > > hid_hw_power results in a usb_autopm_put_interface. In this case, the > reference count is decremented to 0, and a delayed autosuspend request > is attempted. > hid_hw_close leads to usbhid_close, which clears needs_remote_wakeup. > > However, there's no guarantee that the clear of needs_remote_wakeup > will occur before the delayed work ( runtime_idle() -> > autosuspend_check() ) runs. Moving usbhid->intf->needs_remote_wakeup > = 0 to before the usb_kill_urb(usbhid->urbin) only serves to reduce > the amount of time between these events and makes this particular > failure less likely. > > The correct solution is to put get/put around each change of > needs_remote_wakeup, as that will correctly trigger another delayed > autosuspend_check(), whose result is affected by the state of > needs_remote_wakeup. > > Since autosuspend_check() occurs as delayed work, I think it is > appropriate to add get/put around the clear in usbhid_stop as well. As Oliver pointed out, there's no real need to resume a device which is already suspended (the only effect would be to allow it to suspend again but with wakeup disabled -- and this would happen anyway if a wakeup occurred). Instead of using get and put, we should have an idle call. There is no USB wrapper for pm_runtime_idle calls, but one could be added. Still, in the meantime can you check to see what happens if you add pm_runtime_idle(&usbhid->intf->dev); in usbhid_close() just after needs_remote_wakeup is set to 0? You can do the same thing in usbhid_stop() if you want. Alan Stern -- 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
On Sat, 22 Nov 2014, Alan Stern wrote: > There is no USB wrapper for pm_runtime_idle calls, but one could be > added. Still, in the meantime can you check to see what happens if you > add > > pm_runtime_idle(&usbhid->intf->dev); > > in usbhid_close() just after needs_remote_wakeup is set to 0? You can > do the same thing in usbhid_stop() if you want. Come to think of it, we probably need pm_runtime_idle(usbhid->intf->dev->parent); in addition to the function call above. When a USB wrapper is written, it can take care of these details. Alan Stern -- 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
On Fri, 2014-11-21 at 17:00 -0800, Benson Leung wrote: > If devices are already asleep with this flag enabled, that means that > they are presently configured for remote wake. Yes, but that doesn't matter. The drivers must be ready for a device being resumed at any time. Remote wakeup just adds one more reason. > Waking the device in the case of a close() is appropriate because it > also has the effect of re-suspending the device with the capability > disabled, as it is no longer necessary. But there is very little to be gained by switching off remote wakeup. The additional energy consumption devices with remote wakeup enabled will be dwarfed by the energy needed for an additional wakeup. 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 Mon, Nov 24, 2014 at 1:13 AM, Oliver Neukum <oneukum@suse.de> wrote: > > But there is very little to be gained by switching off remote wakeup. > The additional energy consumption devices with remote wakeup enabled > will be dwarfed by the energy needed for an additional wakeup. > That makes sense to me. Does this mean we should be moving toward a solution that doesn't wake suspended devices on close for other usb devices, not just hid? This particular pattern of get()/needs_remote_wakeup=0/put() on close() appears in several other drivers, for example : 62ecae0 Input: wacom - properly enable runtime PM 5d9efc5 Input: usbtouchscreen - implement runtime power management
Hi Alan, On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > There is no USB wrapper for pm_runtime_idle calls, but one could be > added. Still, in the meantime can you check to see what happens if you > add > > pm_runtime_idle(&usbhid->intf->dev); > > in usbhid_close() just after needs_remote_wakeup is set to 0? You can > do the same thing in usbhid_stop() if you want. I tried using this in lieu of usb_autopm_get/put_interface: usbhid->intf->needs_remote_wakeup = 0; pm_runtime_idle(&usbhid->intf->dev); pm_runtime_idle(usbhid->intf->dev.parent); It did not work. I see the autosuspend_check() that was kicked off as a result of hid_hw_power, which falls into the "remote wakeup needed for autosuspend" branch, but I don't see another autosuspend_check() that picks up the updated value of needs_remote_wakeup.
On Mon, 2014-11-24 at 16:56 -0800, Benson Leung wrote: > Hi Oliver, > > On Mon, Nov 24, 2014 at 1:13 AM, Oliver Neukum <oneukum@suse.de> wrote: > > > > But there is very little to be gained by switching off remote wakeup. > > The additional energy consumption devices with remote wakeup enabled > > will be dwarfed by the energy needed for an additional wakeup. > > > > That makes sense to me. Does this mean we should be moving toward a > solution that doesn't wake suspended devices on close for other usb > devices, not just hid? > > This particular pattern of get()/needs_remote_wakeup=0/put() on > close() appears in several other drivers, for example : > 62ecae0 Input: wacom - properly enable runtime PM > 5d9efc5 Input: usbtouchscreen - implement runtime power management Yes, we should never wake up a device just to unset remote wakeup for runtime PM. In hindsight those patches were clumsy. 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
On Mon, 24 Nov 2014, Benson Leung wrote: > Hi Alan, > > > On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > There is no USB wrapper for pm_runtime_idle calls, but one could be > > added. Still, in the meantime can you check to see what happens if you > > add > > > > pm_runtime_idle(&usbhid->intf->dev); > > > > in usbhid_close() just after needs_remote_wakeup is set to 0? You can > > do the same thing in usbhid_stop() if you want. > > I tried using this in lieu of usb_autopm_get/put_interface: > > usbhid->intf->needs_remote_wakeup = 0; > pm_runtime_idle(&usbhid->intf->dev); > pm_runtime_idle(usbhid->intf->dev.parent); > > It did not work. I see the autosuspend_check() that was kicked off as > a result of hid_hw_power, which falls into the "remote wakeup needed > for autosuspend" branch, but I don't see another autosuspend_check() > that picks up the updated value of needs_remote_wakeup. Well, why not? In order to work on the kernel effectively, you need the right mind-set. Don't just tell people when something goes wrong -- figure out why the problem occurred and propose a way to fix it. Alan Stern -- 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
On Tue, Nov 25, 2014 at 7:24 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 24 Nov 2014, Benson Leung wrote: > >> Hi Alan, >> >> >> On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > There is no USB wrapper for pm_runtime_idle calls, but one could be >> > added. Still, in the meantime can you check to see what happens if you >> > add >> > >> > pm_runtime_idle(&usbhid->intf->dev); >> > >> > in usbhid_close() just after needs_remote_wakeup is set to 0? You can >> > do the same thing in usbhid_stop() if you want. >> >> I tried using this in lieu of usb_autopm_get/put_interface: >> >> usbhid->intf->needs_remote_wakeup = 0; >> pm_runtime_idle(&usbhid->intf->dev); >> pm_runtime_idle(usbhid->intf->dev.parent); >> >> It did not work. I see the autosuspend_check() that was kicked off as >> a result of hid_hw_power, which falls into the "remote wakeup needed >> for autosuspend" branch, but I don't see another autosuspend_check() >> that picks up the updated value of needs_remote_wakeup. > > Well, why not? > > In order to work on the kernel effectively, you need the right > mind-set. Don't just tell people when something goes wrong -- figure > out why the problem occurred and propose a way to fix it. Sure. I'll dig into this deeper today. I got to this late yesterday and I ran out of time before I could find out what was not behaving correctly. > > Alan Stern >
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 04e34b9..2a0b91d 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -734,8 +734,15 @@ void usbhid_close(struct hid_device *hid) spin_unlock_irq(&usbhid->lock); hid_cancel_delayed_stuff(usbhid); if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL)) { + int autopm_error; + + 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); } } else { spin_unlock_irq(&usbhid->lock); @@ -1179,9 +1186,17 @@ static void usbhid_stop(struct hid_device *hid) if (WARN_ON(!usbhid)) return; - if (hid->quirks & HID_QUIRK_ALWAYS_POLL) + if (hid->quirks & HID_QUIRK_ALWAYS_POLL) { + int autopm_error; + + 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); spin_lock_irq(&usbhid->lock); /* Sync with error and led handlers */ set_bit(HID_DISCONNECTED, &usbhid->iofl);
usbhid->intf->needs_remote_wakeup is set when a device is opened, and is cleared when a device is closed. When a usbhid device that does not support remote wake ( i.e. !device_can_wakeup() ) is closed, we fail out of autosuspend_check() because the autosuspend check is called before the flag is cleared as a result of usb_kill_urb(usbhid->urbin); The result is that a device that may otherwise autosuspend will fail to enter suspend again after all handles to it are 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, the same protection isn't there when clearing needs_remote_wakeup. This will add that to usbhid_close as well as usbhid_stop. Signed-off-by: Benson Leung <bleung@chromium.org> --- drivers/hid/usbhid/hid-core.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)