On Wed, 15 Jan 2003, Oliver Neukum wrote: > Am Mittwoch, 15. Januar 2003 17:04 schrieb Alan Stern: > > resubmitted urbs. Second (and more subtle), there are differences between > > resubmitting an urb from the handler as opposed to submitting it again > > after the handler is done. For example, any bandwidth reserved for the > > urb is kept when the handler resubmits, but it is deallocated otherwise. > > Eh, how so? usb_hcd_giveback_urb() does call unlink_urb() > unconditionally.
This is where my lack of knowledge of the details underlying the usb core shows through. Okay, looking at hcd.c I see where urb_unlink() is called and how it releases the bandwidth. I'm surprised; is it really supposed to work this way? I recall a long discussion a few months ago between you and David Brownell concerning the question of how to reserve bandwidth. The upshot was that there was no need, because bandwidth was allocated on demand and would be retained as long as an urb was resubmitted by its completion handler. Looking at the code, it appears that is not the case -- usb_release_bandwidth() is called whether the urb is resubmitted or not. I don't know what ought to happen here. Somebody will have to fill me in. > The URB cannot finish regularly because that would be a hardware > IRQ reentering itself. The generic IRQ code should prevent that. Okay, there's no reason to worry about the possibility of a resubmitted urb being COMPLETING, and hence no reason to acquire the handler_lock before setting the state to COMPLETING. (But what if the resubmitted urb goes to a different usb device, on a different bus attached to a different IRQ line? Can't one hardware IRQ interrupt another? What if it is fielded by a different processor?) > So the only cause of a status change at that point can be usb_unlink_urb(). > In that case going to CANCELED seem to be correct and not causing > a problem. > > IMHO changing the status to CANCELED is no problem and neither > is spinning on the handler_lock. Submitting and unlinking an URB > from a completion handler is arcane anyway. The situation I had in mind (rare, I admit -- but that's where the gremlins live) is that the handler always resubmits, but one time through the loop another processor decides to unlink the urb somewhere between when it was resubmitted and when the handler returned. When that happens the state will be CANCELED upon the return from the handler, and so usb_hcd_giveback_urb() will proceed to change the state to IDLE. (It has to assume this was a normally cancelled urb, not one that was resubmitted and then cancelled.) This would be wrong; the state should remain CANCELED since the handler for the cancelled urb has not yet been called. Now the state is corrupted, something I would like to avoid. Although corrupting the state in this way seems innocent enough here, it has the potential to lead to problems later on. Since it only requires a little more programming effort to get things right, I think we should do so. > Could you explain why CANCELED and TERMINATING should be differentiated? > It seems to me that both usb_unlink_urb() and usb_submit_urb() would fail > with an error in both cases. Well, the explanation above shows why something like TERMINATING needs to exist. Differentiating them occurs in two places. Immediately after the call to the completion handler, the state gets set to IDLE if it was TERMINATING but not if it was CANCELED. Although usb_submit_urb() will fail with an error in both cases, the errors ought to be different: If the state was CANCELED then the urb was resubmitted _before its handler was called_; definitely a programming bug. If the state was TERMINATING then the handler is trying to resubmit a cancelled urb, which is not a bug; the routine should fail but not BUG or oops. Lastly, usb_unlink_urb() does not need to differentiate these states. > > In hcd_unlink_urb(): the CANCELED and TERMINATING cases are both BUGS (or > > maybe they are both just oopses) -- someone has tried to unlink the same > > urb twice. In the COMPLETING case, the state should change to > > TERMINATING. > > A double unlink can happen in some network drivers if timeout and disconnect > race. Can we simply return an error? Yes. The drivers could be rewritten to avoid a double unlink (I did just that for usb-storage), but an error here is probably okay. > I'll do that as soon as we have a final version. I've followed your > suggestions that seem obvious, but can we discuss the introduction > of a TERMINATING state and double locking? Now that I know that giveback_urb cannot be called reentrantly, I see there's no need to worry about double locking. That routine is the only place that needs to acquire handler_lock while there is a possibility of holding another spinlock, so the issue doesn't arise. If you feel that my approach still isn't right and want to discuss it some more, I'll be happy to oblige. Alan Stern ------------------------------------------------------- This SF.NET email is sponsored by: A Thawte Code Signing Certificate is essential in establishing user confidence by providing assurance of authenticity and code integrity. Download our Free Code Signing guide: http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0028en _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
