From 972e6a993f278b416a8ee3ec65475724fc36feb2 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Wed, 23 Mar 2016 12:17:09 -0400 Subject: [PATCH] HID: usbhid: fix inconsistent reset/resume/reset-resume behavior 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 Reported-by: Daniel Fraga Tested-by: Daniel Fraga CC: Signed-off-by: Jiri Kosina --- drivers/hid/usbhid/hid-core.c | 73 ++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 36 deletions(-) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index ad71160b9ea4..ae83af649a60 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -951,14 +951,6 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count) 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; @@ -1404,6 +1396,37 @@ static void hid_cease_io(struct usbhid_device *usbhid) 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) { @@ -1453,14 +1476,14 @@ static int hid_post_reset(struct usb_interface *intf) 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; } @@ -1483,25 +1506,9 @@ void usbhid_put_power(struct hid_device *hid) #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; @@ -1570,12 +1577,8 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message) 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; @@ -1584,10 +1587,8 @@ static int hid_resume(struct usb_interface *intf) 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);