Patch "HID: usbhid: fix inconsistent reset/resume/reset-resume behavior" has been added to the 3.14-stable tree

PATCH [email protected] 1 participant 1 message Apr 18, 2016

Subsystems (3)

Stable branch HID USB HID/hidbp drivers (USB keyboards, mice, remote controls, ...)

lore

Share & tools

lore

[email protected]

author maintainer Applied

2016-04-18 01:25 UTC #0 lore raw

This is a note to let you know that I've just added the patch titled

HID: usbhid: fix inconsistent reset/resume/reset-resume behavior

to the 3.14-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
hid-usbhid-fix-inconsistent-reset-resume-reset-resume-behavior.patch
and it can be found in the queue-3.14 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From 972e6a993f278b416a8ee3ec65475724fc36feb2 Mon Sep 17 00:00:00 2001

From: Alan Stern <[email protected]>
Date: Wed, 23 Mar 2016 12:17:09 -0400
Subject: HID: usbhid: fix inconsistent reset/resume/reset-resume behavior

From: Alan Stern <[email protected]>

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 <[email protected]>

Reported-by Daniel Fraga <[email protected]>

Tested-by Daniel Fraga <[email protected]>

Signed-off-by Jiri Kosina <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/hid/usbhid/hid-core.c |   73 +++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

drivers/hid/usbhid/hid-core.c +37 ~~-36~~

@@ -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);

Patches currently in stable-queue which might be from [email protected] are

queue-3.14/hid-usbhid-fix-inconsistent-reset-resume-reset-resume-behavior.patch