Messages in this thread Patch in this message | | | Date | Mon, 20 Aug 2001 17:44:48 -0400 | From | Pete Zaitcev <> | Subject | Re: Patch for bizzare oops in USB |
| |
> Date: Mon, 20 Aug 2001 13:59:37 +0200 > From: Thomas Sailer <sailer@scs.ch>
> > - current->state = TASK_INTERRUPTIBLE; > > + current->state = TASK_UNINTERRUPTIBLE; /* MUST BE SO. -- zaitcev */
> This is bad for other users of usb_control_msg/usb_bulk_msg that depend on > the sleep to be interruptible. Instead of bouncing back and forth whether > those routines shall sleep interruptibly or uninterruptibly, we should either > provide two routines or a parameter that specifies whether the sleep > shall be interruptible, or create a local version of usb_control_msg > if ov511 is the only user requiring uninterruptible sleep.
A prolifiration of subtly different versions of basic primitives is not an answer either. But wait, here's a better fix. The root of the evil is that the waiting thread accesses urb->status before a callback happened, which is unsafe.
BTW, I took a liberty to clean the thing up a bit. It looked as if the author of that fragment was not sure of what he was doing, and the style was quite dirty. I think a number of wrongs for such a small fragment was astonishing.
- "status" was an errno in the begining, then 5 lines down it's bool (== timeout), then it turns into system style once again. - Wasted pointer to wait head - Unused void *stuff. - typedef without _t and unprefixed type name in global header. - Urban legend of test for waitqueue_active() before wakeup. This is one half wrong because so many people do it, anyone has an idea why? Half a point deducted. - Confused and redundant checking for -EINPROGRESS (even if it was the cause of oops)
And the last one ... - THE GOD DAMN RACE THAT OOPS - that's 10 hacker points down! It only goes to show that replacing interruptible_sleep_on with add_wait_queue/schedule/remove_wait_queue does not make any racy code correct automatically.
Cheesh, I am surprised _anything_ in Linux kernel works.
-- Pete
--- linux-2.4.8/include/linux/usb.h Fri Aug 10 18:16:46 2001 +++ linux-2.4.8-e/include/linux/usb.h Mon Aug 20 10:28:36 2001 @@ -539,13 +539,12 @@ * SYNCHRONOUS CALL SUPPORT * *-------------------------------------------------------------------*/ -typedef struct +struct usb_api_data { - wait_queue_head_t *wakeup; - - void* stuff; - /* more to follow */ -} api_wrapper_data; + wait_queue_head_t wqh; + int done; + /* void* stuff; */ /* Possible extension later. */ +}; /* -------------------------------------------------------------------------- */ --- linux-2.4.8/drivers/usb/usb.c Tue Jul 24 14:20:56 2001 +++ linux-2.4.8-e/drivers/usb/usb.c Mon Aug 20 14:05:45 2001 @@ -1041,15 +1041,10 @@ *-------------------------------------------------------------------*/ static void usb_api_blocking_completion(urb_t *urb) { - api_wrapper_data *awd = (api_wrapper_data *)urb->context; + struct usb_api_data *awd = (struct usb_api_data *)urb->context; - if (waitqueue_active(awd->wakeup)) - wake_up(awd->wakeup); -#if 0 - else - dbg("(blocking_completion): waitqueue empty!"); - // even occurs if urb was unlinked by timeout... -#endif + awd->done = 1; + wake_up(&awd->wqh); } /*-------------------------------------------------------------------* @@ -1060,35 +1055,32 @@ static int usb_start_wait_urb(urb_t *urb, int timeout, int* actual_length) { DECLARE_WAITQUEUE(wait, current); - DECLARE_WAIT_QUEUE_HEAD(wqh); - api_wrapper_data awd; + struct usb_api_data awd; int status; - - awd.wakeup = &wqh; - init_waitqueue_head(&wqh); + + init_waitqueue_head(&awd.wqh); + awd.done = 0; + current->state = TASK_INTERRUPTIBLE; - add_wait_queue(&wqh, &wait); + add_wait_queue(&awd.wqh, &wait); + urb->context = &awd; status = usb_submit_urb(urb); if (status) { // something went wrong usb_free_urb(urb); current->state = TASK_RUNNING; - remove_wait_queue(&wqh, &wait); + remove_wait_queue(&awd.wqh, &wait); return status; } - if (urb->status == -EINPROGRESS) { - while (timeout && urb->status == -EINPROGRESS) - status = timeout = schedule_timeout(timeout); - } else - status = 1; + while (timeout && !awd.done) + timeout = schedule_timeout(timeout); current->state = TASK_RUNNING; - remove_wait_queue(&wqh, &wait); + remove_wait_queue(&awd.wqh, &wait); - if (!status) { - // timeout + if (!timeout) { printk("usb_control/bulk_msg: timeout\n"); usb_unlink_urb(urb); // remove urb safely status = -ETIMEDOUT; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |