Message-ID: <CAGS+omBR6=SknipHES5Kb20V-8pN5knSx1=Q=z7xP6xc-GfWKQ@mail.gmail.com>
Date:	Wed, 14 Dec 2011 17:41:25 +0800
From:	Daniel Kurtz <djkurtz@...omium.org>
To:	Oliver Neukum <oneukum@...e.de>
Cc:	jkosina@...e.cz, bleung@...omium.org, stern@...land.harvard.edu,
	olofj@...omium.org, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

On Wed, Dec 14, 2011 at 5:14 PM, Oliver Neukum <oneukum@...e.de> wrote:
> Am Mittwoch, 14. Dezember 2011, 09:00:18 schrieb Daniel Kurtz:
>> On Wed, Dec 14, 2011 at 3:55 PM, Oliver Neukum <oneukum@...e.de> wrote:
>> > Am Donnerstag, 17. November 2011, 12:23:49 schrieb Daniel Kurtz:
>> >> Unfortunately, this usually happens while the first LED request is
>> >> actually still being processed.  Thus when the completion handler tries
>> >> to submit the second LED request it fails, since REPORTED_IDLE is
>> >> already set!  This REPORTED_IDLE check failure causes the completion
>> >> handler to complete, however without clearing the CTRL_RUNNING flag.
>> >> This, in turn, means that the suspend() handler's wait_io() condition
>> >> is never satisfied, and instead it times out after 10 seconds, aborting
>> >> the original system suspend.
>> >>
>> >> This patch changes the behavior to the following:
>> >>   (1) allow completion handler to finish submitting all queued URBs, even if
>> >>       REPORTED_IDLE is set.  This guarantees that all URBs queued before the
>> >>       hid-core suspend() call will be submitted before the system is
>> >>       suspended.
>> >
>> > Hi,
>> >
>> > why is this desirable? You'd want to requests to be executed at resumption.
>> > A system suspend will alter the LED state anyway.
>>
>> This is how a system suspend 'alters' the LED state.  The input layer
>> sends those LED off commands down through HID as it is trying to
>> suspend.  We want to make sure the usbhid layer actually finishes
>> forwarding those requests on to the device before the system is
>> suspended.
>
> You know the HID layer will send those commands. You don't know which
> other commands may already be queued. So I don't know whether you really want
> to execute them all while the system is asuspending.

I'm confused.  The HID layer is sending which commands?

AFAICT, suspend works like this:
    On system suspend, the input.c suspend() gets called, which saves
off current LED value, and sends LED off events to all keyboards.  If
more than one LED was on, there will be multiple LED off requests.
The HID core immediately submits the first of them (assuming it isn't
in the middle of sending another Control request URB), and queues URBs
for the rest of them, to be submitted by the completion handler.
    Meanwhile, the hid-core.c suspend() gets called soon after the
input suspend() finishes.  This can happen while usbhid driver is
still processing the first LED off request, and therefore there are
still other URBs queued up.  However, since suspend() sets
REPORTED_IDLE, the completion handler can't actually submit them.
Therefore, suspend() times out while waiting for i/o to complete, and
as a result aborts the system suspend.

This patch fixes this problem, and synchronizes queued URBs with
system suspend by sending all urbs that were queued before the usbhid
suspend() was called.

Please let me know if I'm missing something?

-Dan

>
>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists