This is becoming more complex than I expected... In the following, please don't think that I'm trying to defend the strategy I suggested earlier. I don't care very much what solution ends up being adopted; I just want to make people aware of the scope and nature of the problem.
Part of the difficulty stems from the fact that a driver's view of things might not be in close accord with the core's views, and that even different core developers may have had different views at one time or another. On Mon, 8 Dec 2003, David Brownell wrote: > Alan Stern responding to David Brownell: > > >>But did any kernel ever guarantee those semantics? I don't think so. > >>Certainly 2.6 never did, and neither OHCI nor EHCI on 2.4 does... > > > > I agree, they didn't. However, that lack didn't prevent driver authors > > from _assuming_ that they did. > > Sure, but I can't think of that as anything other than a > set of long-standing driver bugs. Someone with a different viewpoint might call it a long-standing bug in the core :-) Regardless, it should be fixed. > > Whether this is smaller than my patch is a question of interpretation: > > Yours is a much bigger change for the device driver! It also doesn't > > But those are less fragile than the combination of usbcore and > the hcds. Changing the semantics of unlinking could have lots > of un-intended consequences at lower levels; IMO they should > get even simpler, not more complicated. The changes I suggested did not affect the lower levels (HCDs) more than minimally. Mostly they changed the way synchronous unlink worked at the hcd.c level and added a failure mode to submit_urb. Overall I could argue that the net result was not more complication, in that synchronous unlinking would behave more uniformly than it currently does. > On the other hand, if we just agreed not to call those semantics > by the name "unlinking", and to provide them through some simple > function like usb_urb_begone(), then I see simple ways out. > > Sure many drivers would need "obviously correct" one-line changes, > to call that routine. But that'd be unlikely to break any of > the hard-to-fix lower level code. That's fine by me. A large part of the problem has been that people want varying functionality but they call the functions by the same name. One person saying "Your implementation of usb_unlink_urb is wrong because it doesn't do what I think it should", when in fact it does do what the other person thinks it should, can only lead to mayhem. > > address the problems of failing resubmission and races between submit and > > unlink. > > That second problem is orthogonal; even drivers that use > asynchronous unlinking have to solve it. Yes. The resubmission issue is more subtle. After all, if one agrees that synchronous unlink means waiting until the completion handler has returned _and_ the URB is idle, what then should happen if the completion handler resubmits? On the other hand, if one thinks that synchronous unlink merely means waiting until the completion handler returns, there's no problem. It's that same paradigm error. Different people have different expectations and we need to settle on a single meaning. > > But as far as I can see, there is _no_ statement in the kernel source > > about what struct usb_operations->disable() is supposed to do. The > > comments in front of one implementation in hcd.c (hcd_endpoint_disable) > > merely state that all endpoint state is gone from the hardware. Since > > UHCI doesn't store any endpoint state in the hardware to begin with, > > that doesn't say anything. _Nothing_ is mentioned anywhere about > > waiting until all the software/driver state is gone as well. > > If usbfs were waiting for urb completions, that UHCI-specific > behavior wouldn't matter -- there'd be no oopsing. If that > UHCI-specific behavior were changed, there'd also be none. > > ... the thing is, I think that we probably added an "expectation" > that HCDs wait for the URBs to complete, by some changes several > months after the disable() method was first defined. That's > where the UHCI-specific problem came up. This is not a real problem. We just need to add kerneldoc explaining what the disable() method is supposed to do (along with adding the UHCI implementation). > > If the urb was resubmitted by the completion handler, urb_begone() as > > written would loop endlessly. > > At the risk of sounding like a broken record, device > drivers that don't have synchronization on resubmit > paths are buggy, and they need to add it. Wouldn't it be nice if the core provided a mechanism (not a policy!) that would make it easy for drivers to synchronize on resubmit paths without having to add a lot of tricky locking code? endpoint_disable() combined with urb_begone() might well be enough. Alan Stern ------------------------------------------------------- This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
