HID: usbhid: do not sleep when opening device
PATCH 1/1 HID: usbhid: do not sleep when opening device
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.
--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)
drivers/hid/usbhid/hid-core.c +15 -12
@@ -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; }
drivers/hid/usbhid/usbhid.h +1 -0
@@ -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 */
Signature · 5 lines
2.27.0.rc0.183.gde8f92d652-goog -- Dmitry
Quoted 53 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +15
-1@@ -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);
drivers/hid/usbhid/hid-core.c static int usbhid_open(struct hid_device *hid) · line 726
Quoted 10 lines
drivers/hid/usbhid/hid-core.c +0
-10- -/* -* 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
Quoted 22 lines
drivers/hid/usbhid/hid-core.c +0
-1-clear_bit (HID_RESUME_RUNNING , &usbhid->iofl);return res; }drivers/hid/usbhid/usbhid.h +1
-0@@ -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 */ Signature · 5 lines
2.27.0.rc0.183.gde8f92d652-goog -- Dmitry
Quoted 65 lines · Guenter Roeck
Quoted 53 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +15
-1@@ -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);drivers/hid/usbhid/hid-core.c static int usbhid_open(struct hid_device *hid) · line 726
Quoted 10 lines
drivers/hid/usbhid/hid-core.c +0
-10- -/* -* 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_timehere ?
Quoted 5 lines
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.
Signature · 1 line
Dmitry
Guenter Roeck
Reviewed-byQuoted 70 lines · Dmitry Torokhov
Quoted 69 lines · Guenter Roeck
Quoted 53 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +15
-1@@ -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);drivers/hid/usbhid/hid-core.c static int usbhid_open(struct hid_device *hid) · line 726
Quoted 10 lines
drivers/hid/usbhid/hid-core.c +0
-10- -/* -* 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_timehere ?if (res == 0) usbhid->input_start_time = jiffies + msecs_to_jiffies(50); clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
Quoted 7 lines
Quoted 1 line
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 inhid_start_in().
Ah yes, that makes sense.
Thanks,
Guenter
Quoted 41 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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);
drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1@@ -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...)
Quoted 39 lines
drivers/hid/usbhid/hid-core.c +0
-11hid_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; }drivers/hid/usbhid/usbhid.h +1
-1@@ -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
Quoted 44 lines · Nicolas Boichat
Quoted 41 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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)
Quoted 9 lines
drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1if (!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
Quoted 39 lines
Quoted 39 lines
drivers/hid/usbhid/hid-core.c +0
-11hid_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; }drivers/hid/usbhid/usbhid.h +1
-1@@ -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
Quoted 55 lines · Guenter Roeck
Quoted 44 lines · Nicolas Boichat
Quoted 41 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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)Quoted 9 lines
drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1if (!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...)
Quoted 2 lines
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.
Signature · 1 line
Dmitry
Quoted 57 lines · Dmitry Torokhov
Quoted 55 lines · Guenter Roeck
Quoted 53 lines · Nicolas Boichat
Quoted 41 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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);drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1@@ -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...)
Quoted 7 lines
Quoted 2 lines
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
Quoted 7 lines
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
Quoted 62 lines · Guenter Roeck
Quoted 60 lines · Dmitry Torokhov
Quoted 57 lines · Guenter Roeck
Quoted 44 lines · Nicolas Boichat
Quoted 41 lines · Dmitry Torokhov
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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)Quoted 9 lines
drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1if (!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.
Quoted 6 lines
Quoted 4 lines
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.
Signature · 1 line
Dmitry
Jiri Kosina
maintainerQuoted 58 lines · Dmitry Torokhov
Quoted 58 lines
Quoted 57 lines
Quoted 50 lines
Quoted 40 lines
Quoted 40 lines
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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)Quoted 9 lines
drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1if (!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...)
Quoted 2 lines
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.
Quoted 6 lines
Quoted 1 line
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!
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 63 lines · Jiri Kosina
Quoted 60 lines · Dmitry Torokhov
Quoted 59 lines
Quoted 50 lines
Quoted 50 lines
Quoted 49 lines
Quoted 40 lines
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 <[email protected]>Signed-off-by Dmitry Torokhov <[email protected]>--- drivers/hid/usbhid/hid-core.c | 27 +++++++++++++++------------ drivers/hid/usbhid/usbhid.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-)drivers/hid/usbhid/hid-core.c +13
-0@@ -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);drivers/hid/usbhid/hid-core.c static void hid_irq_in(struct urb *urb) · line 297
Quoted 6 lines
drivers/hid/usbhid/hid-core.c +2
-1@@ -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...)
Quoted 7 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 7 lines
Thanks!
--
Jiri Kosina
SUSE Labs