The attached patch is lightly tested, I don't seem to have
devices handy that'll actually trigger the timeout path.
But it enumerates fine and looks cleaner, so I'm sending it
out for comments and feedback.
What it does:
- Gets rid of the ancient "usb_api_data" struct, which
was just a version of the newer "struct completion".
- Adds a clone of wait_for_completion() that can time out
rather than block forever. This is NOT currently found
in kernel/sched.c and <linux/completion.h> but maybe it
should be.
- Reworks usb_start_wait_urb() guts to be less convoluted,
and hence more likely correct. It uses that wait_timeout()
call and re-apportions responsibilities somewhat.
This shouldn't ever produce "raced timeout" messages and
the like, which the old code would do, and it won't make
me question "can that possibly be correct?" every time I
look at the code.
The comments I'm after include (a) does it work for you too?
(b) if you had setups that would time out before, do they
work better now? worse? (c) should the wait_timeout()
stuff be moved out of the USB subsystem?
Unfortunately this can't replace the usb-storage synch+timeout
primitives right now. Easy to allow SLAB_NOIO to be used on
the submit, harder to remove a pre-submit cancel race.
- Dave
p.s. CC'd Linus because of question (c) and because there's a
chance the problem he had last week might be fixed by
this patch.
--- ./drivers/usb-dist/core/message.c Wed Oct 30 20:52:12 2002
+++ ./drivers/usb/core/message.c Thu Nov 7 10:09:17 2002
@@ -11,72 +11,68 @@
#include "hcd.h" /* for usbcore internals */
-struct usb_api_data {
- wait_queue_head_t wqh;
- int done;
-};
-static void usb_api_blocking_completion(struct urb *urb)
+/* version of wait_for_completion() that can exit early
+ * returns nonzero only on on timeout
+ */
+static int wait_timeout (struct completion *x, signed long timeout_jiffies)
{
- struct usb_api_data *awd = (struct usb_api_data *)urb->context;
-
- awd->done = 1;
- wmb();
- wake_up(&awd->wqh);
+ might_sleep ();
+ spin_lock_irq (&x->wait.lock);
+ if (!x->done) {
+ DECLARE_WAITQUEUE (wait, current);
+
+ wait.flags |= WQ_FLAG_EXCLUSIVE;
+ __add_wait_queue_tail (&x->wait, &wait);
+ do {
+ __set_current_state (TASK_UNINTERRUPTIBLE);
+ spin_unlock_irq (&x->wait.lock);
+ timeout_jiffies = schedule_timeout (timeout_jiffies);
+ if (timeout_jiffies == 0) {
+ __remove_wait_queue (&x->wait, &wait);
+ return 1;
+ }
+ spin_lock_irq (&x->wait.lock);
+ } while (!x->done);
+ __remove_wait_queue (&x->wait, &wait);
+ }
+ x->done--;
+ spin_unlock_irq (&x->wait.lock);
+ return 0;
}
-// Starts urb and waits for completion or timeout
-static int usb_start_wait_urb(struct urb *urb, int timeout, int* actual_length)
-{
- DECLARE_WAITQUEUE(wait, current);
- struct usb_api_data awd;
- int status;
+/*-------------------------------------------------------------------*/
- init_waitqueue_head(&awd.wqh);
- awd.done = 0;
+static void urb_callback (struct urb *urb)
+{
+ complete ((struct completion *) urb->context);
+}
+
+static int usb_start_wait_urb(struct urb *urb, int timeout)
+{
+ struct completion complete;
+ int status;
- set_current_state(TASK_UNINTERRUPTIBLE);
- add_wait_queue(&awd.wqh, &wait);
+ init_completion (&complete);
+ urb->transfer_flags |= URB_ASYNC_UNLINK;
+ urb->context = &complete;
+ urb->complete = urb_callback;
- urb->context = &awd;
- status = usb_submit_urb(urb, GFP_ATOMIC);
- if (status) {
- // something went wrong
- usb_free_urb(urb);
- set_current_state(TASK_RUNNING);
- remove_wait_queue(&awd.wqh, &wait);
+ status = usb_submit_urb (urb, SLAB_KERNEL);
+ if (status < 0)
return status;
- }
- while (timeout && !awd.done)
- {
- timeout = schedule_timeout(timeout);
- set_current_state(TASK_UNINTERRUPTIBLE);
- rmb();
+ if (wait_timeout (&complete, timeout)) {
+ warn ("timeout on usb-%s-%s ep%d-%s (addr %d)",
+ urb->dev->bus->bus_name, urb->dev->devpath,
+ usb_pipeendpoint (urb->pipe),
+ usb_pipein (urb->pipe) ? "in" : "out",
+ urb->dev->devnum);
+ usb_unlink_urb (urb);
+ wait_for_completion (&complete);
+ urb->status = -ETIMEDOUT;
}
-
- set_current_state(TASK_RUNNING);
- remove_wait_queue(&awd.wqh, &wait);
-
- if (!timeout && !awd.done) {
- if (urb->status != -EINPROGRESS) { /* No callback?!! */
- printk(KERN_ERR "usb: raced timeout, "
- "pipe 0x%x status %d time left %d\n",
- urb->pipe, urb->status, timeout);
- status = urb->status;
- } else {
- warn("usb_control/bulk_msg: timeout");
- usb_unlink_urb(urb); // remove urb safely
- status = -ETIMEDOUT;
- }
- } else
- status = urb->status;
-
- if (actual_length)
- *actual_length = urb->actual_length;
-
- usb_free_urb(urb);
- return status;
+ return urb->status;
}
/*-------------------------------------------------------------------*/
@@ -86,20 +82,19 @@
{
struct urb *urb;
int retv;
- int length;
urb = usb_alloc_urb(0, GFP_KERNEL);
if (!urb)
return -ENOMEM;
usb_fill_control_urb(urb, usb_dev, pipe, (unsigned char*)cmd, data, len,
- usb_api_blocking_completion, 0);
+ 0, 0);
- retv = usb_start_wait_urb(urb, timeout, &length);
- if (retv < 0)
- return retv;
- else
- return length;
+ retv = usb_start_wait_urb(urb, timeout);
+ if (retv >= 0)
+ retv = urb->actual_length;
+ usb_free_urb(urb);
+ return retv;
}
/**
@@ -182,6 +177,7 @@
void *data, int len, int *actual_length, int timeout)
{
struct urb *urb;
+ int status;
if (len < 0)
return -EINVAL;
@@ -190,10 +186,13 @@
if (!urb)
return -ENOMEM;
- usb_fill_bulk_urb(urb, usb_dev, pipe, data, len,
- usb_api_blocking_completion, 0);
+ usb_fill_bulk_urb(urb, usb_dev, pipe, data, len, 0, 0);
- return usb_start_wait_urb(urb,timeout,actual_length);
+ status = usb_start_wait_urb(urb,timeout);
+ if (actual_length)
+ *actual_length = urb->actual_length;
+ usb_free_urb (urb);
+ return status;
}
/*-------------------------------------------------------------------*/
