| From 972e6a993f278b416a8ee3ec65475724fc36feb2 Mon Sep 17 00:00:00 2001 |
| From: Alan Stern <stern@rowland.harvard.edu> |
| Date: Wed, 23 Mar 2016 12:17:09 -0400 |
| Subject: HID: usbhid: fix inconsistent reset/resume/reset-resume behavior |
| |
| From: Alan Stern <stern@rowland.harvard.edu> |
| |
| commit 972e6a993f278b416a8ee3ec65475724fc36feb2 upstream. |
| |
| The usbhid driver has inconsistently duplicated code in its post-reset, |
| resume, and reset-resume pathways. |
| |
| reset-resume doesn't check HID_STARTED before trying to |
| restart the I/O queues. |
| |
| resume fails to clear the HID_SUSPENDED flag if HID_STARTED |
| isn't set. |
| |
| resume calls usbhid_restart_queues() with usbhid->lock held |
| and the others call it without holding the lock. |
| |
| The first item in particular causes a problem following a reset-resume |
| if the driver hasn't started up its I/O. URB submission fails because |
| usbhid->urbin is NULL, and this triggers an unending reset-retry loop. |
| |
| This patch fixes the problem by creating a new subroutine, |
| hid_restart_io(), to carry out all the common activities. It also |
| adds some checks that were missing in the original code: |
| |
| After a reset, there's no need to clear any halted endpoints. |
| |
| After a resume, if a reset is pending there's no need to |
| restart any I/O until the reset is finished. |
| |
| After a resume, if the interrupt-IN endpoint is halted there's |
| no need to submit the input URB until the halt has been |
| cleared. |
| |
| Signed-off-by: Alan Stern <stern@rowland.harvard.edu> |
| Reported-by: Daniel Fraga <fragabr@gmail.com> |
| Tested-by: Daniel Fraga <fragabr@gmail.com> |
| Signed-off-by: Jiri Kosina <jkosina@suse.cz> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/hid/usbhid/hid-core.c | 73 +++++++++++++++++++++--------------------- |
| 1 file changed, 37 insertions(+), 36 deletions(-) |
| |
| --- a/drivers/hid/usbhid/hid-core.c |
| +++ b/drivers/hid/usbhid/hid-core.c |
| @@ -940,14 +940,6 @@ static int usbhid_output_raw_report(stru |
| return ret; |
| } |
| |
| -static void usbhid_restart_queues(struct usbhid_device *usbhid) |
| -{ |
| - if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)) |
| - usbhid_restart_out_queue(usbhid); |
| - if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl)) |
| - usbhid_restart_ctrl_queue(usbhid); |
| -} |
| - |
| static void hid_free_buffers(struct usb_device *dev, struct hid_device *hid) |
| { |
| struct usbhid_device *usbhid = hid->driver_data; |
| @@ -1376,6 +1368,37 @@ static void hid_cease_io(struct usbhid_d |
| usb_kill_urb(usbhid->urbout); |
| } |
| |
| +static void hid_restart_io(struct hid_device *hid) |
| +{ |
| + struct usbhid_device *usbhid = hid->driver_data; |
| + int clear_halt = test_bit(HID_CLEAR_HALT, &usbhid->iofl); |
| + int reset_pending = test_bit(HID_RESET_PENDING, &usbhid->iofl); |
| + |
| + spin_lock_irq(&usbhid->lock); |
| + clear_bit(HID_SUSPENDED, &usbhid->iofl); |
| + usbhid_mark_busy(usbhid); |
| + |
| + if (clear_halt || reset_pending) |
| + schedule_work(&usbhid->reset_work); |
| + usbhid->retry_delay = 0; |
| + spin_unlock_irq(&usbhid->lock); |
| + |
| + if (reset_pending || !test_bit(HID_STARTED, &usbhid->iofl)) |
| + return; |
| + |
| + if (!clear_halt) { |
| + if (hid_start_in(hid) < 0) |
| + hid_io_error(hid); |
| + } |
| + |
| + spin_lock_irq(&usbhid->lock); |
| + if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)) |
| + usbhid_restart_out_queue(usbhid); |
| + if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl)) |
| + usbhid_restart_ctrl_queue(usbhid); |
| + spin_unlock_irq(&usbhid->lock); |
| +} |
| + |
| /* Treat USB reset pretty much the same as suspend/resume */ |
| static int hid_pre_reset(struct usb_interface *intf) |
| { |
| @@ -1425,14 +1448,14 @@ static int hid_post_reset(struct usb_int |
| return 1; |
| } |
| |
| + /* No need to do another reset or clear a halted endpoint */ |
| spin_lock_irq(&usbhid->lock); |
| clear_bit(HID_RESET_PENDING, &usbhid->iofl); |
| + clear_bit(HID_CLEAR_HALT, &usbhid->iofl); |
| spin_unlock_irq(&usbhid->lock); |
| hid_set_idle(dev, intf->cur_altsetting->desc.bInterfaceNumber, 0, 0); |
| - status = hid_start_in(hid); |
| - if (status < 0) |
| - hid_io_error(hid); |
| - usbhid_restart_queues(usbhid); |
| + |
| + hid_restart_io(hid); |
| |
| return 0; |
| } |
| @@ -1455,25 +1478,9 @@ void usbhid_put_power(struct hid_device |
| #ifdef CONFIG_PM |
| static int hid_resume_common(struct hid_device *hid, bool driver_suspended) |
| { |
| - struct usbhid_device *usbhid = hid->driver_data; |
| - int status; |
| - |
| - spin_lock_irq(&usbhid->lock); |
| - clear_bit(HID_SUSPENDED, &usbhid->iofl); |
| - usbhid_mark_busy(usbhid); |
| - |
| - if (test_bit(HID_CLEAR_HALT, &usbhid->iofl) || |
| - test_bit(HID_RESET_PENDING, &usbhid->iofl)) |
| - schedule_work(&usbhid->reset_work); |
| - usbhid->retry_delay = 0; |
| - |
| - usbhid_restart_queues(usbhid); |
| - spin_unlock_irq(&usbhid->lock); |
| - |
| - status = hid_start_in(hid); |
| - if (status < 0) |
| - hid_io_error(hid); |
| + int status = 0; |
| |
| + hid_restart_io(hid); |
| if (driver_suspended && hid->driver && hid->driver->resume) |
| status = hid->driver->resume(hid); |
| return status; |
| @@ -1542,12 +1549,8 @@ static int hid_suspend(struct usb_interf |
| static int hid_resume(struct usb_interface *intf) |
| { |
| struct hid_device *hid = usb_get_intfdata (intf); |
| - struct usbhid_device *usbhid = hid->driver_data; |
| int status; |
| |
| - if (!test_bit(HID_STARTED, &usbhid->iofl)) |
| - return 0; |
| - |
| status = hid_resume_common(hid, true); |
| dev_dbg(&intf->dev, "resume status %d\n", status); |
| return 0; |
| @@ -1556,10 +1559,8 @@ static int hid_resume(struct usb_interfa |
| static int hid_reset_resume(struct usb_interface *intf) |
| { |
| struct hid_device *hid = usb_get_intfdata(intf); |
| - struct usbhid_device *usbhid = hid->driver_data; |
| int status; |
| |
| - clear_bit(HID_SUSPENDED, &usbhid->iofl); |
| status = hid_post_reset(intf); |
| if (status >= 0 && hid->driver && hid->driver->reset_resume) { |
| int ret = hid->driver->reset_resume(hid); |