On Wed, 18 Jul 2007, David Brownell wrote: > On Wednesday 18 July 2007, Alan Stern wrote: > > We currently don't have any way to go from the endpoint structure to > > the usb_device structure, so dev is not implicit. > > Easy enough to fix if we want. Similarly with the relevant interface > (a one-to-one mapping except for ep0).
Yes. It's not clear that it would help very much. There are lots of places where we need to go from the URB to the device, so it would best to keep the device pointer where it is directly accessible. About the only advantage would be to shorten usb_fill_*_urb() by one line. > > This has led to a rather strange difficulty. As part of dequeuing an > > URB, the HCD will have to verify that the URB is linked into the > > endpoint's urb_list. However the endpoint is accessed via a pointer in > > an array (udev->ep_in[] or udev->ep_out[]) indexed by the endpoint > > number as stored in urb->pipe -- and when an endpoint is disabled the > > pointer is set to NULL. This will make it impossible for > > usb_hcd_endpoint_disable() to do its job! Once the endpoint has been > > disabled, the unlink verification won't work. > > I'd expect HCD signatures to change. E.g. dequeue() can take a > host endpoint just like enqueue(). Eventually I'd hope the HCDs > would stop doing such lookups entirely... With the host endpoint pointer in the URB, neither method needs that extra argument. I was planning to remove it from the enqueue methods. > > I've got two proposed changes to help deal with this. The first is to > > add an "enabled" flag to struct usb_host_endpoint. This simplifies the > > testing a lot and is generally superior to the array entry approach > > IMO. > > A backpointer to the device could do double duty here ... if it's > null then enqueue() is disallowed, and otherwise it would be used > for whatever. Maybe... I'm leery of that approach because of the possibility that someone will come along and try to use the backpointer at a time when it has been set to NULL. > > The second is to change struct urb by adding a pointer to the > > usb_host_endpoint. We have discussed this before; eventually urb->ep > > should replace urb->pipe entirely. For now, until the drivers can be > > changed we'll have to live with both fields. Still, it's a step in the > > right direction. > > When you set it up in submit_urb() you aren't testing if it's enabled. The test occurs later, under the protection of a spinlock. This is necessary because disable_endpoint needs to synchronize new submissions with the dequeuing of all existing URBs. > I wouldn't mark that as "out"; for now it's internal to core+hcd, and > probably shouldn't be meaningful except between submit and complete... Good point. I'll change it to "(internal)". Hopefully it will eventually become "(in)"... > > Of particular interest is the new routine usb_control_urb_is_out(). > > The one piece of information present in a pipe but not an endpoint > > descriptor is the direction of a control transfer. This routine > > figures out the direction by looking at bRequestType and wLength in the > > setup packet. > > ISTR other special casing that kicks in "early", e.g. before the > SET_ADDRESS request completes. I had completely forgotten about that! Yes, you're perfectly correct; that code will need special attention. (No doubt it would have shown up very quickly under testing, but I posted the RFC before trying to test it. In fact I still haven't done a test.) > > Does anybody object to this? > > Not in principle, modulo the notes above, but I don't have time for > anything like a careful review. Thanks for the feedback. Alan Stern ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/ _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
