From: Guenter Roeck <groeck@google.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Guenter Roeck <groeck@chromium.org>,
Nicolas Boichat <drinkcat@chromium.org>,
linux-usb@vger.kernel.org,
"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] HID: usbhid: do not sleep when opening device
Date: Tue, 9 Jun 2020 22:28:00 -0700 [thread overview]
Message-ID: <CABXOdTfdLv5iiZhyGmrmYfk15esoiJhshs_ey+9ma7hLqC4p2A@mail.gmail.com> (raw)
In-Reply-To: <20200610043824.GA171503@dtor-ws>
On Tue, Jun 9, 2020 at 9:38 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> usbhid tries to give the device 50 milliseconds to drain its queues when
> opening the device, but dies it naively by simply sleeping in open handler,
> which slows down device probing (and thus may affect overall boot time).
>
> However we do not need to sleep as we can instead mark a point of time in
> the future when we should start processing the events.
>
> Reported-by: Nicolas Boichat <drinkcat@chromium.org>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
>
> v2: switched from using jiffies to ktime_t to make sure we won't have
> issues with jiffies overflowing.
>
> drivers/hid/usbhid/hid-core.c | 53 +++++++++++++++++++----------------
> drivers/hid/usbhid/usbhid.h | 2 ++
> 2 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index c7bc9db5b192..72c92aab2b18 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -26,6 +26,7 @@
> #include <linux/wait.h>
> #include <linux/workqueue.h>
> #include <linux/string.h>
> +#include <linux/timekeeping.h>
>
> #include <linux/usb.h>
>
> @@ -95,6 +96,18 @@ static int hid_start_in(struct hid_device *hid)
> set_bit(HID_NO_BANDWIDTH, &usbhid->iofl);
> } else {
> clear_bit(HID_NO_BANDWIDTH, &usbhid->iofl);
> +
> + if (test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
> + /*
> + * In case events are generated while nobody was
> + * listening, some are released when the device
> + * is re-opened. Wait 50 msec for the queue to
> + * empty before allowing events to go through
> + * hid.
> + */
> + usbhid->input_start_time =
> + ktime_add_ms(ktime_get_coarse(), 50);
> + }
> }
> }
> spin_unlock_irqrestore(&usbhid->lock, flags);
> @@ -280,20 +293,23 @@ static void hid_irq_in(struct urb *urb)
> if (!test_bit(HID_OPENED, &usbhid->iofl))
> break;
> usbhid_mark_busy(usbhid);
> - if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
> - hid_input_report(urb->context, HID_INPUT_REPORT,
> - urb->transfer_buffer,
> - urb->actual_length, 1);
> - /*
> - * autosuspend refused while keys are pressed
> - * because most keyboards don't wake up when
> - * a key is released
> - */
> - if (hid_check_keys_pressed(hid))
> - set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> - else
> - clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> + if (test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
> + if (ktime_before(ktime_get_coarse(),
> + usbhid->input_start_time))
> + break;
> + clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> }
> + hid_input_report(urb->context, HID_INPUT_REPORT,
> + urb->transfer_buffer, urb->actual_length, 1);
> + /*
> + * autosuspend refused while keys are pressed
> + * because most keyboards don't wake up when
> + * a key is released
> + */
> + if (hid_check_keys_pressed(hid))
> + set_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> + else
> + clear_bit(HID_KEYS_PRESSED, &usbhid->iofl);
> break;
> case -EPIPE: /* stall */
> usbhid_mark_busy(usbhid);
> @@ -714,17 +730,6 @@ static int usbhid_open(struct hid_device *hid)
> }
>
> usb_autopm_put_interface(usbhid->intf);
> -
> - /*
> - * In case events are generated while nobody was listening,
> - * some are released when the device is re-opened.
> - * Wait 50 msec for the queue to empty before allowing events
> - * to go through hid.
> - */
> - if (res == 0)
> - msleep(50);
> -
> - clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> return res;
> }
>
> diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
> index 8620408bd7af..0f0bcf7037f8 100644
> --- a/drivers/hid/usbhid/usbhid.h
> +++ b/drivers/hid/usbhid/usbhid.h
> @@ -13,6 +13,7 @@
>
> #include <linux/types.h>
> #include <linux/slab.h>
> +#include <linux/ktime.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> #include <linux/timer.h>
> @@ -82,6 +83,7 @@ struct usbhid_device {
>
> spinlock_t lock; /* fifo spinlock */
> unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
> + ktime_t input_start_time; /* When to start handling input */
> struct timer_list io_retry; /* Retry timer */
> unsigned long stop_retry; /* Time to give up, in jiffies */
> unsigned int retry_delay; /* Delay length in ms */
> --
> 2.27.0.278.ge193c7cf3a9-goog
>
>
> --
> Dmitry
next prev parent reply other threads:[~2020-06-10 5:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-10 4:38 [PATCH v2] HID: usbhid: do not sleep when opening device Dmitry Torokhov
2020-06-10 5:28 ` Guenter Roeck [this message]
2020-06-16 15:14 ` Jiri Kosina
2020-08-18 11:59 ` Johannes Hirte
2020-08-18 17:52 ` Jiri Kosina
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABXOdTfdLv5iiZhyGmrmYfk15esoiJhshs_ey+9ma7hLqC4p2A@mail.gmail.com \
--to=groeck@google.com \
--cc=benjamin.tissoires@redhat.com \
--cc=dmitry.torokhov@gmail.com \
--cc=drinkcat@chromium.org \
--cc=groeck@chromium.org \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).