From d3132792285859253c466354fd8d54d1fe0ba786 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Tue, 9 Jun 2020 21:38:24 -0700 Subject: 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. Reported-by: Nicolas Boichat Signed-off-by: Dmitry Torokhov Reviewed-by: Guenter Roeck Signed-off-by: Jiri Kosina --- drivers/hid/usbhid/hid-core.c | 53 +++++++++++++++++++++++-------------------- drivers/hid/usbhid/usbhid.h | 2 ++ 2 files changed, 31 insertions(+), 24 deletions(-) (limited to 'drivers') diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 17a638f15082..1235288b65bf 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -26,6 +26,7 @@ #include #include #include +#include #include @@ -95,6 +96,18 @@ static int hid_start_in(struct hid_device *hid) set_bit(HID_NO_BANDWIDTH, &usbhid->iofl); } else { clear_bit(HID_NO_BANDWIDTH, &usbhid->iofl); + + if (test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) { + /* + * In case events are generated while nobody was + * listening, some are released when the device + * is re-opened. Wait 50 msec for the queue to + * empty before allowing events to go through + * hid. + */ + usbhid->input_start_time = + ktime_add_ms(ktime_get_coarse(), 50); + } } } spin_unlock_irqrestore(&usbhid->lock, flags); @@ -280,20 +293,23 @@ static void hid_irq_in(struct urb *urb) if (!test_bit(HID_OPENED, &usbhid->iofl)) break; usbhid_mark_busy(usbhid); - if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) { - hid_input_report(urb->context, HID_INPUT_REPORT, - urb->transfer_buffer, - urb->actual_length, 1); - /* - * autosuspend refused while keys are pressed - * because most keyboards don't wake up when - * a key is released - */ - if (hid_check_keys_pressed(hid)) - set_bit(HID_KEYS_PRESSED, &usbhid->iofl); - else - clear_bit(HID_KEYS_PRESSED, &usbhid->iofl); + if (test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) { + if (ktime_before(ktime_get_coarse(), + usbhid->input_start_time)) + break; + clear_bit(HID_RESUME_RUNNING, &usbhid->iofl); } + hid_input_report(urb->context, HID_INPUT_REPORT, + urb->transfer_buffer, urb->actual_length, 1); + /* + * autosuspend refused while keys are pressed + * because most keyboards don't wake up when + * a key is released + */ + if (hid_check_keys_pressed(hid)) + set_bit(HID_KEYS_PRESSED, &usbhid->iofl); + else + clear_bit(HID_KEYS_PRESSED, &usbhid->iofl); break; case -EPIPE: /* stall */ usbhid_mark_busy(usbhid); @@ -720,17 +736,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); - Done: mutex_unlock(&usbhid->mutex); return res; diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h index 75fe85d3d27a..c6ad684d099a 100644 --- a/drivers/hid/usbhid/usbhid.h +++ b/drivers/hid/usbhid/usbhid.h @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -83,6 +84,7 @@ struct usbhid_device { struct mutex mutex; /* start/stop/open/close */ spinlock_t lock; /* fifo spinlock */ unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */ + ktime_t input_start_time; /* When to start handling input */ struct timer_list io_retry; /* Retry timer */ unsigned long stop_retry; /* Time to give up, in jiffies */ unsigned int retry_delay; /* Delay length in ms */ -- cgit v1.2.3 From 8e9ddbde9ddbba74c86b0c5f8eefb26192b3242e Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Wed, 10 Jun 2020 13:31:01 +0100 Subject: HID: usbhid: remove redundant assignment to variable retval The variable retval is being initialized with a value that is never read and it is being updated later with a new value. The initialization is redundant and can be removed. Addresses-Coverity: ("Unused value") Signed-off-by: Colin Ian King Signed-off-by: Jiri Kosina --- drivers/hid/usbhid/hid-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 1235288b65bf..492dd641a25d 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1672,7 +1672,7 @@ struct usb_interface *usbhid_find_interface(int minor) static int __init hid_init(void) { - int retval = -ENOMEM; + int retval; retval = hid_quirks_init(quirks_param, BUS_USB, MAX_USBHID_BOOT_QUIRKS); if (retval) -- cgit v1.2.3 From 1ad273d6ff2d3af6ce3a4c8bd81a2ba10151ae0e Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Tue, 9 Jun 2020 16:03:19 +1000 Subject: HID: input: do not run GET_REPORT unless there's a Resolution Multiplier hid-multitouch currently runs GET_REPORT for Contact Max and again to retrieve the Win8 blob. If both are within the same report, the Resolution Multiplier code calls GET_FEATURE again and this time, possibly due to timing, it causes the ILITEK-TP device interpret the GET_FEATURE as an instruction to change the mode and effectively stop the device from functioning as expected. Notably: the device doesn't even have a Resolution Multiplier so it shouldn't be affected by any of this at all. Fix this by making sure we only execute GET_REPORT if there is a Resolution Multiplier in the respective report. Where the HID_QUIRK_NO_INIT_REPORTS field is set we just bail out immediately. This shouldn't be triggered by any real device anyway. Signed-off-by: Peter Hutterer Tested-by: Wen He Signed-off-by: Jiri Kosina --- drivers/hid/hid-input.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) (limited to 'drivers') diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index dea9cc65bf80..c8633beae260 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1560,21 +1560,12 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid, { struct hid_usage *usage; bool update_needed = false; + bool get_report_completed = false; int i, j; if (report->maxfield == 0) return false; - /* - * If we have more than one feature within this report we - * need to fill in the bits from the others before we can - * overwrite the ones for the Resolution Multiplier. - */ - if (report->maxfield > 1) { - hid_hw_request(hid, report, HID_REQ_GET_REPORT); - hid_hw_wait(hid); - } - for (i = 0; i < report->maxfield; i++) { __s32 value = use_logical_max ? report->field[i]->logical_maximum : @@ -1593,6 +1584,25 @@ static bool __hidinput_change_resolution_multipliers(struct hid_device *hid, if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER) continue; + /* + * If we have more than one feature within this + * report we need to fill in the bits from the + * others before we can overwrite the ones for the + * Resolution Multiplier. + * + * But if we're not allowed to read from the device, + * we just bail. Such a device should not exist + * anyway. + */ + if (!get_report_completed && report->maxfield > 1) { + if (hid->quirks & HID_QUIRK_NO_INIT_REPORTS) + return update_needed; + + hid_hw_request(hid, report, HID_REQ_GET_REPORT); + hid_hw_wait(hid); + get_report_completed = true; + } + report->field[i]->value[j] = value; update_needed = true; } -- cgit v1.2.3 From 4f57cace81438cc873a96f9f13f08298815c9b51 Mon Sep 17 00:00:00 2001 From: Grant Likely Date: Fri, 10 Jul 2020 16:19:39 +0100 Subject: HID: input: Fix devices that return multiple bytes in battery report Some devices, particularly the 3DConnexion Spacemouse wireless 3D controllers, return more than just the battery capacity in the battery report. The Spacemouse devices return an additional byte with a device specific field. However, hidinput_query_battery_capacity() only requests a 2 byte transfer. When a spacemouse is connected via USB (direct wire, no wireless dongle) and it returns a 3 byte report instead of the assumed 2 byte battery report the larger transfer confuses and frightens the USB subsystem which chooses to ignore the transfer. Then after 2 seconds assume the device has stopped responding and reset it. This can be reproduced easily by using a wired connection with a wireless spacemouse. The Spacemouse will enter a loop of resetting every 2 seconds which can be observed in dmesg. This patch solves the problem by increasing the transfer request to 4 bytes instead of 2. The fix isn't particularly elegant, but it is simple and safe to backport to stable kernels. A further patch will follow to more elegantly handle battery reports that contain additional data. Signed-off-by: Grant Likely Cc: Darren Hart Cc: Jiri Kosina Cc: Benjamin Tissoires Cc: stable@vger.kernel.org Tested-by: Darren Hart Signed-off-by: Jiri Kosina --- drivers/hid/hid-input.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index c8633beae260..b8eabf206e74 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -350,13 +350,13 @@ static int hidinput_query_battery_capacity(struct hid_device *dev) u8 *buf; int ret; - buf = kmalloc(2, GFP_KERNEL); + buf = kmalloc(4, GFP_KERNEL); if (!buf) return -ENOMEM; - ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2, + ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 4, dev->battery_report_type, HID_REQ_GET_REPORT); - if (ret != 2) { + if (ret < 2) { kfree(buf); return -ENODATA; } -- cgit v1.2.3