[ Since Greg KH and Duncan Sands haven't contributed directly to this discussion, I'm removing their names from the CC: above -- they can read this via the regular mailing list. ]
On Wed, 15 Jan 2003, Oliver Neukum wrote: > I take it back, there's a need for a TERMINATING state. > Your argument is right. But it's somewhat different still. > > usb_hcd_giveback_urb() must be able to tell apart > between usb_unlink_urb() called in state SUBMITTED > and COMPLETING because the transitions are different. > > SUBMITTED - usb_unlink_urb() -> CANCELED > COMPLETING - usb_unlink_urb() -> TERMINATING > > before handler after handler new state > CANCELED * IDLE > SUBMITTED COMPLETING IDLE > SUBMITTED CANCELED CANCELED > SUBMITTED TERMINATING IDLE I think your code is right, but it is more complicated than it needs to be. usb_hcd_giveback_urb() can already detect this difference because unlink called in state SUBMITTED means that upon entry to giveback_urb() the state is CANCELED, whereas unlink called in state COMPLETING means that upon entry to giveback_urb() the state is SUBMITTED. So in my proposal, at the start of giveback_urb() if state is SUBMITTED it gets set to COMPLETING; if state is CANCELED it gets set to TERMINATING. This eliminates the need for your old_state variable in giveback_urb(). When the handler returns, if the current state is COMPLETING or TERMINATING then it gets set to IDLE, otherwise (SUBMITTED or CANCELED) it doesn't change. In the end, this does the same thing as your code; it's just a little simpler. > This means that lock order must be handler_lock before lock. Yes. In fact, I would change the code in giveback_urb() so that handler_lock is _unlocked_ at the end, just before usb_put_urb(). That way, when a synchronous unlink stops spinning, all the state changes have taken place. By the same token, we should make sure that when a normal (i.e., not during the handler) synchronous call to usb_unlink_urb() returns, all the state changes have taken place. That can be accomplished by adding a spin_lock(&urb->handler_lock); spin_unlock(&urb->handler_lock); pair in usb_unlink_urb() just after wait_for_completion(&splice_done). Maybe that code path can be merged with the err_out_waiting path. > How do you like this version? Aside from the things mentioned above, it looks fine. Alan Stern ------------------------------------------------------- This SF.NET email is sponsored by: Thawte.com Understand how to protect your customers personal information by implementing SSL on your Apache Web Server. Click here to get our FREE Thawte Apache Guide: http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0029en _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
