* [PATCH] HID: usbhid: do not sleep when opening device
@ 2020-05-29 19:59 Dmitry Torokhov
2020-05-29 20:14 ` Guenter Roeck
2020-05-29 23:50 ` Nicolas Boichat
0 siblings, 2 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-05-29 19:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: groeck, Nicolas Boichat, linux-usb, linux-input, linux-kernel
usbhid tries to give the device 50 milliseconds to drain its queues
when opening the device, but does 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>
---
drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
drivers/hid/usbhid/usbhid.h | 1 +
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index c7bc9db5b192..e69992e945b2 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
+ msecs_to_jiffies(50);
+ }
}
}
spin_unlock_irqrestore(&usbhid->lock, flags);
@@ -280,7 +293,8 @@ 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)) {
+ if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
+ time_after(jiffies, usbhid->input_start_time)) {
hid_input_report(urb->context, HID_INPUT_REPORT,
urb->transfer_buffer,
urb->actual_length, 1);
@@ -714,17 +728,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..805949671b96 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -82,6 +82,7 @@ struct usbhid_device {
spinlock_t lock; /* fifo spinlock */
unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
+ unsigned long input_start_time; /* When to start handling input, in jiffies */
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.rc0.183.gde8f92d652-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-29 19:59 [PATCH] HID: usbhid: do not sleep when opening device Dmitry Torokhov
@ 2020-05-29 20:14 ` Guenter Roeck
2020-05-29 20:33 ` Dmitry Torokhov
2020-05-29 23:50 ` Nicolas Boichat
1 sibling, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-05-29 20:14 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, groeck, Nicolas Boichat,
linux-usb, linux-input, linux-kernel
On Fri, May 29, 2020 at 12:59:51PM -0700, Dmitry Torokhov wrote:
> usbhid tries to give the device 50 milliseconds to drain its queues
> when opening the device, but does 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>
> ---
> drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> drivers/hid/usbhid/usbhid.h | 1 +
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index c7bc9db5b192..e69992e945b2 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> + msecs_to_jiffies(50);
> + }
> }
> }
> spin_unlock_irqrestore(&usbhid->lock, flags);
> @@ -280,7 +293,8 @@ 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)) {
> + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> + time_after(jiffies, usbhid->input_start_time)) {
> hid_input_report(urb->context, HID_INPUT_REPORT,
> urb->transfer_buffer,
> urb->actual_length, 1);
> @@ -714,17 +728,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);
> -
Can you just set usbhid->input_start_time here ?
if (res == 0)
usbhid->input_start_time = jiffies + msecs_to_jiffies(50);
clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
Then you might not need the added code in hid_start_in().
Thanks,
Guenter
> - 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..805949671b96 100644
> --- a/drivers/hid/usbhid/usbhid.h
> +++ b/drivers/hid/usbhid/usbhid.h
> @@ -82,6 +82,7 @@ struct usbhid_device {
>
> spinlock_t lock; /* fifo spinlock */
> unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
> + unsigned long input_start_time; /* When to start handling input, in jiffies */
> 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.rc0.183.gde8f92d652-goog
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-29 20:14 ` Guenter Roeck
@ 2020-05-29 20:33 ` Dmitry Torokhov
2020-05-29 20:44 ` Guenter Roeck
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2020-05-29 20:33 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jiri Kosina, Benjamin Tissoires, groeck, Nicolas Boichat,
linux-usb, linux-input, linux-kernel
On Fri, May 29, 2020 at 01:14:24PM -0700, Guenter Roeck wrote:
> On Fri, May 29, 2020 at 12:59:51PM -0700, Dmitry Torokhov wrote:
> > usbhid tries to give the device 50 milliseconds to drain its queues
> > when opening the device, but does 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>
> > ---
> > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > drivers/hid/usbhid/usbhid.h | 1 +
> > 2 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index c7bc9db5b192..e69992e945b2 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > + msecs_to_jiffies(50);
> > + }
> > }
> > }
> > spin_unlock_irqrestore(&usbhid->lock, flags);
> > @@ -280,7 +293,8 @@ 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)) {
> > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > + time_after(jiffies, usbhid->input_start_time)) {
> > hid_input_report(urb->context, HID_INPUT_REPORT,
> > urb->transfer_buffer,
> > urb->actual_length, 1);
> > @@ -714,17 +728,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);
> > -
> Can you just set usbhid->input_start_time here ?
> if (res == 0)
> usbhid->input_start_time = jiffies + msecs_to_jiffies(50);
> clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
>
> Then you might not need the added code in hid_start_in().
That was my first version, but if hid_start_in() fails we start a timer
and try to retry the IO (and the "res" in 0 in this case). And we want
to mark the time only after we successfully submitted the interrupt URB,
that is why the code is in hid_start_in().
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-29 20:33 ` Dmitry Torokhov
@ 2020-05-29 20:44 ` Guenter Roeck
0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2020-05-29 20:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, groeck, Nicolas Boichat,
linux-usb, linux-input, linux-kernel
On Fri, May 29, 2020 at 01:33:24PM -0700, Dmitry Torokhov wrote:
> On Fri, May 29, 2020 at 01:14:24PM -0700, Guenter Roeck wrote:
> > On Fri, May 29, 2020 at 12:59:51PM -0700, Dmitry Torokhov wrote:
> > > usbhid tries to give the device 50 milliseconds to drain its queues
> > > when opening the device, but does 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>
> > > ---
> > > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > > drivers/hid/usbhid/usbhid.h | 1 +
> > > 2 files changed, 16 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > index c7bc9db5b192..e69992e945b2 100644
> > > --- a/drivers/hid/usbhid/hid-core.c
> > > +++ b/drivers/hid/usbhid/hid-core.c
> > > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > > + msecs_to_jiffies(50);
> > > + }
> > > }
> > > }
> > > spin_unlock_irqrestore(&usbhid->lock, flags);
> > > @@ -280,7 +293,8 @@ 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)) {
> > > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > > + time_after(jiffies, usbhid->input_start_time)) {
> > > hid_input_report(urb->context, HID_INPUT_REPORT,
> > > urb->transfer_buffer,
> > > urb->actual_length, 1);
> > > @@ -714,17 +728,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);
> > > -
> > Can you just set usbhid->input_start_time here ?
> > if (res == 0)
> > usbhid->input_start_time = jiffies + msecs_to_jiffies(50);
> > clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> >
> > Then you might not need the added code in hid_start_in().
>
> That was my first version, but if hid_start_in() fails we start a timer
> and try to retry the IO (and the "res" in 0 in this case). And we want
> to mark the time only after we successfully submitted the interrupt URB,
> that is why the code is in hid_start_in().
>
Ah yes, that makes sense.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Thanks,
Guenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-29 19:59 [PATCH] HID: usbhid: do not sleep when opening device Dmitry Torokhov
2020-05-29 20:14 ` Guenter Roeck
@ 2020-05-29 23:50 ` Nicolas Boichat
2020-05-30 0:48 ` Guenter Roeck
1 sibling, 1 reply; 11+ messages in thread
From: Nicolas Boichat @ 2020-05-29 23:50 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, Guenter Roeck, linux-usb,
open list:HID CORE LAYER, lkml
On Sat, May 30, 2020 at 3:59 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> usbhid tries to give the device 50 milliseconds to drain its queues
> when opening the device, but does 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>
> ---
> drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> drivers/hid/usbhid/usbhid.h | 1 +
> 2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index c7bc9db5b192..e69992e945b2 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> + msecs_to_jiffies(50);
> + }
> }
> }
> spin_unlock_irqrestore(&usbhid->lock, flags);
> @@ -280,7 +293,8 @@ 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)) {
> + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> + time_after(jiffies, usbhid->input_start_time)) {
Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
> hid_input_report(urb->context, HID_INPUT_REPORT,
> urb->transfer_buffer,
> urb->actual_length, 1);
> @@ -714,17 +728,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..805949671b96 100644
> --- a/drivers/hid/usbhid/usbhid.h
> +++ b/drivers/hid/usbhid/usbhid.h
> @@ -82,6 +82,7 @@ struct usbhid_device {
>
> spinlock_t lock; /* fifo spinlock */
> unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
> + unsigned long input_start_time; /* When to start handling input, in jiffies */
> 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.rc0.183.gde8f92d652-goog
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-29 23:50 ` Nicolas Boichat
@ 2020-05-30 0:48 ` Guenter Roeck
2020-05-30 1:09 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-05-30 0:48 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, Guenter Roeck,
linux-usb, open list:HID CORE LAYER, lkml
On Fri, May 29, 2020 at 4:50 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> On Sat, May 30, 2020 at 3:59 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > usbhid tries to give the device 50 milliseconds to drain its queues
> > when opening the device, but does 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>
> > ---
> > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > drivers/hid/usbhid/usbhid.h | 1 +
> > 2 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index c7bc9db5b192..e69992e945b2 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > + msecs_to_jiffies(50);
> > + }
> > }
> > }
> > spin_unlock_irqrestore(&usbhid->lock, flags);
> > @@ -280,7 +293,8 @@ 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)) {
> > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > + time_after(jiffies, usbhid->input_start_time)) {
>
> Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
>
time_after() is overflow-safe. That is why it is used and jiffies is
not compared directly.
Guenter
> > hid_input_report(urb->context, HID_INPUT_REPORT,
> > urb->transfer_buffer,
> > urb->actual_length, 1);
> > @@ -714,17 +728,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..805949671b96 100644
> > --- a/drivers/hid/usbhid/usbhid.h
> > +++ b/drivers/hid/usbhid/usbhid.h
> > @@ -82,6 +82,7 @@ struct usbhid_device {
> >
> > spinlock_t lock; /* fifo spinlock */
> > unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
> > + unsigned long input_start_time; /* When to start handling input, in jiffies */
> > 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.rc0.183.gde8f92d652-goog
> >
> >
> > --
> > Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-30 0:48 ` Guenter Roeck
@ 2020-05-30 1:09 ` Dmitry Torokhov
2020-05-30 1:22 ` Guenter Roeck
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2020-05-30 1:09 UTC (permalink / raw)
To: Guenter Roeck
Cc: Nicolas Boichat, Jiri Kosina, Benjamin Tissoires, Guenter Roeck,
linux-usb, open list:HID CORE LAYER, lkml
On Fri, May 29, 2020 at 05:48:26PM -0700, Guenter Roeck wrote:
> On Fri, May 29, 2020 at 4:50 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Sat, May 30, 2020 at 3:59 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > usbhid tries to give the device 50 milliseconds to drain its queues
> > > when opening the device, but does 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>
> > > ---
> > > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > > drivers/hid/usbhid/usbhid.h | 1 +
> > > 2 files changed, 16 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > index c7bc9db5b192..e69992e945b2 100644
> > > --- a/drivers/hid/usbhid/hid-core.c
> > > +++ b/drivers/hid/usbhid/hid-core.c
> > > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > > + msecs_to_jiffies(50);
> > > + }
> > > }
> > > }
> > > spin_unlock_irqrestore(&usbhid->lock, flags);
> > > @@ -280,7 +293,8 @@ 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)) {
> > > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > > + time_after(jiffies, usbhid->input_start_time)) {
> >
> > Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
> >
>
> time_after() is overflow-safe. That is why it is used and jiffies is
> not compared directly.
Well, it is overflow safe, but still can not measure more than 50 days,
so if you have a device open for 50+ days there will be a 50msec gap
where it may lose events.
I guess we can switch to ktime(). A bit more expensive on 32 bits, but
in reality I do not think anyone would care.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-30 1:09 ` Dmitry Torokhov
@ 2020-05-30 1:22 ` Guenter Roeck
2020-05-30 1:34 ` Dmitry Torokhov
0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2020-05-30 1:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Nicolas Boichat, Jiri Kosina, Benjamin Tissoires, Guenter Roeck,
linux-usb, open list:HID CORE LAYER, lkml
On Fri, May 29, 2020 at 6:09 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, May 29, 2020 at 05:48:26PM -0700, Guenter Roeck wrote:
> > On Fri, May 29, 2020 at 4:50 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > On Sat, May 30, 2020 at 3:59 AM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > usbhid tries to give the device 50 milliseconds to drain its queues
> > > > when opening the device, but does 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>
> > > > ---
> > > > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > > > drivers/hid/usbhid/usbhid.h | 1 +
> > > > 2 files changed, 16 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > > index c7bc9db5b192..e69992e945b2 100644
> > > > --- a/drivers/hid/usbhid/hid-core.c
> > > > +++ b/drivers/hid/usbhid/hid-core.c
> > > > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > > > + msecs_to_jiffies(50);
> > > > + }
> > > > }
> > > > }
> > > > spin_unlock_irqrestore(&usbhid->lock, flags);
> > > > @@ -280,7 +293,8 @@ 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)) {
> > > > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > > > + time_after(jiffies, usbhid->input_start_time)) {
> > >
> > > Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
> > >
> >
> > time_after() is overflow-safe. That is why it is used and jiffies is
> > not compared directly.
>
> Well, it is overflow safe, but still can not measure more than 50 days,
> so if you have a device open for 50+ days there will be a 50msec gap
> where it may lose events.
>
Or you could explicitly use 64-bit jiffies.
Guenter
> I guess we can switch to ktime(). A bit more expensive on 32 bits, but
> in reality I do not think anyone would care.
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-30 1:22 ` Guenter Roeck
@ 2020-05-30 1:34 ` Dmitry Torokhov
2020-06-01 17:13 ` Jiri Kosina
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2020-05-30 1:34 UTC (permalink / raw)
To: Guenter Roeck
Cc: Nicolas Boichat, Jiri Kosina, Benjamin Tissoires, Guenter Roeck,
linux-usb, open list:HID CORE LAYER, lkml
On Fri, May 29, 2020 at 06:22:49PM -0700, Guenter Roeck wrote:
> On Fri, May 29, 2020 at 6:09 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, May 29, 2020 at 05:48:26PM -0700, Guenter Roeck wrote:
> > > On Fri, May 29, 2020 at 4:50 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >
> > > > On Sat, May 30, 2020 at 3:59 AM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > usbhid tries to give the device 50 milliseconds to drain its queues
> > > > > when opening the device, but does 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>
> > > > > ---
> > > > > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > > > > drivers/hid/usbhid/usbhid.h | 1 +
> > > > > 2 files changed, 16 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > > > index c7bc9db5b192..e69992e945b2 100644
> > > > > --- a/drivers/hid/usbhid/hid-core.c
> > > > > +++ b/drivers/hid/usbhid/hid-core.c
> > > > > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > > > > + msecs_to_jiffies(50);
> > > > > + }
> > > > > }
> > > > > }
> > > > > spin_unlock_irqrestore(&usbhid->lock, flags);
> > > > > @@ -280,7 +293,8 @@ 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)) {
> > > > > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > > > > + time_after(jiffies, usbhid->input_start_time)) {
> > > >
> > > > Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
> > > >
> > >
> > > time_after() is overflow-safe. That is why it is used and jiffies is
> > > not compared directly.
> >
> > Well, it is overflow safe, but still can not measure more than 50 days,
> > so if you have a device open for 50+ days there will be a 50msec gap
> > where it may lose events.
> >
>
> Or you could explicitly use 64-bit jiffies.
Indeed.
Jiri, Benjamin, do you have preference between jiffies64 and ktime_t? I
guess jiffies64 is a tiny bit less expensive.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-05-30 1:34 ` Dmitry Torokhov
@ 2020-06-01 17:13 ` Jiri Kosina
2020-06-02 9:14 ` Benjamin Tissoires
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Kosina @ 2020-06-01 17:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Guenter Roeck, Nicolas Boichat, Benjamin Tissoires,
Guenter Roeck, linux-usb, open list:HID CORE LAYER, lkml
On Fri, 29 May 2020, Dmitry Torokhov wrote:
> > > > > > usbhid tries to give the device 50 milliseconds to drain its queues
> > > > > > when opening the device, but does 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>
> > > > > > ---
> > > > > > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > > > > > drivers/hid/usbhid/usbhid.h | 1 +
> > > > > > 2 files changed, 16 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > > > > index c7bc9db5b192..e69992e945b2 100644
> > > > > > --- a/drivers/hid/usbhid/hid-core.c
> > > > > > +++ b/drivers/hid/usbhid/hid-core.c
> > > > > > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > > > > > + msecs_to_jiffies(50);
> > > > > > + }
> > > > > > }
> > > > > > }
> > > > > > spin_unlock_irqrestore(&usbhid->lock, flags);
> > > > > > @@ -280,7 +293,8 @@ 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)) {
> > > > > > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > > > > > + time_after(jiffies, usbhid->input_start_time)) {
> > > > >
> > > > > Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
> > > > >
> > > >
> > > > time_after() is overflow-safe. That is why it is used and jiffies is
> > > > not compared directly.
> > >
> > > Well, it is overflow safe, but still can not measure more than 50 days,
> > > so if you have a device open for 50+ days there will be a 50msec gap
> > > where it may lose events.
> > >
> >
> > Or you could explicitly use 64-bit jiffies.
>
> Indeed.
>
> Jiri, Benjamin, do you have preference between jiffies64 and ktime_t? I
> guess jiffies64 is a tiny bit less expensive.
If I would be writing the code, I'd use ktime_t, because I personally like
that abstraction more :) But either variant works for me.
Thanks!
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] HID: usbhid: do not sleep when opening device
2020-06-01 17:13 ` Jiri Kosina
@ 2020-06-02 9:14 ` Benjamin Tissoires
0 siblings, 0 replies; 11+ messages in thread
From: Benjamin Tissoires @ 2020-06-02 9:14 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dmitry Torokhov, Guenter Roeck, Nicolas Boichat, Guenter Roeck,
Linux USB Mailing List, open list:HID CORE LAYER, lkml
On Mon, Jun 1, 2020 at 7:13 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Fri, 29 May 2020, Dmitry Torokhov wrote:
>
> > > > > > > usbhid tries to give the device 50 milliseconds to drain its queues
> > > > > > > when opening the device, but does 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>
> > > > > > > ---
> > > > > > > drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------
> > > > > > > drivers/hid/usbhid/usbhid.h | 1 +
> > > > > > > 2 files changed, 16 insertions(+), 12 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > > > > > index c7bc9db5b192..e69992e945b2 100644
> > > > > > > --- a/drivers/hid/usbhid/hid-core.c
> > > > > > > +++ b/drivers/hid/usbhid/hid-core.c
> > > > > > > @@ -95,6 +95,19 @@ 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_and_clear_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 = jiffies +
> > > > > > > + msecs_to_jiffies(50);
> > > > > > > + }
> > > > > > > }
> > > > > > > }
> > > > > > > spin_unlock_irqrestore(&usbhid->lock, flags);
> > > > > > > @@ -280,7 +293,8 @@ 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)) {
> > > > > > > + if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl) &&
> > > > > > > + time_after(jiffies, usbhid->input_start_time)) {
> > > > > >
> > > > > > Are we worried about jiffies overflowing (32-bit@1000Hz is "only" 49.7 days...)
> > > > > >
> > > > >
> > > > > time_after() is overflow-safe. That is why it is used and jiffies is
> > > > > not compared directly.
> > > >
> > > > Well, it is overflow safe, but still can not measure more than 50 days,
> > > > so if you have a device open for 50+ days there will be a 50msec gap
> > > > where it may lose events.
> > > >
> > >
> > > Or you could explicitly use 64-bit jiffies.
> >
> > Indeed.
> >
> > Jiri, Benjamin, do you have preference between jiffies64 and ktime_t? I
> > guess jiffies64 is a tiny bit less expensive.
>
> If I would be writing the code, I'd use ktime_t, because I personally like
> that abstraction more :) But either variant works for me.
I don't have a strong opinion on either variant. Feel free to use
whatever you like the most.
Cheers,
Benjamin
>
> Thanks!
>
> --
> Jiri Kosina
> SUSE Labs
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-02 9:14 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-29 19:59 [PATCH] HID: usbhid: do not sleep when opening device Dmitry Torokhov
2020-05-29 20:14 ` Guenter Roeck
2020-05-29 20:33 ` Dmitry Torokhov
2020-05-29 20:44 ` Guenter Roeck
2020-05-29 23:50 ` Nicolas Boichat
2020-05-30 0:48 ` Guenter Roeck
2020-05-30 1:09 ` Dmitry Torokhov
2020-05-30 1:22 ` Guenter Roeck
2020-05-30 1:34 ` Dmitry Torokhov
2020-06-01 17:13 ` Jiri Kosina
2020-06-02 9:14 ` Benjamin Tissoires
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).