Re: usbhid control queue full, due to stuck control request

Re: usbhid control queue full, due to stuck control request

[David Fries]

From: Oliver Neukum <oliver@neukum.org>

Some devices do not react to a control request (seen on APC UPS's)
resulting in a slow stream of messages,
"generic-usb ... control queue full".
Therefore request need a timeout.

Signed-off-by: Oliver Neukum <oliver@neukum.org>
Duplicated the change from last_out to last_ctrl and verified.
Signed-off-by: David Fries <david@fries.net>
---

Oliver Neukum, this is based on your patch, do you agree with the
changes?

One possible problem with this fix is it drops the stalled URB
transfer.  That might pose a problem for one shot messages, possibly
hid_ctrl and hid_irq_out could resubmit the same urb when -ECONNRESET
is received, assuming the message content wasn't the reason it
timedout in the first place.
David Fries <david@fries.net>

 drivers/hid/usbhid/hid-core.c |   28 ++++++++++++++++++++++++++--
 drivers/hid/usbhid/usbhid.h   |    2 ++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 3c1fcb7..076a29a 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -318,6 +318,7 @@ static int hid_submit_out(struct hid_device *hid)
 			err_hid("usb_submit_urb(out) failed");
 			return -1;
 		}
+		usbhid->last_out = jiffies;
 	} else {
 		/*
 		 * queue work to wake up the device.
@@ -379,6 +380,7 @@ static int hid_submit_ctrl(struct hid_device *hid)
 			err_hid("usb_submit_urb(ctrl) failed");
 			return -1;
 		}
+		usbhid->last_ctrl = jiffies;
 	} else {
 		/*
 		 * queue work to wake up the device.
@@ -513,9 +515,20 @@ void __usbhid_submit_report(struct hid_device *hid, struct hid_report *report, u
 		usbhid->out[usbhid->outhead].report = report;
 		usbhid->outhead = head;
 
-		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl))
+		if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
 			if (hid_submit_out(hid))
 				clear_bit(HID_OUT_RUNNING, &usbhid->iofl);
+		} else {
+			/*
+			 * the queue is known to run
+			 * but an earlier request may be stuck
+			 * we may need to time out
+			 * no race because this is called under
+			 * spinlock
+			 */
+			if (time_after(jiffies, usbhid->last_out + HZ * 5))
+				usb_unlink_urb(usbhid->urbout);
+		}
 		return;
 	}
 
@@ -536,9 +549,20 @@ void __usbhid_submit_report(struct hid_device *hid, struct hid_report *report, u
 	usbhid->ctrl[usbhid->ctrlhead].dir = dir;
 	usbhid->ctrlhead = head;
 
-	if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl))
+	if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
 		if (hid_submit_ctrl(hid))
 			clear_bit(HID_CTRL_RUNNING, &usbhid->iofl);
+	} else {
+		/*
+		 * the queue is known to run
+		 * but an earlier request may be stuck
+		 * we may need to time out
+		 * no race because this is called under
+		 * spinlock
+		 */
+		if (time_after(jiffies, usbhid->last_ctrl + HZ * 5))
+			usb_unlink_urb(usbhid->urbctrl);
+	}
 }
 
 void usbhid_submit_report(struct hid_device *hid, struct hid_report *report, unsigned char dir)
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 08f505c..ec20400 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -80,12 +80,14 @@ struct usbhid_device {
 	unsigned char ctrlhead, ctrltail;                               /* Control fifo head & tail */
 	char *ctrlbuf;                                                  /* Control buffer */
 	dma_addr_t ctrlbuf_dma;                                         /* Control buffer dma */
+	unsigned long last_ctrl;						/* record of last output for timeouts */
 
 	struct urb *urbout;                                             /* Output URB */
 	struct hid_output_fifo out[HID_CONTROL_FIFO_SIZE];              /* Output pipe fifo */
 	unsigned char outhead, outtail;                                 /* Output pipe fifo head & tail */
 	char *outbuf;                                                   /* Output buffer */
 	dma_addr_t outbuf_dma;                                          /* Output buffer dma */
+	unsigned long last_out;							/* record of last output for timeouts */
 
 	spinlock_t lock;						/* fifo spinlock */
 	unsigned long iofl;                                             /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
-- 
1.5.6.5

Re: usbhid control queue full, due to stuck control request

[Oliver Neukum]

Am Freitag, 12. Februar 2010 01:49:10 schrieb David Fries:
> Oliver Neukum, this is based on your patch, do you agree with the
> changes?

I do agree.

> One possible problem with this fix is it drops the stalled URB
> transfer.  That might pose a problem for one shot messages, possibly
> hid_ctrl and hid_irq_out could resubmit the same urb when -ECONNRESET
> is received, assuming the message content wasn't the reason it
> timedout in the first place.

That is a big assumption. Realistically, if the hardware needs over 5s
to answer the hardware is buggy. We are working around that. It is best
to get not too fancy, unless testing shows that we need to.

	Regards
		Oliver

Re: usbhid control queue full, due to stuck control request

[Jiri Kosina]

On Fri, 12 Feb 2010, Oliver Neukum wrote:

> > Oliver Neukum, this is based on your patch, do you agree with the
> > changes?
> 
> I do agree.

Thanks a lot guys for tracking this down and fixing it. I have applied the 
patch.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: usbhid control queue full, due to stuck control request

[Oliver Neukum]

Am Freitag, 12. Februar 2010 12:57:11 schrieb Jiri Kosina:
> On Fri, 12 Feb 2010, Oliver Neukum wrote:
> 
> > > Oliver Neukum, this is based on your patch, do you agree with the
> > > changes?
> > 
> > I do agree.
> 
> Thanks a lot guys for tracking this down and fixing it. I have applied the 
> patch.

This being a fix for broken hardware it should go into stable, too.
Would you forward it?

	Regards
		Oliver

Re: usbhid control queue full, due to stuck control request

[Jiri Kosina]

On Fri, 12 Feb 2010, Oliver Neukum wrote:

> > > > Oliver Neukum, this is based on your patch, do you agree with the
> > > > changes?
> > > 
> > > I do agree.
> > 
> > Thanks a lot guys for tracking this down and fixing it. I have applied the 
> > patch.
> 
> This being a fix for broken hardware it should go into stable, too.
> Would you forward it?

Yes, will take care of that.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.