Vojtech, thank you for the explanation. That's much better than the
one-line message from Pavel which just said there was a problem.
Randy -- please reject _all_ of Pavel's patches. At best, they don't fix
anything. At worst, they introduce new problems, hide other problems, and
slow me down by forcing me to diff and merge his changes and mine.
Pavel - look, I hate to sound like this, but would you please stop messing
around in places you don't fully understand? I'll be the first to admit
that your comment about the spinlock_irqsave as a good catch. But,
without even waiting for a reply from me, you issued multiple patches
which didn't fix anything. In fact your attempt to remove duplicated code
breaks things because of the status-less nature of CB transport. Of
course, you didn't know that. You also didn't know that the reduction of
common code is on my TODO list. But it's well behind getting functional
bugs (like the lack of reset functions) done. And every time you bypass
me (the official maintainer of usb-storage -- check the MAINTAINERS file),
you slow the process down. That may be fine during most times, but with
the race for 2.4 on, it's unacceptable.
I'm always open to bug reports. If you want to submit a suggested
pseudo-code or even a real patch, go ahead. But you can't go sending
patches to the list with a phrase like "I hope I did not break anything",
especially in cases where you have. And, if you do find something wrong,
a message with more than one line saying something "is completely broken
in usb-storage", and then have a second line which inaccurately describes
the problem. You claim, "You may never ever block while you are holding
spinlock" which is not the case. Correctly, as Vojtech points out, it's
improper for me to hold the _irqsave spinlock. Tho, oddly enough, it
seems to work for me on my system. But Vojtech is correct, and I will fix
this tomorrow morning.
Matt Dharm
On Mon, 3 Apr 2000, Vojtech Pavlik wrote:
> On Sun, Apr 02, 2000 at 02:40:52PM -0700, Matthew Dharm wrote:
>
> > What's wrong with this? We need to hold the spinlock to prevent the
> > disconnect function from proceeding while we are using the device.
> > The spinlock we are holding is a private spinlock. I don't see what you
> > think is wrong.
>
> You cannot call anything that calls schedule() while holding
> spinlock_irqsave. spinlock without _irqsave would be in my opinion OK,
> but schedule() will sti() unconditionally and thus you'll lose the irq
> protection.
>
> You do this on other places of usb-storage, too, for example usb_string
> on line 1248 calls usb_control_msg which in turn calls schedule(), and
> that's done from withing spinlock_irqsave.
>
> It's no fun doing raceless code. I'll have to audit my drivers soon,
> too, because I'm sure they're rather full of races, because they do
> almost no locking :(
>
>
--
Matthew Dharm Home: [EMAIL PROTECTED]
Engineer, Qualcomm, Inc. Work: [EMAIL PROTECTED]
Ye gods! I have feet??!
-- Dust Puppy
User Friendly, 12/4/1997
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
