All of lore.kernel.org
 help / color / mirror / Atom feed
From: <gregkh@linuxfoundation.org>
To: stern@rowland.harvard.edu, fragabr@gmail.com,
	gregkh@linuxfoundation.org, jkosina@suse.cz
Cc: <stable@vger.kernel.org>, <stable-commits@vger.kernel.org>
Subject: Patch "HID: usbhid: fix inconsistent reset/resume/reset-resume behavior" has been added to the 3.14-stable tree
Date: Mon, 18 Apr 2016 10:25:29 +0900	[thread overview]
Message-ID: <1460942729216145@kroah.com> (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 <stable@vger.kernel.org> know about it.


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


Patches currently in stable-queue which might be from stern@rowland.harvard.edu are

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

                 reply	other threads:[~2016-04-18  1:25 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1460942729216145@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=fragabr@gmail.com \
    --cc=jkosina@suse.cz \
    --cc=stable-commits@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.