[PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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(-)

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Alan Stern]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Alan Stern]

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

[PATCH v2] HID: usbhid: clear needs_remote_wakeup before usb_kill_urb

[Benson Leung]

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 <bleung@chromium.org>
---
 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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Alan Stern]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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.


-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Oliver Neukum]

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 <oneukum@suse.de>

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Alan Stern]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

On Fri, Nov 14, 2014 at 7:17 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> 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
>
>
>



-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Alan Stern]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Oliver Neukum]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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


-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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.

-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Oliver Neukum]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Alan Stern]

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

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

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
>



-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

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

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

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

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[Oliver Neukum]

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

[PATCH] HID: usbhid: get/put around clearing needs_remote_wakeup

[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.

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 <bleung@chromium.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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);
-- 
2.14.1.581.gf28d330327-goog


-- 
Dmitry