Re: [PATCH] HID: usbhid: do not sleep when opening device
On Mon, Jun 1, 2020 at 7:13 PM Jiri Kosina 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 > > > > > > > Signed-off-by: Dmitry Torokhov > > > > > > > --- > > > > > > > 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, > > > > > > > >iofl); > > > > > > > } else { > > > > > > > clear_bit(HID_NO_BANDWIDTH, > > > > > > > >iofl); > > > > > > > + > > > > > > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > > > > > > + >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(>lock, flags); > > > > > > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > > > > > > if (!test_bit(HID_OPENED, >iofl)) > > > > > > > break; > > > > > > > usbhid_mark_busy(usbhid); > > > > > > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) > > > > > > > { > > > > > > > + if (!test_bit(HID_RESUME_RUNNING, >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 >
Re: [PATCH] HID: usbhid: do not sleep when opening device
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 > > > > > > Signed-off-by: Dmitry Torokhov > > > > > > --- > > > > > > 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, > > > > > > >iofl); > > > > > > } else { > > > > > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > > > > > + > > > > > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > > > > > + >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(>lock, flags); > > > > > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > > > > > if (!test_bit(HID_OPENED, >iofl)) > > > > > > break; > > > > > > usbhid_mark_busy(usbhid); > > > > > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > > > > > + if (!test_bit(HID_RESUME_RUNNING, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
On Fri, May 29, 2020 at 06:22:49PM -0700, Guenter Roeck wrote: > On Fri, May 29, 2020 at 6:09 PM Dmitry Torokhov > 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 > > > wrote: > > > > > > > > On Sat, May 30, 2020 at 3:59 AM 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 > > > > > Signed-off-by: Dmitry Torokhov > > > > > --- > > > > > 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, > > > > > >iofl); > > > > > } else { > > > > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > > > > + > > > > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > > > > + >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(>lock, flags); > > > > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > > > > if (!test_bit(HID_OPENED, >iofl)) > > > > > break; > > > > > usbhid_mark_busy(usbhid); > > > > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > > > > + if (!test_bit(HID_RESUME_RUNNING, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
On Fri, May 29, 2020 at 6:09 PM Dmitry Torokhov 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 > > wrote: > > > > > > On Sat, May 30, 2020 at 3:59 AM 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 > > > > Signed-off-by: Dmitry Torokhov > > > > --- > > > > 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, > > > > >iofl); > > > > } else { > > > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > > > + > > > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > > > + >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(>lock, flags); > > > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > > > if (!test_bit(HID_OPENED, >iofl)) > > > > break; > > > > usbhid_mark_busy(usbhid); > > > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > > > + if (!test_bit(HID_RESUME_RUNNING, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
On Fri, May 29, 2020 at 05:48:26PM -0700, Guenter Roeck wrote: > On Fri, May 29, 2020 at 4:50 PM Nicolas Boichat wrote: > > > > On Sat, May 30, 2020 at 3:59 AM 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 > > > Signed-off-by: Dmitry Torokhov > > > --- > > > 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, >iofl); > > > } else { > > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > > + > > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > > + >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(>lock, flags); > > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > > if (!test_bit(HID_OPENED, >iofl)) > > > break; > > > usbhid_mark_busy(usbhid); > > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > > + if (!test_bit(HID_RESUME_RUNNING, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
On Fri, May 29, 2020 at 4:50 PM Nicolas Boichat wrote: > > On Sat, May 30, 2020 at 3:59 AM 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 > > Signed-off-by: Dmitry Torokhov > > --- > > 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, >iofl); > > } else { > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > + > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > + >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(>lock, flags); > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > if (!test_bit(HID_OPENED, >iofl)) > > break; > > usbhid_mark_busy(usbhid); > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > + if (!test_bit(HID_RESUME_RUNNING, >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, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
On Sat, May 30, 2020 at 3:59 AM 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 > Signed-off-by: Dmitry Torokhov > --- > 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, >iofl); > } else { > clear_bit(HID_NO_BANDWIDTH, >iofl); > + > + if (test_and_clear_bit(HID_RESUME_RUNNING, > + >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(>lock, flags); > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > if (!test_bit(HID_OPENED, >iofl)) > break; > usbhid_mark_busy(usbhid); > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > + if (!test_bit(HID_RESUME_RUNNING, >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, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
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 > > > Signed-off-by: Dmitry Torokhov > > > --- > > > 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, >iofl); > > > } else { > > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > > + > > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > > +>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(>lock, flags); > > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > > if (!test_bit(HID_OPENED, >iofl)) > > > break; > > > usbhid_mark_busy(usbhid); > > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > > + if (!test_bit(HID_RESUME_RUNNING, >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, >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 Thanks, Guenter
Re: [PATCH] HID: usbhid: do not sleep when opening device
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 > > Signed-off-by: Dmitry Torokhov > > --- > > 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, >iofl); > > } else { > > clear_bit(HID_NO_BANDWIDTH, >iofl); > > + > > + if (test_and_clear_bit(HID_RESUME_RUNNING, > > + >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(>lock, flags); > > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > > if (!test_bit(HID_OPENED, >iofl)) > > break; > > usbhid_mark_busy(usbhid); > > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > > + if (!test_bit(HID_RESUME_RUNNING, >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, >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
Re: [PATCH] HID: usbhid: do not sleep when opening device
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 > Signed-off-by: Dmitry Torokhov > --- > 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, >iofl); > } else { > clear_bit(HID_NO_BANDWIDTH, >iofl); > + > + if (test_and_clear_bit(HID_RESUME_RUNNING, > +>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(>lock, flags); > @@ -280,7 +293,8 @@ static void hid_irq_in(struct urb *urb) > if (!test_bit(HID_OPENED, >iofl)) > break; > usbhid_mark_busy(usbhid); > - if (!test_bit(HID_RESUME_RUNNING, >iofl)) { > + if (!test_bit(HID_RESUME_RUNNING, >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, >iofl); Then you might not need the added code in hid_start_in(). Thanks, Guenter > - clear_bit(HID_RESUME_RUNNING, >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