> However, a grep of drivers/usb/*.c tells me that most drivers
> ignore the return value of usb_unlink_urb(). That's quite a lot of
> potential bugs ... there's a race. Happens on MP systems (CPU
> #1 is running the HCD's interrupt handler, which calls the urb's
> driver completion callback; CPU #2 tries to unlink the URB, fails,
> and thinks it's safe to free). But maybe not on uniprocessors.
Also on UP, unless interrupts are off.
> > This well and good and shows you that a completion handler
> > may be running and you need to wait for it to finish.
>
> Or ... the (control or bulk) completion handler might not yet
> have been invoked; or it might have already completed.
Yes. There are standard techniques known to make this work without a race.
That they are standard does not make them easy.
So let's do it once and do it right.
> The difference is clearly known to the completion handler.
> This is a good reason to adopt the policy that urb freeing
> should only be triggered from completion handlers. (Which
> is what "usbnet" does, FWIW :)
It's a reason precisely not to do that.
Upon return from disconnect they must have been freed
at least in some drivers eg. V4L.
The only easy way to ensure that is if the disconnect method can be sure
that it's always safe to reference the urb.
> > But there's no generic function to do so.
> >
> > Any driver can include this on its own, ...
>
> Which is surely why the idea of having a "generic" function
> for it feels to me like API bloat.
Then be consequent.
Either we have a minimum API, which means removing synchronous
unlinking or we have an API that moves common code into
the core.
> > > Could you explain why it's hard to actually let the completion
> > > handlers (each driver knows when they're called :) flag that it's
> > > OK to free such memory?
> >
> > Inconvinient. People will get it wrong.
>
> People get critical sections wrong all the time, but those are
> just bugs they need to fix. Same thing should apply here.
Nope. Writing correct code needs to be easy.
If that means more work for HCD/core programmers, so be it.
There are more device drivers than HCD drivers.
People will think about disconnecting in their disconnect methods.
If the completion handlers (aside from handling an error)
have to be aware of this, bugs are bound to be introduced.
> > Having gone through a lot
> > of disconnect methods I've been convinced that what can go wrong
> > will go wrong.
>
> I believe that most of the drivers in linux/drivers/usb were written
> (or were derived from drivers that were written) before the USB
> API was very stable at all. Disconnect processing itself wasn't
> quite stable at the time 2.4.0 shipped, and while there was a
> bunch of work to fix oops-on-disconnect bugs, that was mostly
> a case of fixing obvious failures rather than auditing and fixing
> drivers. The same applies for MP bugs, in spades -- most of
> us don't have MP systems to test with, and there's only so far
> you can go with SMP builds on UP hardware.
>
>
> I think the root cause of those problems has little to do with API
> problems, and a lot to do with the fact that correct disconnect
> and MP processing has often been last on the list of things that
> get done "right" in USB drivers.
That is true only partially.
Disconnecting is hard all by itself in principle.
Therefore driver authors need as much help as possible.
All the work for making a disconnect free of races needs to
be easyly doable in the disconnect method.
A simple sequence of
usb_unlink_urb()
usb_wait_completion()
usb_free_urb()
is hard to get wrong.
People doing their own implementation of the second
step will get it wrong in many cases, exactly because of the
difficulties you have mentioned.
Judging by your grep it is quite clear that people are simply
not aware of the problem. Educating them is a waste of effort.
Give them easy to use tools and an example and things will
improve.
Regards
Oliver
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
