On Fri, 30 Apr 2004, Oliver Neukum wrote: > Am Freitag, 30. April 2004 16:12 schrieb Alan Stern: > > On Fri, 30 Apr 2004, Oliver Neukum wrote: > > > Very, very tempting. Elegant and simple. But I have a problem with it. > > > It's entirely legal for a driver to call usb_get_urb() twice. And then we > > > have a problem. So I don't think that we should use the refcount. > > > > Yes, it is legal for a driver to do that. I suspect that very few of them > > do, because there's not often a reason for it. In fact, a recursive grep > > of the kernel source shows that usb_get_urb() is mentioned only within > > include/linux/usb.h, core/hcd.c, core/urb.c, host/ehci-sched.c, and > > host/hc_simple.c. No drivers call it. Anyway, we can always document at > > the start of the routine that it shouldn't be used if the URB's reference > > count has been monkeyed with. > > But that's the direction we are moving in, isn't it?
I don't think so. Sure, people are paying more attention now to refcounting devices and interfaces, but not URBs. So long as 0 drivers call usb_get_urb(), it's fair to say that we are _not_ moving in that direction. > > On the other hand, there are some very good reasons for using the refcount > > in that test. Whatever we test, it needs to be something that is changed > > in giveback_urb after the completion handler returns. The only field for > > which that is currently true is the refcount. Remember, at the end of > > giveback_urb usbcore no longer owns the URB; the driver does. Hence there > > isn't much that giveback_urb can afford to change. > > But giveback_urb doesn't need to change anything. Calling the handler > itself it knows when it has run. Well, in your patch you've got it changing urb->phase. And that's at a time when usbcore doesn't own the URB. Furthermore, the issue isn't how giveback_urb knows when the handler has run -- it's how usb_kill_urb() knows. > > Also, whatever we use, it has to take into account the possibility of > > resubmission (and even the theoretical possibility of nested invocations > > of giveback_urb!). The only thing that will handle this properly is a > > refcount of some sort. Yes, we could introduce another one that would > > track just the active submissions -- but why go to all that trouble (and > > introduce yet another field into the already-bloated struct urb) when > > we have a ready-made reference count sitting right there? > > I don't think we need a refcount. Could you have a look at this code: I looked at it quickly. In effect, your urb->phase _is_ a refcount, although it only takes on 2-1/2 values (DOING and DONE are approximately equivalent). You have ignored the possibility that giveback_urb might be called reentrantly for the same URB. You have ignored the possibility that more than one thread might call usb_kill_urb() for the same URB simultaneously. Rather than preventing resubmission, you are allowing it and then awkwardly trying to unlink the URB again. Also, you have added a bunch of extra stuff to struct urb, which is already too big, and a lot of extra code to usbcore. If you object to using the existing refcount, wouldn't it be a lot simpler to adopt my scheme but using an additional submission-count field? That would increase the size of struct urb by a smaller amount. Alan Stern ------------------------------------------------------- This SF.Net email is sponsored by: Oracle 10g Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE. http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
