[PATCH 0/3 v2] usb/hid-core: drain URB queue when going to suspend

[PATCH 0/3 v2] usb/hid-core: drain URB queue when going to suspend

[Daniel Kurtz]

In some situations, trying to suspend a laptop with an attached USB keyboard
would fail if both NumLock and CapsLock LEDs were lit.
This was due to a race condition between asynchronously submitted
LED-manipulating CTRL URBs and the suspend process.

This is a different solution to the same problem highlighted here:
https://lkml.org/lkml/2011/9/2/391

These patches are against Jiri's hid/for-next branch.

Daniel Kurtz (3):
  HID: usbhid: remove LED_ON
  HID: usbhid: hid-core: submit queued urbs before suspend
  HID: usbhid: defer LED setting to a workqueue

Differences since v1:
  * Rebase on hid/for-next
  * Solve race with usbhid_stop() [reported by Oliver Neukum]

 drivers/hid/hid-input.c       |   42 +++++++
 drivers/hid/usbhid/hid-core.c |  241 ++++++++++++++++++++++++-----------------
 drivers/hid/usbhid/usbhid.h   |    3 +-
 include/linux/hid.h           |    2 +
 4 files changed, 187 insertions(+), 101 deletions(-)

-- 
1.7.3.1

[PATCH 1/3] HID: usbhid: remove LED_ON

[Daniel Kurtz]

LED_ON was defined in the original version of the hid-core autosuspend patch.
However, during review, the setting and clearing of it was redone
using ledcount.  The test was left in accidentally.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/hid/usbhid/hid-core.c |   10 ----------
 drivers/hid/usbhid/usbhid.h   |    1 -
 2 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b403fce..6606134 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1367,16 +1367,6 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
 			return -EIO;
 	}
 
-	if (!ignoreled && PMSG_IS_AUTO(message)) {
-		spin_lock_irq(&usbhid->lock);
-		if (test_bit(HID_LED_ON, &usbhid->iofl)) {
-			spin_unlock_irq(&usbhid->lock);
-			usbhid_mark_busy(usbhid);
-			return -EBUSY;
-		}
-		spin_unlock_irq(&usbhid->lock);
-	}
-
 	hid_cancel_delayed_stuff(usbhid);
 	hid_cease_io(usbhid);
 
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 1673cac..2d8957c 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -55,7 +55,6 @@ struct usb_interface *usbhid_find_interface(int minor);
 #define HID_STARTED		8
 #define HID_REPORTED_IDLE	9
 #define HID_KEYS_PRESSED	10
-#define HID_LED_ON		11
 
 /*
  * USB-specific HID struct, to be pointed to
-- 
1.7.3.1

[PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

[Daniel Kurtz]

If any userspace program has opened a keyboard device, the input core
de-activates the keyboard's LEDs upon suspend().  It does this by sending
individual EV_LED[LED_X]=0 events to the underlying device driver by
directly calling the driver's registered event() handler.

The usb-hid driver event() handler processes each request by immediately
attempting to submit a CTRL URB to turn off the LED.  USB URB submission
is asynchronous.  First the URB is added to the head of the ctrl queue.
Then, if the CTRL_RUNNING flag is false, the URB is submitted immediately
(and CTRL_RUNNING is set).  If the CTRL_RUNNING flag was already true,
then the newly queued URB is submitted in the ctrl completion handler when
all previously submitted URBs have completed.  When all queued URBs have
been submitted, the completion handler clears the CTRL_RUNNING flag.

In the 2-LED suspend case, at input suspend(), 2 LED event CTRL URBs get
queued, with only the first actually submitted.  Soon after input
suspend() handler finishes, the usb-hid suspend() handler gets called.
Since this is NOT a PM_EVENT_AUTO suspend, the handler sets
REPORTED_IDLE, then waits for io to complete.

Unfortunately, this usually happens while the first LED request is
actually still being processed.  Thus when the completion handler tries
to submit the second LED request it fails, since REPORTED_IDLE is
already set!  This REPORTED_IDLE check failure causes the completion
handler to complete, however without clearing the CTRL_RUNNING flag.
This, in turn, means that the suspend() handler's wait_io() condition
is never satisfied, and instead it times out after 10 seconds, aborting
the original system suspend.

This patch changes the behavior to the following:
  (1) allow completion handler to finish submitting all queued URBs, even if
      REPORTED_IDLE is set.  This guarantees that all URBs queued before the
      hid-core suspend() call will be submitted before the system is
      suspended.
  (2) if REPORTED_IDLE is set and the URB queue is empty, queue, but
      don't submit, new URB submission requests.  These queued requests get
      submitted when resume() flushes the URB queue. This is similar to the
      existing behavior, however, any requests that arrive while the queue is
      not yet empty will still get submitted before suspend.
  (3) set the RUNNING flag when flushing the URB queue in resume().
      This keeps URBs that were queued in (2) from colliding with any new
      URBs that are being submitted during the resume process.  The new URB
      submission requests upon resume get properly queued behind the ones
      being flushed instead of the current situation where they collide,
      causing memory corruption and oopses.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/hid/usbhid/hid-core.c |  184 +++++++++++++++++++++++------------------
 1 files changed, 105 insertions(+), 79 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 6606134..719f6b0 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -197,16 +197,24 @@ static int usbhid_restart_out_queue(struct usbhid_device *usbhid)
 {
 	struct hid_device *hid = usb_get_intfdata(usbhid->intf);
 	int kicked;
+	int r;
 
 	if (!hid)
 		return 0;
 
 	if ((kicked = (usbhid->outhead != usbhid->outtail))) {
 		dbg("Kicking head %d tail %d", usbhid->outhead, usbhid->outtail);
+
+		r = usb_autopm_get_interface_async(usbhid->intf);
+		if (r < 0)
+			return r;
+		/* Asynchronously flush queue. */
+		set_bit(HID_OUT_RUNNING, &usbhid->iofl);
 		if (hid_submit_out(hid)) {
 			clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
-			wake_up(&usbhid->wait);
+			usb_autopm_put_interface_async(usbhid->intf);
 		}
+		wake_up(&usbhid->wait);
 	}
 	return kicked;
 }
@@ -215,6 +223,7 @@ static int usbhid_restart_ctrl_queue(struct usbhid_device *usbhid)
 {
 	struct hid_device *hid = usb_get_intfdata(usbhid->intf);
 	int kicked;
+	int r;
 
 	WARN_ON(hid == NULL);
 	if (!hid)
@@ -222,10 +231,17 @@ static int usbhid_restart_ctrl_queue(struct usbhid_device *usbhid)
 
 	if ((kicked = (usbhid->ctrlhead != usbhid->ctrltail))) {
 		dbg("Kicking head %d tail %d", usbhid->ctrlhead, usbhid->ctrltail);
+
+		r = usb_autopm_get_interface_async(usbhid->intf);
+		if (r < 0)
+			return r;
+		/* Asynchronously flush queue. */
+		set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
 		if (hid_submit_ctrl(hid)) {
 			clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
-			wake_up(&usbhid->wait);
+			usb_autopm_put_interface_async(usbhid->intf);
 		}
+		wake_up(&usbhid->wait);
 	}
 	return kicked;
 }
@@ -304,30 +320,21 @@ static int hid_submit_out(struct hid_device *hid)
 	report = usbhid->out[usbhid->outtail].report;
 	raw_report = usbhid->out[usbhid->outtail].raw_report;
 
-	r = usb_autopm_get_interface_async(usbhid->intf);
-	if (r < 0)
-		return -1;
+	usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
+						 1 + (report->id > 0);
+	usbhid->urbout->dev = hid_to_usb_dev(hid);
+	memcpy(usbhid->outbuf, raw_report,
+	       usbhid->urbout->transfer_buffer_length);
+	kfree(raw_report);
 
-	/*
-	 * if the device hasn't been woken, we leave the output
-	 * to resume()
-	 */
-	if (!test_bit(HID_REPORTED_IDLE, &usbhid->iofl)) {
-		usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) + 1 + (report->id > 0);
-		usbhid->urbout->dev = hid_to_usb_dev(hid);
-		memcpy(usbhid->outbuf, raw_report, usbhid->urbout->transfer_buffer_length);
-		kfree(raw_report);
-
-		dbg_hid("submitting out urb\n");
+	dbg_hid("submitting out urb\n");
 
-		if (usb_submit_urb(usbhid->urbout, GFP_ATOMIC)) {
-			hid_err(hid, "usb_submit_urb(out) failed\n");
-			usb_autopm_put_interface_async(usbhid->intf);
-			return -1;
-		}
-		usbhid->last_out = jiffies;
+	r = usb_submit_urb(usbhid->urbout, GFP_ATOMIC);
+	if (r < 0) {
+		hid_err(hid, "usb_submit_urb(out) failed: %d\n", r);
+		return r;
 	}
-
+	usbhid->last_out = jiffies;
 	return 0;
 }
 
@@ -343,50 +350,48 @@ static int hid_submit_ctrl(struct hid_device *hid)
 	raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report;
 	dir = usbhid->ctrl[usbhid->ctrltail].dir;
 
-	r = usb_autopm_get_interface_async(usbhid->intf);
-	if (r < 0)
-		return -1;
-	if (!test_bit(HID_REPORTED_IDLE, &usbhid->iofl)) {
-		len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
-		if (dir == USB_DIR_OUT) {
-			usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
-			usbhid->urbctrl->transfer_buffer_length = len;
-			memcpy(usbhid->ctrlbuf, raw_report, len);
-			kfree(raw_report);
-		} else {
-			int maxpacket, padlen;
-
-			usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
-			maxpacket = usb_maxpacket(hid_to_usb_dev(hid), usbhid->urbctrl->pipe, 0);
-			if (maxpacket > 0) {
-				padlen = DIV_ROUND_UP(len, maxpacket);
-				padlen *= maxpacket;
-				if (padlen > usbhid->bufsize)
-					padlen = usbhid->bufsize;
-			} else
-				padlen = 0;
-			usbhid->urbctrl->transfer_buffer_length = padlen;
-		}
-		usbhid->urbctrl->dev = hid_to_usb_dev(hid);
-
-		usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;
-		usbhid->cr->bRequest = (dir == USB_DIR_OUT) ? HID_REQ_SET_REPORT : HID_REQ_GET_REPORT;
-		usbhid->cr->wValue = cpu_to_le16(((report->type + 1) << 8) | report->id);
-		usbhid->cr->wIndex = cpu_to_le16(usbhid->ifnum);
-		usbhid->cr->wLength = cpu_to_le16(len);
-
-		dbg_hid("submitting ctrl urb: %s wValue=0x%04x wIndex=0x%04x wLength=%u\n",
-			usbhid->cr->bRequest == HID_REQ_SET_REPORT ? "Set_Report" : "Get_Report",
-			usbhid->cr->wValue, usbhid->cr->wIndex, usbhid->cr->wLength);
-
-		if (usb_submit_urb(usbhid->urbctrl, GFP_ATOMIC)) {
-			usb_autopm_put_interface_async(usbhid->intf);
-			hid_err(hid, "usb_submit_urb(ctrl) failed\n");
-			return -1;
-		}
-		usbhid->last_ctrl = jiffies;
+	len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
+	if (dir == USB_DIR_OUT) {
+		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
+		usbhid->urbctrl->transfer_buffer_length = len;
+		memcpy(usbhid->ctrlbuf, raw_report, len);
+		kfree(raw_report);
+	} else {
+		int maxpacket, padlen;
+
+		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
+		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
+					  usbhid->urbctrl->pipe, 0);
+		if (maxpacket > 0) {
+			padlen = DIV_ROUND_UP(len, maxpacket);
+			padlen *= maxpacket;
+			if (padlen > usbhid->bufsize)
+				padlen = usbhid->bufsize;
+		} else
+			padlen = 0;
+		usbhid->urbctrl->transfer_buffer_length = padlen;
 	}
-
+	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
+
+	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;
+	usbhid->cr->bRequest = (dir == USB_DIR_OUT) ? HID_REQ_SET_REPORT :
+						      HID_REQ_GET_REPORT;
+	usbhid->cr->wValue = cpu_to_le16(((report->type + 1) << 8) |
+					 report->id);
+	usbhid->cr->wIndex = cpu_to_le16(usbhid->ifnum);
+	usbhid->cr->wLength = cpu_to_le16(len);
+
+	dbg_hid("submitting ctrl urb: %s wValue=0x%04x wIndex=0x%04x wLength=%u\n",
+		usbhid->cr->bRequest == HID_REQ_SET_REPORT ? "Set_Report" :
+							     "Get_Report",
+		usbhid->cr->wValue, usbhid->cr->wIndex, usbhid->cr->wLength);
+
+	r = usb_submit_urb(usbhid->urbctrl, GFP_ATOMIC);
+	if (r < 0) {
+		hid_err(hid, "usb_submit_urb(ctrl) failed: %d\n", r);
+		return r;
+	}
+	usbhid->last_ctrl = jiffies;
 	return 0;
 }
 
@@ -423,11 +428,8 @@ static void hid_irq_out(struct urb *urb)
 	else
 		usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);
 
-	if (usbhid->outhead != usbhid->outtail) {
-		if (hid_submit_out(hid)) {
-			clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
-			wake_up(&usbhid->wait);
-		}
+	if (usbhid->outhead != usbhid->outtail && !hid_submit_out(hid)) {
+		/* Successfully submitted next urb in queue */
 		spin_unlock_irqrestore(&usbhid->lock, flags);
 		return;
 	}
@@ -474,13 +476,9 @@ static void hid_ctrl(struct urb *urb)
 	else
 		usbhid->ctrltail = (usbhid->ctrltail + 1) & (HID_CONTROL_FIFO_SIZE - 1);
 
-	if (usbhid->ctrlhead != usbhid->ctrltail) {
-		if (hid_submit_ctrl(hid)) {
-			clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
-			wake_up(&usbhid->wait);
-		}
+	if (usbhid->ctrlhead != usbhid->ctrltail && !hid_submit_ctrl(hid)) {
+		/* Successfully submitted next urb in queue */
 		spin_unlock(&usbhid->lock);
-		usb_autopm_put_interface_async(usbhid->intf);
 		return;
 	}
 
@@ -515,9 +513,23 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
 		usbhid->out[usbhid->outhead].report = report;
 		usbhid->outhead = head;
 
+		/* Try to awake from autosuspend... */
+		if (usb_autopm_get_interface_async(usbhid->intf) < 0)
+			return;
+
+		/*
+		 * But if still suspended, leave urb enqueued, don't submit.
+		 * Submission will occur if/when resume() drains the queue.
+		 */
+		if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl))
+			return;
+
 		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
-			if (hid_submit_out(hid))
+			if (hid_submit_out(hid)) {
 				clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
+				usb_autopm_put_interface_async(usbhid->intf);
+			}
+			wake_up(&usbhid->wait);
 		} else {
 			/*
 			 * the queue is known to run
@@ -549,9 +561,23 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
 	usbhid->ctrl[usbhid->ctrlhead].dir = dir;
 	usbhid->ctrlhead = head;
 
+	/* Try to awake from autosuspend... */
+	if (usb_autopm_get_interface_async(usbhid->intf) < 0)
+		return;
+
+	/*
+	 * If already suspended, leave urb enqueued, but don't submit.
+	 * Submission will occur if/when resume() drains the queue.
+	 */
+	if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl))
+		return;
+
 	if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
-		if (hid_submit_ctrl(hid))
+		if (hid_submit_ctrl(hid)) {
 			clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
+			usb_autopm_put_interface_async(usbhid->intf);
+		}
+		wake_up(&usbhid->wait);
 	} else {
 		/*
 		 * the queue is known to run
-- 
1.7.3.1

Re: [PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

[Oliver Neukum]

Am Donnerstag, 17. November 2011, 12:23:49 schrieb Daniel Kurtz:
> Unfortunately, this usually happens while the first LED request is
> actually still being processed.  Thus when the completion handler tries
> to submit the second LED request it fails, since REPORTED_IDLE is
> already set!  This REPORTED_IDLE check failure causes the completion
> handler to complete, however without clearing the CTRL_RUNNING flag.
> This, in turn, means that the suspend() handler's wait_io() condition
> is never satisfied, and instead it times out after 10 seconds, aborting
> the original system suspend.
> 
> This patch changes the behavior to the following:
>   (1) allow completion handler to finish submitting all queued URBs, even if
>       REPORTED_IDLE is set.  This guarantees that all URBs queued before the
>       hid-core suspend() call will be submitted before the system is
>       suspended.

Hi,

why is this desirable? You'd want to requests to be executed at resumption.
A system suspend will alter the LED state anyway.

	Regards
		Oliver
-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 

Re: [PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

[Daniel Kurtz]

On Wed, Dec 14, 2011 at 3:55 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Donnerstag, 17. November 2011, 12:23:49 schrieb Daniel Kurtz:
>> Unfortunately, this usually happens while the first LED request is
>> actually still being processed.  Thus when the completion handler tries
>> to submit the second LED request it fails, since REPORTED_IDLE is
>> already set!  This REPORTED_IDLE check failure causes the completion
>> handler to complete, however without clearing the CTRL_RUNNING flag.
>> This, in turn, means that the suspend() handler's wait_io() condition
>> is never satisfied, and instead it times out after 10 seconds, aborting
>> the original system suspend.
>>
>> This patch changes the behavior to the following:
>>   (1) allow completion handler to finish submitting all queued URBs, even if
>>       REPORTED_IDLE is set.  This guarantees that all URBs queued before the
>>       hid-core suspend() call will be submitted before the system is
>>       suspended.
>
> Hi,
>
> why is this desirable? You'd want to requests to be executed at resumption.
> A system suspend will alter the LED state anyway.

This is how a system suspend 'alters' the LED state.  The input layer
sends those LED off commands down through HID as it is trying to
suspend.  We want to make sure the usbhid layer actually finishes
forwarding those requests on to the device before the system is
suspended.

Regards,
-Daniel

>
>        Regards
>                Oliver
> --
> - - -
> SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg)
> Maxfeldstraße 5
> 90409 Nürnberg
> Germany
> - - -
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

[Oliver Neukum]

Am Mittwoch, 14. Dezember 2011, 09:00:18 schrieb Daniel Kurtz:
> On Wed, Dec 14, 2011 at 3:55 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > Am Donnerstag, 17. November 2011, 12:23:49 schrieb Daniel Kurtz:
> >> Unfortunately, this usually happens while the first LED request is
> >> actually still being processed.  Thus when the completion handler tries
> >> to submit the second LED request it fails, since REPORTED_IDLE is
> >> already set!  This REPORTED_IDLE check failure causes the completion
> >> handler to complete, however without clearing the CTRL_RUNNING flag.
> >> This, in turn, means that the suspend() handler's wait_io() condition
> >> is never satisfied, and instead it times out after 10 seconds, aborting
> >> the original system suspend.
> >>
> >> This patch changes the behavior to the following:
> >>   (1) allow completion handler to finish submitting all queued URBs, even if
> >>       REPORTED_IDLE is set.  This guarantees that all URBs queued before the
> >>       hid-core suspend() call will be submitted before the system is
> >>       suspended.
> >
> > Hi,
> >
> > why is this desirable? You'd want to requests to be executed at resumption.
> > A system suspend will alter the LED state anyway.
> 
> This is how a system suspend 'alters' the LED state.  The input layer
> sends those LED off commands down through HID as it is trying to
> suspend.  We want to make sure the usbhid layer actually finishes
> forwarding those requests on to the device before the system is
> suspended.

You know the HID layer will send those commands. You don't know which
other commands may already be queued. So I don't know whether you really want
to execute them all while the system is asuspending.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

[Daniel Kurtz]

On Wed, Dec 14, 2011 at 5:14 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Mittwoch, 14. Dezember 2011, 09:00:18 schrieb Daniel Kurtz:
>> On Wed, Dec 14, 2011 at 3:55 PM, Oliver Neukum <oneukum@suse.de> wrote:
>> > Am Donnerstag, 17. November 2011, 12:23:49 schrieb Daniel Kurtz:
>> >> Unfortunately, this usually happens while the first LED request is
>> >> actually still being processed.  Thus when the completion handler tries
>> >> to submit the second LED request it fails, since REPORTED_IDLE is
>> >> already set!  This REPORTED_IDLE check failure causes the completion
>> >> handler to complete, however without clearing the CTRL_RUNNING flag.
>> >> This, in turn, means that the suspend() handler's wait_io() condition
>> >> is never satisfied, and instead it times out after 10 seconds, aborting
>> >> the original system suspend.
>> >>
>> >> This patch changes the behavior to the following:
>> >>   (1) allow completion handler to finish submitting all queued URBs, even if
>> >>       REPORTED_IDLE is set.  This guarantees that all URBs queued before the
>> >>       hid-core suspend() call will be submitted before the system is
>> >>       suspended.
>> >
>> > Hi,
>> >
>> > why is this desirable? You'd want to requests to be executed at resumption.
>> > A system suspend will alter the LED state anyway.
>>
>> This is how a system suspend 'alters' the LED state.  The input layer
>> sends those LED off commands down through HID as it is trying to
>> suspend.  We want to make sure the usbhid layer actually finishes
>> forwarding those requests on to the device before the system is
>> suspended.
>
> You know the HID layer will send those commands. You don't know which
> other commands may already be queued. So I don't know whether you really want
> to execute them all while the system is asuspending.

I'm confused.  The HID layer is sending which commands?

AFAICT, suspend works like this:
    On system suspend, the input.c suspend() gets called, which saves
off current LED value, and sends LED off events to all keyboards.  If
more than one LED was on, there will be multiple LED off requests.
The HID core immediately submits the first of them (assuming it isn't
in the middle of sending another Control request URB), and queues URBs
for the rest of them, to be submitted by the completion handler.
    Meanwhile, the hid-core.c suspend() gets called soon after the
input suspend() finishes.  This can happen while usbhid driver is
still processing the first LED off request, and therefore there are
still other URBs queued up.  However, since suspend() sets
REPORTED_IDLE, the completion handler can't actually submit them.
Therefore, suspend() times out while waiting for i/o to complete, and
as a result aborts the system suspend.

This patch fixes this problem, and synchronizes queued URBs with
system suspend by sending all urbs that were queued before the usbhid
suspend() was called.

Please let me know if I'm missing something?

-Dan

>
>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] HID: usbhid: hid-core: submit queued urbs before suspend

[Oliver Neukum]

Am Mittwoch, 14. Dezember 2011, 10:41:25 schrieben Sie:
> On Wed, Dec 14, 2011 at 5:14 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Mittwoch, 14. Dezember 2011, 09:00:18 schrieb Daniel Kurtz:
> >> On Wed, Dec 14, 2011 at 3:55 PM, Oliver Neukum <oneukum@suse.de> wrote:
> >> > Am Donnerstag, 17. November 2011, 12:23:49 schrieb Daniel Kurtz:
> >> >> Unfortunately, this usually happens while the first LED request is
> >> >> actually still being processed.  Thus when the completion handler tries
> >> >> to submit the second LED request it fails, since REPORTED_IDLE is
> >> >> already set!  This REPORTED_IDLE check failure causes the completion
> >> >> handler to complete, however without clearing the CTRL_RUNNING flag.
> >> >> This, in turn, means that the suspend() handler's wait_io() condition
> >> >> is never satisfied, and instead it times out after 10 seconds, aborting
> >> >> the original system suspend.
> >> >>
> >> >> This patch changes the behavior to the following:
> >> >>   (1) allow completion handler to finish submitting all queued URBs, even if
> >> >>       REPORTED_IDLE is set.  This guarantees that all URBs queued before the
> >> >>       hid-core suspend() call will be submitted before the system is
> >> >>       suspended.
> >> >
> >> > Hi,
> >> >
> >> > why is this desirable? You'd want to requests to be executed at resumption.
> >> > A system suspend will alter the LED state anyway.
> >>
> >> This is how a system suspend 'alters' the LED state.  The input layer
> >> sends those LED off commands down through HID as it is trying to
> >> suspend.  We want to make sure the usbhid layer actually finishes
> >> forwarding those requests on to the device before the system is
> >> suspended.
> >
> > You know the HID layer will send those commands. You don't know which
> > other commands may already be queued. So I don't know whether you really want
> > to execute them all while the system is asuspending.
> 
> I'm confused.  The HID layer is sending which commands?

That depends on the state of the system. The system may suspend
right after you pressed CAPS_LOCK for example.

> This patch fixes this problem, and synchronizes queued URBs with
> system suspend by sending all urbs that were queued before the usbhid
> suspend() was called.

Your solution is correct. But you have two choices to solve this problem.

a) submit and wait for all IO before you suspend
b) queue all requests and restart IO as you resume

You chose solution a. Is that the best solution?

	Regards
		Oliver

[PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

Defer LED setting action to a workqueue.
This is more likely to send all LED change events in a single URB.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/hid/hid-input.c       |   42 ++++++++++++++++++++++++++++++++++++
 drivers/hid/usbhid/hid-core.c |   47 +++++++++++++++++++++++++++++++---------
 drivers/hid/usbhid/usbhid.h   |    2 +
 include/linux/hid.h           |    2 +
 4 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 6e32526..9509684 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -867,6 +867,48 @@ int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int
 }
 EXPORT_SYMBOL_GPL(hidinput_find_field);
 
+struct hid_field *hidinput_get_led_field(struct hid_device *hid)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	int i, j;
+
+	list_for_each_entry(report,
+			    &hid->report_enum[HID_OUTPUT_REPORT].report_list,
+			    list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++)
+				if (field->usage[j].type == EV_LED)
+					return field;
+		}
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(hidinput_get_led_field);
+
+unsigned int hidinput_count_leds(struct hid_device *hid)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	int i, j;
+	unsigned int count = 0;
+
+	list_for_each_entry(report,
+			    &hid->report_enum[HID_OUTPUT_REPORT].report_list,
+			    list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++)
+				if (field->usage[j].type == EV_LED &&
+				    field->value[j])
+					count += 1;
+		}
+	}
+	return count;
+}
+EXPORT_SYMBOL_GPL(hidinput_count_leds);
+
 static int hidinput_open(struct input_dev *dev)
 {
 	struct hid_device *hid = input_get_drvdata(dev);
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 719f6b0..5bf91db 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -602,6 +602,30 @@ void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, uns
 }
 EXPORT_SYMBOL_GPL(usbhid_submit_report);
 
+/* Workqueue routine to send requests to change LEDs */
+static void hid_led(struct work_struct *work)
+{
+	struct usbhid_device *usbhid =
+		container_of(work, struct usbhid_device, led_work);
+	struct hid_device *hid = usbhid->hid;
+	struct hid_field *field;
+	unsigned long flags;
+
+	field = hidinput_get_led_field(hid);
+	if (!field) {
+		hid_warn(hid, "LED event field not found\n");
+		return;
+	}
+
+	spin_lock_irqsave(&usbhid->lock, flags);
+	if (!test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
+		usbhid->ledcount = hidinput_count_leds(hid);
+		hid_dbg(usbhid->hid, "New ledcount = %u\n", usbhid->ledcount);
+		__usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	}
+	spin_unlock_irqrestore(&usbhid->lock, flags);
+}
+
 static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
 {
 	struct hid_device *hid = input_get_drvdata(dev);
@@ -621,17 +645,15 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
 		return -1;
 	}
 
+	spin_lock_irqsave(&usbhid->lock, flags);
 	hid_set_field(field, offset, value);
-	if (value) {
-		spin_lock_irqsave(&usbhid->lock, flags);
-		usbhid->ledcount++;
-		spin_unlock_irqrestore(&usbhid->lock, flags);
-	} else {
-		spin_lock_irqsave(&usbhid->lock, flags);
-		usbhid->ledcount--;
-		spin_unlock_irqrestore(&usbhid->lock, flags);
-	}
-	usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	spin_unlock_irqrestore(&usbhid->lock, flags);
+
+	/*
+	 * Defer performing requested LED action.
+	 * This is more likely gather all LED changes into a single URB.
+	 */
+	schedule_work(&usbhid->led_work);
 
 	return 0;
 }
@@ -1126,7 +1148,7 @@ static void usbhid_stop(struct hid_device *hid)
 		return;
 
 	clear_bit(HID_STARTED, &usbhid->iofl);
-	spin_lock_irq(&usbhid->lock);	/* Sync with error handler */
+	spin_lock_irq(&usbhid->lock);	/* Sync with error and led handlers */
 	set_bit(HID_DISCONNECTED, &usbhid->iofl);
 	spin_unlock_irq(&usbhid->lock);
 	usb_kill_urb(usbhid->urbin);
@@ -1260,6 +1282,8 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
 	spin_lock_init(&usbhid->lock);
 
+	INIT_WORK(&usbhid->led_work, hid_led);
+
 	ret = hid_add_device(hid);
 	if (ret) {
 		if (ret != -ENODEV)
@@ -1292,6 +1316,7 @@ static void hid_cancel_delayed_stuff(struct usbhid_device *usbhid)
 {
 	del_timer_sync(&usbhid->io_retry);
 	cancel_work_sync(&usbhid->reset_work);
+	cancel_work_sync(&usbhid->led_work);
 }
 
 static void hid_cease_io(struct usbhid_device *usbhid)
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 2d8957c..cb8f703 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -96,6 +96,8 @@ struct usbhid_device {
 	struct work_struct reset_work;                                  /* Task context for resets */
 	wait_queue_head_t wait;						/* For sleeping */
 	int ledcount;							/* counting the number of active leds */
+
+	struct work_struct led_work;					/* Task context for setting LEDs */
 };
 
 #define	hid_to_usb_dev(hid_dev) \
diff --git a/include/linux/hid.h b/include/linux/hid.h
index deed5f9..0df7d62 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -711,6 +711,8 @@ extern void hidinput_disconnect(struct hid_device *);
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, int, int);
 int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
+struct hid_field *hidinput_get_led_field(struct hid_device *hid);
+unsigned int hidinput_count_leds(struct hid_device *hid);
 void hid_output_report(struct hid_report *report, __u8 *data);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
-- 
1.7.3.1

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Oliver Neukum]

Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
> Defer LED setting action to a workqueue.
> This is more likely to send all LED change events in a single URB.

Hi,

I hope I am looking at the correct version of this patch.
But as far as I can see the work for handling LEDs is not delayed
while a reset is going on. That is wrong.

	Regards
		Oliver

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

On Wed, Dec 14, 2011 at 4:01 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
>> Defer LED setting action to a workqueue.
>> This is more likely to send all LED change events in a single URB.
>
> Hi,
>
> I hope I am looking at the correct version of this patch.

Yes, that is the correct version.  It looks like I forgot to bump r in
the [PATCH] of the subject.

> But as far as I can see the work for handling LEDs is not delayed
> while a reset is going on. That is wrong.

Good catch, I think.  Your comment is a bit terse, so it is difficult to
tell exactly what you are recommending.  Perhaps something like the following?

+/* Workqueue routine to send requests to change LEDs */
+static void hid_led(struct work_struct *work)
+{
+       struct usbhid_device *usbhid =
+               container_of(work, struct usbhid_device, led_work);
+       struct hid_device *hid = usbhid->hid;
+       struct hid_field *field;
+       unsigned long flags;
+
+       field = hidinput_get_led_field(hid);
+       if (!field) {
+               hid_warn(hid, "LED event field not found\n");
+               return;
+       }
+
+       spin_lock_irqsave(&usbhid->lock, flags);
+       if (!test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
+           !test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
+               usbhid->ledcount = hidinput_count_leds(hid);
+               hid_dbg(usbhid->hid, "New ledcount = %u\n", usbhid->ledcount);
+               __usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+       }
+       spin_unlock_irqrestore(&usbhid->lock, flags);
+}


-Dan

>
>        Regards
>                Oliver
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Oliver Neukum]

Am Mittwoch, 14. Dezember 2011, 09:19:31 schrieb Daniel Kurtz:
> On Wed, Dec 14, 2011 at 4:01 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
> >> Defer LED setting action to a workqueue.
> >> This is more likely to send all LED change events in a single URB.
> >
> > Hi,
> >
> > I hope I am looking at the correct version of this patch.
> 
> Yes, that is the correct version.  It looks like I forgot to bump r in
> the [PATCH] of the subject.
> 
> > But as far as I can see the work for handling LEDs is not delayed
> > while a reset is going on. That is wrong.
> 
> Good catch, I think.  Your comment is a bit terse, so it is difficult to
> tell exactly what you are recommending.  Perhaps something like the following?

The problem done this way is what submits the work again after
it did nothing because a reset was pending.

	Regards
		Oliver

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

On Wed, Dec 14, 2011 at 4:01 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
>> Defer LED setting action to a workqueue.
>> This is more likely to send all LED change events in a single URB.
>
> Hi,
>
> I hope I am looking at the correct version of this patch.
> But as far as I can see the work for handling LEDs is not delayed
> while a reset is going on. That is wrong.

Actually, I'm not sure why this is wrong.  I find the reset handling
quite confusing in this driver.
Can you point out what will fail, and recommend how to fix it?

Thanks & Best Regards
-Daniel

>
>        Regards
>                Oliver
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Oliver Neukum]

Am Mittwoch, 14. Dezember 2011, 11:33:03 schrieb Daniel Kurtz:
> On Wed, Dec 14, 2011 at 4:01 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
> >> Defer LED setting action to a workqueue.
> >> This is more likely to send all LED change events in a single URB.
> >
> > Hi,
> >
> > I hope I am looking at the correct version of this patch.
> > But as far as I can see the work for handling LEDs is not delayed
> > while a reset is going on. That is wrong.
> 
> Actually, I'm not sure why this is wrong.  I find the reset handling
> quite confusing in this driver.
> Can you point out what will fail, and recommend how to fix it?

That is part of the workings of a USB driver. After you return from
pre_reset() you don't do IO to the device. After post_reset() the
device should be in its default state whose LED states won't match
what they ought to be.

	Regards
		Oliver

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

On Wed, Dec 14, 2011 at 7:22 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Mittwoch, 14. Dezember 2011, 11:33:03 schrieb Daniel Kurtz:
>> On Wed, Dec 14, 2011 at 4:01 PM, Oliver Neukum <oneukum@suse.de> wrote:
>> > Am Donnerstag, 17. November 2011, 12:23:50 schrieb Daniel Kurtz:
>> >> Defer LED setting action to a workqueue.
>> >> This is more likely to send all LED change events in a single URB.
>> >
>> > Hi,
>> >
>> > I hope I am looking at the correct version of this patch.
>> > But as far as I can see the work for handling LEDs is not delayed
>> > while a reset is going on. That is wrong.
>>
>> Actually, I'm not sure why this is wrong.  I find the reset handling
>> quite confusing in this driver.
>> Can you point out what will fail, and recommend how to fix it?
>
> That is part of the workings of a USB driver. After you return from
> pre_reset() you don't do IO to the device. After post_reset() the
> device should be in its default state whose LED states won't match
> what they ought to be.

Hi Oliver,

I'm sorry, I'm not seeing how using a workqueue to set the LEDs
changes anything with respect to how the driver deals with USB reset.

With or without a workqueue, LED control urbs are being queued and/or
submitted asynchronously to reset.  AFAICT, the only thing that
changes here is exactly when __usbhid_submit_report() is called.  In
the original case, this happens directly in the input event handler.
In the workqueue case, it happens in a system worker thread some short
time after the input handler finishes.

Could you please be more specific about what this patch breaks and
perhaps give some guidance on how to fix it?

Thanks and Best Regards,
-Daniel

>
>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Oliver Neukum]

Am Donnerstag, 15. Dezember 2011, 07:43:09 schrieb Daniel Kurtz:

Hi,

> I'm sorry, I'm not seeing how using a workqueue to set the LEDs
> changes anything with respect to how the driver deals with USB reset.

You are unfortunately right. This means that the driver currently is already
buggy.
 
> With or without a workqueue, LED control urbs are being queued and/or
> submitted asynchronously to reset.  AFAICT, the only thing that
> changes here is exactly when __usbhid_submit_report() is called.  In
> the original case, this happens directly in the input event handler.
> In the workqueue case, it happens in a system worker thread some short
> time after the input handler finishes.

The current driver kills the ctrl URB in cease_io() which is not enough
to prevent new IO.

> Could you please be more specific about what this patch breaks and
> perhaps give some guidance on how to fix it?

It breaks nothing. It just continues a bug and I assumed it was not present.
Basically the work queue must do nothing after pre_reset() and post_reset()
ought to rerun the work in case some request came down during that time.

	Regards
		Oliver

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

On Thu, Dec 15, 2011 at 5:01 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
> Am Donnerstag, 15. Dezember 2011, 07:43:09 schrieb Daniel Kurtz:
>
> Hi,
>
> > I'm sorry, I'm not seeing how using a workqueue to set the LEDs
> > changes anything with respect to how the driver deals with USB reset.
>
> You are unfortunately right. This means that the driver currently is already
> buggy.
>
> > With or without a workqueue, LED control urbs are being queued and/or
> > submitted asynchronously to reset.  AFAICT, the only thing that
> > changes here is exactly when __usbhid_submit_report() is called.  In
> > the original case, this happens directly in the input event handler.
> > In the workqueue case, it happens in a system worker thread some short
> > time after the input handler finishes.
>
> The current driver kills the ctrl URB in cease_io() which is not enough
> to prevent new IO.
>
> > Could you please be more specific about what this patch breaks and
> > perhaps give some guidance on how to fix it?
>
> It breaks nothing. It just continues a bug and I assumed it was not present.
> Basically the work queue must do nothing after pre_reset() and post_reset()
> ought to rerun the work in case some request came down during that time.

So, is this an Ack for this patchset?
Can we fix any existing races in later patches?

Regards,
-Daniel

>        Regards
>                Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Oliver Neukum]

Am Dienstag, 20. Dezember 2011, 11:12:37 schrieb Daniel Kurtz:
> > > Could you please be more specific about what this patch breaks and
> > > perhaps give some guidance on how to fix it?
> >
> > It breaks nothing. It just continues a bug and I assumed it was not present.
> > Basically the work queue must do nothing after pre_reset() and post_reset()
> > ought to rerun the work in case some request came down during that time.
> 
> So, is this an Ack for this patchset?
> Can we fix any existing races in later patches?

Very well. You are fixing a bad bug and not making matters worse.

Acked-by: Oliver Neukum <oneukum@suse.de>

	Regards
		Oliver
-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Jiri Kosina]

On Tue, 20 Dec 2011, Oliver Neukum wrote:

> > > > Could you please be more specific about what this patch breaks and
> > > > perhaps give some guidance on how to fix it?
> > >
> > > It breaks nothing. It just continues a bug and I assumed it was not present.
> > > Basically the work queue must do nothing after pre_reset() and post_reset()
> > > ought to rerun the work in case some request came down during that time.
> > 
> > So, is this an Ack for this patchset?
> > Can we fix any existing races in later patches?
> 
> Very well. You are fixing a bad bug and not making matters worse.
> 
> Acked-by: Oliver Neukum <oneukum@suse.de>

Thanks guys. Applied.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/3 v2] usb/hid-core: drain URB queue when going to suspend

[Daniel Kurtz]

On Thu, Nov 17, 2011 at 7:23 PM, Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> In some situations, trying to suspend a laptop with an attached USB keyboard
> would fail if both NumLock and CapsLock LEDs were lit.
> This was due to a race condition between asynchronously submitted
> LED-manipulating CTRL URBs and the suspend process.
>
> This is a different solution to the same problem highlighted here:
> https://lkml.org/lkml/2011/9/2/391
>
> These patches are against Jiri's hid/for-next branch.
>
> Daniel Kurtz (3):
>  HID: usbhid: remove LED_ON
>  HID: usbhid: hid-core: submit queued urbs before suspend
>  HID: usbhid: defer LED setting to a workqueue
>
> Differences since v1:
>  * Rebase on hid/for-next
>  * Solve race with usbhid_stop() [reported by Oliver Neukum]
>

Hi Jiri,

Any chance you can take a look at this patch set?

-Dan

>  drivers/hid/hid-input.c       |   42 +++++++
>  drivers/hid/usbhid/hid-core.c |  241 ++++++++++++++++++++++++-----------------
>  drivers/hid/usbhid/usbhid.h   |    3 +-
>  include/linux/hid.h           |    2 +
>  4 files changed, 187 insertions(+), 101 deletions(-)
>
> --
> 1.7.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3 v2] usb/hid-core: drain URB queue when going to suspend

[Jiri Kosina]

On Wed, 7 Dec 2011, Daniel Kurtz wrote:

> > In some situations, trying to suspend a laptop with an attached USB keyboard
> > would fail if both NumLock and CapsLock LEDs were lit.
> > This was due to a race condition between asynchronously submitted
> > LED-manipulating CTRL URBs and the suspend process.
> >
> > This is a different solution to the same problem highlighted here:
> > https://lkml.org/lkml/2011/9/2/391
> >
> > These patches are against Jiri's hid/for-next branch.
> >
> > Daniel Kurtz (3):
> >  HID: usbhid: remove LED_ON
> >  HID: usbhid: hid-core: submit queued urbs before suspend
> >  HID: usbhid: defer LED setting to a workqueue
> >
> > Differences since v1:
> >  * Rebase on hid/for-next
> >  * Solve race with usbhid_stop() [reported by Oliver Neukum]
> >
> 
> Hi Jiri,
> 
> Any chance you can take a look at this patch set?

Hi Dan,

it's still in my queue, sorry for delay; it absolutely hasn't been lost, I 
just haven't gotten to it yet.

Oliver, did you have a chance to review it, please? I'd apprecite your 
eventual Ack on this.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH 0/3] usb/hid-core: drain URB queue when going to suspend

[Daniel Kurtz]

In some situations, trying to suspend a laptop with an attached USB keyboard
would fail if both NumLock and CapsLock LEDs were lit.
This was due to a race condition between asynchronously submitted
LED-manipulating CTRL URBs and the suspend process.

This is a different solution to the same problem highlighted here:
https://lkml.org/lkml/2011/9/2/391

These patches are against Jiri's hid/master branch.

Daniel Kurtz (3):
  HID: usbhid: remove LED_ON
  HID: usbhid: hid-core: submit queued urbs before suspend
  HID: usbhid: defer LED setting to a workqueue

 drivers/hid/hid-input.c       |   42 +++++++
 drivers/hid/usbhid/hid-core.c |  237 ++++++++++++++++++++++++-----------------
 drivers/hid/usbhid/usbhid.h   |    3 +-
 include/linux/hid.h           |    2 +
 4 files changed, 184 insertions(+), 100 deletions(-)

-- 
1.7.3.1

[PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Daniel Kurtz]

Defer LED setting action to a workqueue.
This is more likely to perform all LED change events in a single URB.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/hid/hid-input.c       |   42 ++++++++++++++++++++++++++++++++++++++++
 drivers/hid/usbhid/hid-core.c |   43 +++++++++++++++++++++++++++++++---------
 drivers/hid/usbhid/usbhid.h   |    2 +
 include/linux/hid.h           |    2 +
 4 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f333139..ca2075d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -861,6 +861,48 @@ int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int
 }
 EXPORT_SYMBOL_GPL(hidinput_find_field);
 
+struct hid_field *hidinput_get_led_field(struct hid_device *hid)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	int i, j;
+
+	list_for_each_entry(report,
+			    &hid->report_enum[HID_OUTPUT_REPORT].report_list,
+			    list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++)
+				if (field->usage[j].type == EV_LED)
+					return field;
+		}
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(hidinput_get_led_field);
+
+unsigned int hidinput_count_leds(struct hid_device *hid)
+{
+	struct hid_report *report;
+	struct hid_field *field;
+	int i, j;
+	unsigned int count = 0;
+
+	list_for_each_entry(report,
+			    &hid->report_enum[HID_OUTPUT_REPORT].report_list,
+			    list) {
+		for (i = 0; i < report->maxfield; i++) {
+			field = report->field[i];
+			for (j = 0; j < field->maxusage; j++)
+				if (field->usage[j].type == EV_LED &&
+				    field->value[j])
+					count += 1;
+		}
+	}
+	return count;
+}
+EXPORT_SYMBOL_GPL(hidinput_count_leds);
+
 static int hidinput_open(struct input_dev *dev)
 {
 	struct hid_device *hid = input_get_drvdata(dev);
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 7f67cfa..e5a3e5c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -602,6 +602,28 @@ void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, uns
 }
 EXPORT_SYMBOL_GPL(usbhid_submit_report);
 
+/* Workqueue routine to send requests to change LEDs */
+static void hid_led(struct work_struct *work)
+{
+	struct usbhid_device *usbhid =
+		container_of(work, struct usbhid_device, led_work);
+	struct hid_device *hid = usbhid->hid;
+	struct hid_field *field;
+	unsigned long flags;
+
+	field = hidinput_get_led_field(hid);
+	if (!field) {
+		hid_warn(hid, "LED event field not found\n");
+		return;
+	}
+
+	spin_lock_irqsave(&usbhid->lock, flags);
+	usbhid->ledcount = hidinput_count_leds(hid);
+	hid_warn(hid, "%s: New ledcount = %u\n", __func__, usbhid->ledcount);
+	__usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	spin_unlock_irqrestore(&usbhid->lock, flags);
+}
+
 static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
 {
 	struct hid_device *hid = input_get_drvdata(dev);
@@ -621,17 +643,15 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
 		return -1;
 	}
 
+	spin_lock_irqsave(&usbhid->lock, flags);
 	hid_set_field(field, offset, value);
-	if (value) {
-		spin_lock_irqsave(&usbhid->lock, flags);
-		usbhid->ledcount++;
-		spin_unlock_irqrestore(&usbhid->lock, flags);
-	} else {
-		spin_lock_irqsave(&usbhid->lock, flags);
-		usbhid->ledcount--;
-		spin_unlock_irqrestore(&usbhid->lock, flags);
-	}
-	usbhid_submit_report(hid, field->report, USB_DIR_OUT);
+	spin_unlock_irqrestore(&usbhid->lock, flags);
+
+	/*
+	 * Defer performing requested LED action.
+	 * This is more likely gather all LED changes into a single URB.
+	 */
+	schedule_work(&usbhid->led_work);
 
 	return 0;
 }
@@ -1260,6 +1280,8 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
 	spin_lock_init(&usbhid->lock);
 
+	INIT_WORK(&usbhid->led_work, hid_led);
+
 	ret = hid_add_device(hid);
 	if (ret) {
 		if (ret != -ENODEV)
@@ -1292,6 +1314,7 @@ static void hid_cancel_delayed_stuff(struct usbhid_device *usbhid)
 {
 	del_timer_sync(&usbhid->io_retry);
 	cancel_work_sync(&usbhid->reset_work);
+	cancel_work_sync(&usbhid->led_work);
 }
 
 static void hid_cease_io(struct usbhid_device *usbhid)
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 2d8957c..cb8f703 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -96,6 +96,8 @@ struct usbhid_device {
 	struct work_struct reset_work;                                  /* Task context for resets */
 	wait_queue_head_t wait;						/* For sleeping */
 	int ledcount;							/* counting the number of active leds */
+
+	struct work_struct led_work;					/* Task context for setting LEDs */
 };
 
 #define	hid_to_usb_dev(hid_dev) \
diff --git a/include/linux/hid.h b/include/linux/hid.h
index deed5f9..0df7d62 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -711,6 +711,8 @@ extern void hidinput_disconnect(struct hid_device *);
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, int, int);
 int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
+struct hid_field *hidinput_get_led_field(struct hid_device *hid);
+unsigned int hidinput_count_leds(struct hid_device *hid);
 void hid_output_report(struct hid_report *report, __u8 *data);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
-- 
1.7.3.1

Re: [PATCH 3/3] HID: usbhid: defer LED setting to a workqueue

[Oliver Neukum]

Am Dienstag, 1. November 2011, 10:25:47 schrieb Daniel Kurtz:
> Defer LED setting action to a workqueue.
> This is more likely to perform all LED change events in a single URB.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/hid/hid-input.c       |   42 ++++++++++++++++++++++++++++++++++++++++
>  drivers/hid/usbhid/hid-core.c |   43 +++++++++++++++++++++++++++++++---------
>  drivers/hid/usbhid/usbhid.h   |    2 +
>  include/linux/hid.h           |    2 +

> @@ -1292,6 +1314,7 @@ static void hid_cancel_delayed_stuff(struct usbhid_device *usbhid)
>  {
>  	del_timer_sync(&usbhid->io_retry);
>  	cancel_work_sync(&usbhid->reset_work);
> +	cancel_work_sync(&usbhid->led_work);
>  }

Hi,

this seems to introduce a race in usbhid_stop() which might fail to kill the urb
used to change the LEDs.

	Regards
		Oliver