On Mon, 17 May 2004, Oliver Neukum wrote:
> Am Montag, 17. Mai 2004 18:56 schrieb Greg KH:
> > On Mon, May 17, 2004 at 12:41:42PM -0400, Alan Stern wrote:
> > > +void usb_kill_urb(struct urb *urb)
> > > +{
> > > + if (!(urb->dev && urb->dev->bus && urb->dev->bus->op))
> > > + return;
> > > + urb->reject = 1;
> > > + urb->dev->bus->op->unlink_urb(urb, -ENOENT);
> > > + wait_event(usb_kill_urb_queue, atomic_read(&urb->kref.refcount) <= 1);
> > > + urb->reject = 0;
> > > }
> >
> > Heh, no, you can't just sit and spin on the refcount, because that will
> > not work if I do:
> > usb_free_urb(urb);
> > usb_kill_urb(urb);
>
> Arguably you must not kill what you've freed, as you have no reference.
>
> > And that last urb->reject = 0 will oops if I do that :)
> > (and yes, some drivers do that already today with a call to
> > usb_submit_urb() followed by a call to usb_free_urb().)
I agree with Oliver here. Once you've freed the URB, you have no business
trying to do anything with the pointer. I trust that the drivers today do
_not_ do usb_free_urb() followed by usb_unlink_urb() -- that could oops
almost as easily as your example.
> > Or what if I do:
> > usb_get_urb(urb);
> > usb_get_urb(urb);
> > usb_get_urb(urb);
> > usb_kill_urb(urb);
> > which will cause this function to deadlock.
Yes, and that is mentioned in the kerneldoc at the start of
usb_kill_urb(). I did a quick search through all the USB drivers, and
_none_ of them call usb_get_urb(). Only the core uses that function. So
for now there isn't a problem.
> > You need to grab a reference to the urb at the beginning of the call,
> > and decrement it at the end of it. But in the middle there, you can't
> > count on a paticular refcount value, so you will have to trigger off of
> > something else.
>
> Right. However, you need a _submission_ counter only if giveback_urb()
> can be reentered. If we are ready to ban resubmission to another device
> in completion handlers, a single bit will do.
If the two of you feel strongly that the existing reference counter
shouldn't be used, I will change it. The new counter and the new "reject"
field could easily fit into two shorts.
> Alan wrote:
> >You are right. There's no choice but to add another field for the Reject
> >flag.
>
> But there is. Use urb->lock to protect transfer_flags.
You mean, require every completion handler to do that. I would prefer to
add a new field. And whether this means adding 2 shorts or 2 bits doesn't
matter much, because _any_ addition will cause the structure to grow by a
minimum of 4 bytes (on x86).
Alan Stern
-------------------------------------------------------
This SF.Net email is sponsored by: SourceForge.net Broadband
Sign-up now for SourceForge Broadband and get the fastest
6.0/768 connection for only $19.95/mo for the first 3 months!
http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
