> 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/
This archive was generated by hypermail 2b29 : Thu Aug 23 2001 - 21:00:38 EST