| Message ID | 20200529195951.GA3767@dtor-ws (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID: usbhid: do not sleep when opening device | expand |
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
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.
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
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
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
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.
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
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.
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!
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 >
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 */
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(-)