Am Samstag, 9. August 2003 02:04 schrieb David Brownell: > The code that manges the synchronous control/bulk calls has > been a mess for ages. This patch rewrites it using: > > - "struct completion" instead of a usb-internal clone therof, > - prepare_to_wait()/finish_wait() instead of the tangled > mess it now uses (or a new wait_event_timeout call, as in > previous versions of this patch).
wait_event_timeout still seems ideally suited to the task. > It's a net code shrink and simplification. > > Please review and merge. > > - Dave [..] // 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; + DEFINE_WAIT(wait); + DECLARE_COMPLETION(done); int status; - init_waitqueue_head(&awd.wqh); - awd.done = 0; - - set_current_state(TASK_UNINTERRUPTIBLE); - add_wait_queue(&awd.wqh, &wait); + urb->transfer_flags |= URB_ASYNC_UNLINK; Why async? + urb->context = &done; - urb->context = &awd; + might_sleep(); + prepare_to_wait(&done.wait, &wait, TASK_UNINTERRUPTIBLE); status = usb_submit_urb(urb, GFP_ATOMIC); If you are doing it that way, why do you use a struct completion? A simple wait queue is sufficient. And doesn't export details of how struct completion looks internally. - if (status) { - // something went wrong - usb_free_urb(urb); - set_current_state(TASK_RUNNING); - remove_wait_queue(&awd.wqh, &wait); - return status; - } - - while (timeout && !awd.done) - { - timeout = schedule_timeout(timeout); - set_current_state(TASK_UNINTERRUPTIBLE); - rmb(); - } + if (status != 0) { + urb->actual_length = 0; - set_current_state(TASK_RUNNING); - remove_wait_queue(&awd.wqh, &wait); + /* normal case: we woke without a timeout */ + } else if (schedule_timeout((timeout == 0) + ? MAX_SCHEDULE_TIMEOUT + : timeout) != 0) { + status = urb->status; - 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; + /* abnormal: timed out, so force completion via unlink */ + } else { + status = usb_unlink_urb(urb); + switch (status) { + case -EINPROGRESS: /* normal */ + case -EBUSY: /* already completing */ + dev_err(&urb->dev->dev, "control/bulk timeout\n"); + break; + default: + /* shouldn't happen */ + dev_err(&urb->dev->dev, "c/b unlink err %d\n", status); + break; } - } else - status = urb->status; + wait_for_completion(&done); This is a nop unless you timed out. Why not do the unlink synchronously? + /* NOTE: HCDs return this status too */ + status = -ETIMEDOUT; + } + finish_wait(&done.wait, &wait); if (actual_length) *actual_length = urb->actual_length; - usb_free_urb(urb); return status; } ------------------------------------------------------- This SF.Net email sponsored by: Free pre-built ASP.NET sites including Data Reports, E-commerce, Portals, and Forums are available now. Download today and enter to win an XBOX or Visual Studio .NET. http://aspnet.click-url.com/go/psa00100003ave/direct;at.aspnet_072303_01/01 _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel