> I'm going to guess that you will object to the race condition that exists
> between the time the flag is tested and the URB is submitted. Yes,
> there's a race. But it is unavoidable, unless you want to surround every
> call to the usb core with a spinlock_irq or the equivalent. And in any
That's exactly what's needed. But why irq? Disconnect() runs from task
context.
> case it doesn't matter, since we only want to guarantee that no URB will
> be submitted after disconnect() _returns_. We don't need to (and in fact
> cannot) guarantee that no URB will be submitted after disconnect() _is
> called_.
Right. But why should this matter? If there's a race there's a race.
Unless you can guarantee that disconnect() setting a flag for disconnection
and an urb being submitted or a synchronous helper running don't race
it's a bug.
> Perhaps the real question is: Do we need to guarantee that no URBs will
> remain outstanding after disconnect() returns? This seems to be what you
if (!(dev = urb->dev) || !dev->bus || dev->devnum <= 0)
return -ENODEV;
if (!(op = dev->bus->op) || !op->submit_urb)
return -ENODEV;
from usb_submit_urb(). This is deadly. Unlinking after disconnect() is
deadly as well.
But for an indeterminate amount of time after usb_submit_urb is called,
it's dangerous. Shortly, it's safe only if they are dead.
> are getting at. Unfortunately, I haven't seen written down anywhere
> exactly what conditions the driver needs to satisfy. It would be nice if
> this sort of thing could be added to the documentation somewhere.
Yes.
> > > Here's a suggestion that would help all existing usb drivers. Add an
> > > additional parameter to usb_control_msg (and the other synchronous
> > > message subroutines as well) that takes a struct urb **. The
> > > subroutines can use it to store a pointer to the dynamically allocated
> > > URB, so that the driver would have the ability to cancel the URB
> > > whenever it wanted.
> >
> > No, not whenever, only after the synchronous call has set the pointer.
> > Which you cannot know.
>
> I'm afraid you're right about that. Maybe the synchronous call routines
> should be eliminated entirely.
Now, a week before freeze?
> > I think that it won't work. The correct sequence is:
> > ...
> > down()
> > if (device_not_disconnected)
> > synchronous_call()
> > up()
> > ...
> >
> > and in disconnect():
> >
> > down()
> > mark_as_disconnected()
> > up()
> >
> > As you can see, the check for disconnection cannot be in the synchronous
> > helper.
>
> I'm not clear on how this helps. Yes, if you do it this way then you
> never have to cancel a synchronous message. But instead you force
> disconnect() to hang around waiting until the synchronous message
> completes. Is that really any better? And if it is, why not have
Disconnect() may block. As long as there's an upper limit, it's safe.
> disconnect() do the mark_as_disconnected() before doing the down()?
Why not. I see no real difference.
> > The solution you suggest only works if you can ensure that the
> > synchronous call has given you a handle to unlink the urb before
> > disconnect() runs. To do so requires the synchronous helper to be called
> > with a lock against disconnect() already held.
>
> Well no, I only need to insure that the synchronous call has given me a
> handle before disconnect() _returns_. But I agree that it's a problem.
You cannot do that as well. Furthermore you must not call a synchronous
call after disconnect() has returned. The device pointer you pass is
dangerous.
> My feeling is that we need either to rethink the synchronous calls or get
> rid of them altogether. There's another aspect of usb-storage that runs
It's a week till feature freeze.
> into difficulties with regard to synchronous calls. The problem is that
> most of the things that usb-storage does come about because of calls from
> the scsi layer, and most of these calls are protected by a timeout and
> abort handler. But not all of them are, and also there are a few things
> usb-storage does on its own behalf (initialization, mostly) that also do
> not have the protection of an abort handler.
>
> As a result, we have to be able to abort some calls on demand but not
> others, and we have to use timeouts for some calls but not others. It's a
> real mess, and we haven't gotten it straightened out yet. My vote goes
> for making things as uniform as possible, but Matt Dharm doesn't agree.
> He has said that the driver should use different code paths for the
> different situations. Maybe this discussion about the problems with
> synchronous calls will make us rethink the whole question.
I am afraid I don't know the storage driver well enough to comment on that.
Regards
Oliver
-------------------------------------------------------
This sf.net email is sponsored by: Influence the future
of Java(TM) technology. Join the Java Community
Process(SM) (JCP(SM)) program now.
http://ads.sourceforge.net/cgi-bin/redirect.pl?sunm0002en
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
