[PATCH v2] HID: usbhid: do not sleep when opening device

[PATCH v2] HID: usbhid: do not sleep when opening device

[Dmitry Torokhov]

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

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

Re: [PATCH v2] HID: usbhid: do not sleep when opening device

[Guenter Roeck]

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

Re: [PATCH v2] HID: usbhid: do not sleep when opening device

[Jiri Kosina]

On Tue, 9 Jun 2020, Dmitry Torokhov 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,

I've changed 'dies' to 'does' :) and applied, thanks Dmitry.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] HID: usbhid: do not sleep when opening device

[Johannes Hirte]

On 2020 Jun 09, Dmitry Torokhov 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>
> ---
> 

This change breaks various Logitech devices: 
https://bugzilla.kernel.org/show_bug.cgi?id=208935

-- 
Regards,
  Johannes Hirte

Re: [PATCH v2] HID: usbhid: do not sleep when opening device

[Jiri Kosina]

On Tue, 18 Aug 2020, Johannes Hirte 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>
> > ---
> > 
> 
> This change breaks various Logitech devices: 
> https://bugzilla.kernel.org/show_bug.cgi?id=208935

Copy/pasting from the other thread:

=====
Yeah, this problem popped out also in other contexts, where many Logitech 
devices didn't probe properly, because of the race where the first IRQ is 
dropped on the floor (after hid_device_io_start() happens, but before the 
50ms timeout passess), and report descriptor never gets parsed and 
populated.

As this is just a boot time micro-optimization, I am going to revert the 
patch for 5.9 now, and we can try to fix this properly for next merge 
window.
=====

Thanks,

-- 
Jiri Kosina
SUSE Labs