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 <stern@rowland.harvard.edu>
Reported-by: Daniel Fraga <fragabr@gmail.com>
Tested-by: Daniel Fraga <fragabr@gmail.com>
CC: <stable@vger.kernel.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index ad71160b9ea4341309030db0aea3aa2213ad71d3..ae83af649a607f67239f1a64bf45dd4b5770cc7d 100644 (file)
--- 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);