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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx

On Fri, Nov 14, 2014 at 7:17 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 13 Nov 2014, Benson Leung wrote:
>
>> Hi Alan,
>>
>> On Thu, Nov 13, 2014 at 2:11 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/