From: Oliver Neukum <[email protected]>
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 <[email protected]>
Duplicated the change from last_out to last_ctrl and verified.
Signed-off-by: David Fries <[email protected]>
---
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 <[email protected]>
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
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
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.
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
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.