Re: Patch for bizzare oops in USB

[Johannes Erdfelt <johannes@erdfelt.com>]

On Mon, Aug 20, 2001, Pete Zaitcev <zaitcev@redhat.com> wrote:
> 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.

So am I. To be honest, there's a bunch of things in the USB code I'm not
entirely happy.

I didn't see many of them until recently, but I guess with experience,
comes wisdom. I had intended to fix many of them in 2.5 when it finally
forks.

I like your patch, but since we have the new completion stuff now, we
should probably use that. I'll make the mod and send off the patch to
Linus when I get back from this business trip.

Thanks.

JE