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 <[email protected]>
---
drivers/hid/usbhid/hid-core.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
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);
--
2.1.0.rc2.206.gedb03e5
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
On Thu, Nov 13, 2014 at 12:41 PM, Alan Stern <[email protected]> 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
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
On Thu, 13 Nov 2014, Benson Leung wrote:
> On Thu, Nov 13, 2014 at 12:41 PM, Alan Stern <[email protected]> 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
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.
Clear the flag first before usb_kill_urb, which may result in a
autosuspend_check().
Signed-off-by: Benson Leung <[email protected]>
---
drivers/hid/usbhid/hid-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 04e34b9..ca66ad7 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -734,8 +734,8 @@ void usbhid_close(struct hid_device *hid)
spin_unlock_irq(&usbhid->lock);
hid_cancel_delayed_stuff(usbhid);
if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL)) {
- usb_kill_urb(usbhid->urbin);
usbhid->intf->needs_remote_wakeup = 0;
+ usb_kill_urb(usbhid->urbin);
}
} else {
spin_unlock_irq(&usbhid->lock);
--
2.1.0.rc2.206.gedb03e5
On Thu, Nov 13, 2014 at 1:18 PM, Alan Stern <[email protected]> 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.
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
On Thu, 13 Nov 2014, Benson Leung wrote:
> On Thu, Nov 13, 2014 at 1:18 PM, Alan Stern <[email protected]> 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
Hi Alan,
On Thu, Nov 13, 2014 at 2:11 PM, Alan Stern <[email protected]> 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.
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
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
--
Oliver Neukum <[email protected]>
On Thu, 13 Nov 2014, Benson Leung wrote:
> Hi Alan,
>
> On Thu, Nov 13, 2014 at 2:11 PM, Alan Stern <[email protected]> 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
Hi Alan,
On Fri, Nov 14, 2014 at 7:17 AM, Alan Stern <[email protected]> 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.
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
On Fri, Nov 14, 2014 at 7:17 AM, Alan Stern <[email protected]> wrote:
> On Thu, 13 Nov 2014, Benson Leung wrote:
>
>> Hi Alan,
>>
>> On Thu, Nov 13, 2014 at 2:11 PM, Alan Stern <[email protected]> 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
>
>
>
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
On Fri, Nov 14, 2014 at 1:08 AM, Oliver Neukum <[email protected]> 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.
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
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
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
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
Hi Oliver,
On Mon, Nov 24, 2014 at 1:13 AM, Oliver Neukum <[email protected]> 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
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
Hi Alan,
On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <[email protected]> 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.
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]
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 <[email protected]> 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
On Mon, 24 Nov 2014, Benson Leung wrote:
> Hi Alan,
>
>
> On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <[email protected]> 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
On Tue, Nov 25, 2014 at 7:24 AM, Alan Stern <[email protected]> wrote:
> On Mon, 24 Nov 2014, Benson Leung wrote:
>
>> Hi Alan,
>>
>>
>> On Sat, Nov 22, 2014 at 7:55 AM, Alan Stern <[email protected]> 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
>
--
Benson Leung
Software Engineer, Chrom* OS
[email protected]