This patch (as1593) fixes some logic errors in the usbhid driver
relating to runtime PM. The driver does not balance its calls to
usb_autopm_get_interface_async() and usb_autopm_put_interface_async().
For example, when the control queue is restarted the driver does a
_get. But the resume won't happen immediately, so the driver leaves
the queue stopped. When the resume does occur, the queue is restarted
and a second _get occurs, with no balancing _put.
The patch fixes the problem by rearranging the logic for restarting
the queues. All the _get/_put calls and bitflag settings in
__usbhid_submit_report() are moved into the queue-restart routines. A
balancing _put call is added for the case where the queue is still
suspended. A call to irq_out_pump_restart(), which doesn't take all
the right actions for restarting the irq-OUT queue, is replaced by a
call to usbhid_restart_out_queue(), which does. Similarly for
ctrl_pump_restart().
Finally, new code is added to prevent an autosuspend from happening
every time an URB is cancelled, and the comments explaining what
happens when an URB needs to be cancelled are expanded and clarified.
Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
CC: Oliver Neukum <oliver@xxxxxxxxxx>
---
drivers/hid/usbhid/hid-core.c | 146 ++++++++++++++++++++----------------------
1 file changed, 72 insertions(+), 74 deletions(-)
Index: usb-3.5/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-3.5.orig/drivers/hid/usbhid/hid-core.c
+++ usb-3.5/drivers/hid/usbhid/hid-core.c
@@ -213,9 +213,20 @@ static int usbhid_restart_out_queue(stru
if ((kicked = (usbhid->outhead != usbhid->outtail))) {
hid_dbg(hid, "Kicking head %d tail %d", usbhid->outhead, usbhid->outtail);
+ /* Try to wake up from autosuspend... */
r = usb_autopm_get_interface_async(usbhid->intf);
if (r < 0)
return r;
+
+ /*
+ * If still suspended, don't submit. Submission will
+ * occur if/when resume drains the queue.
+ */
+ if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl)) {
+ usb_autopm_put_interface_no_suspend(usbhid->intf);
+ return r;
+ }
+
/* Asynchronously flush queue. */
set_bit(HID_OUT_RUNNING, &usbhid->iofl);
if (hid_submit_out(hid)) {
@@ -240,9 +251,20 @@ static int usbhid_restart_ctrl_queue(str
if ((kicked = (usbhid->ctrlhead != usbhid->ctrltail))) {
hid_dbg(hid, "Kicking head %d tail %d", usbhid->ctrlhead, usbhid->ctrltail);
+ /* Try to wake up from autosuspend... */
r = usb_autopm_get_interface_async(usbhid->intf);
if (r < 0)
return r;
+
+ /*
+ * If still suspended, don't submit. Submission will
+ * occur if/when resume drains the queue.
+ */
+ if (test_bit(HID_REPORTED_IDLE, &usbhid->iofl)) {
+ usb_autopm_put_interface_no_suspend(usbhid->intf);
+ return r;
+ }
+
/* Asynchronously flush queue. */
set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
if (hid_submit_ctrl(hid)) {
@@ -546,49 +568,36 @@ static void __usbhid_submit_report(struc
usbhid->out[usbhid->outhead].report = report;
usbhid->outhead = head;
- /* Try to awake from autosuspend... */
- if (usb_autopm_get_interface_async(usbhid->intf) < 0)
- return;
+ /* If the queue isn't running, restart it */
+ if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
+ usbhid_restart_out_queue(usbhid);
- /*
- * 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;
+ /* Otherwise see if an earlier request has timed out */
+ } else if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
+
+ /* Prevent autosuspend following the unlink */
+ usb_autopm_get_interface_no_resume(usbhid->intf);
- if (!test_and_set_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
- 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
- * but an earlier request may be stuck
- * we may need to time out
- * no race because the URB is blocked under
- * spinlock
+ * Prevent resubmission in case the URB completes
+ * before we can unlink it. We don't want to cancel
+ * the wrong transfer!
*/
- if (time_after(jiffies, usbhid->last_out + HZ * 5)) {
- usb_block_urb(usbhid->urbout);
- /* drop lock to not deadlock if the callback is called */
- spin_unlock(&usbhid->lock);
- usb_unlink_urb(usbhid->urbout);
- spin_lock(&usbhid->lock);
- usb_unblock_urb(usbhid->urbout);
- /*
- * if the unlinking has already completed
- * the pump will have been stopped
- * it must be restarted now
- */
- if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
- if (!irq_out_pump_restart(hid))
- set_bit(HID_OUT_RUNNING, &usbhid->iofl);
+ usb_block_urb(usbhid->urbout);
+
+ /* Drop lock to avoid deadlock if the callback runs */
+ spin_unlock(&usbhid->lock);
+
+ usb_unlink_urb(usbhid->urbout);
+ spin_lock(&usbhid->lock);
+ usb_unblock_urb(usbhid->urbout);
+ /* Unlink might have stopped the queue */
+ if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
+ usbhid_restart_out_queue(usbhid);
- }
+ /* Now we can allow autosuspend again */
+ usb_autopm_put_interface_async(usbhid->intf);
}
return;
}
@@ -610,47 +619,36 @@ static void __usbhid_submit_report(struc
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 the queue isn't running, restart it */
+ if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
+ usbhid_restart_ctrl_queue(usbhid);
- /*
- * 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;
+ /* Otherwise see if an earlier request has timed out */
+ } else if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
+
+ /* Prevent autosuspend following the unlink */
+ usb_autopm_get_interface_no_resume(usbhid->intf);
- if (!test_and_set_bit(HID_CTRL_RUNNING, &usbhid->iofl)) {
- 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
- * but an earlier request may be stuck
- * we may need to time out
- * no race because the URB is blocked under
- * spinlock
+ * Prevent resubmission in case the URB completes
+ * before we can unlink it. We don't want to cancel
+ * the wrong transfer!
*/
- if (time_after(jiffies, usbhid->last_ctrl + HZ * 5)) {
- usb_block_urb(usbhid->urbctrl);
- /* drop lock to not deadlock if the callback is called */
- spin_unlock(&usbhid->lock);
- usb_unlink_urb(usbhid->urbctrl);
- spin_lock(&usbhid->lock);
- usb_unblock_urb(usbhid->urbctrl);
- /*
- * if the unlinking has already completed
- * the pump will have been stopped
- * it must be restarted now
- */
- if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
- if (!ctrl_pump_restart(hid))
- set_bit(HID_CTRL_RUNNING, &usbhid->iofl);
- }
+ usb_block_urb(usbhid->urbctrl);
+
+ /* Drop lock to avoid deadlock if the callback runs */
+ spin_unlock(&usbhid->lock);
+
+ usb_unlink_urb(usbhid->urbctrl);
+ spin_lock(&usbhid->lock);
+ usb_unblock_urb(usbhid->urbctrl);
+
+ /* Unlink might have stopped the queue */
+ if (!test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
+ usbhid_restart_ctrl_queue(usbhid);
+
+ /* Now we can allow autosuspend again */
+ usb_autopm_put_interface_async(usbhid->intf);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
