On Mon, Aug 10, 2009 at 05:04:33PM -0400, Alan Stern wrote: > On Mon, 10 Aug 2009, Sarah Sharp wrote: > > > > Did you know that usbcore does a usb_disable_endpoint() for ep0 when > > > the maxpacket value is updated? In view of this fact, perhaps the > > > patch isn't needed. > > > > The short answer is that the xHCI code that hooks into the > > usb_disable_endpoint() and usb_enable_endpoint() to update bandwidth > > calculations on the HW doesn't handle changes to control endpoint 0. It > > just ignores the USB core removing and re-adding control endpoint 0 > > during a configuration change. > > usbcore doesn't touch endpoint 0 during config changes. It is removed > and re-added only during device initialization, in usb_ep0_reinit(). Ok, I looked at this and I believe you. :) I looked at the configuration code quite a while ago, and I must have been mis-remembering. > > The usb_ep0_reinit() after the 8 byte > > descriptor fetch would simply be ignored by the xHCI bandwidth code. > > So then how/where do the xHCI HW structures for ep0 get set up > initially? In hcd->driver->address_device()? Yes. The control endpoint structure must be set up before the host controller will issue the set address control transfer (even though it doesn't actually use the control endpoint ring at all). > > The slightly longer answer is that the xHCI HW has two commands for > > updating endpoint information. One is a "Configure Endpoint" command > > that will add, remove, or update information about any endpoint besides > > the default control endpoints. The second command is an "Evaluate > > Context" command that only works on the default control endpoint, and > > the device information (e.g. is this device a hub). > > > > The xHCI bandwidth code builds up an input variable for a Configure > > Endpoint command with every call to usb_disable_endpoint() and > > usb_enable_endpoint(). When the USB core calls > > usb_hcd_check_bandwidth(), the xHCI driver submits that command. > > > > If I supported changing the max packet size through > > usb_disable_endpoint() and usb_enable_endpoint(), I would have to > > special case this bandwidth code to submit an Evaluate Context command > > when endpoint 0 changed. > > Yes, that sounds like a reasonable approach. > > > The USB core removes and readds the control > > endpoint 0 on every configuration change, > > No, it doesn't. > > > which means I would have to > > issue an Evaluate Context command when the default endpoint > > characteristics didn't really change, or further special case this code > > to check whether the endpoint characteristics actually changed. > > > > I'm not really comfortable with special casing the bandwidth code to > > handle max packet size 0 changes. > > It's just a way of handling ep0 like every other endpoint. Of course, > if the xHCI hardware really does handle ep0 specially then it's > understandable that you might want to do this in a different way. Yes, the xHCI hardware treats ep0 differently than other endpoints. > > One slight rant: why does the USB core disable both directions of the > > control endpoint 0 but only enable one direction? Shouldn't it enable > > both directions? That's yet another reason why I didn't want to handle > > control endpoint 0 changes in the bandwidth code. > > Sorry, I don't understand the question (or rant). Where does usbcore > enable only one direction of ep0? Are you talking about > usb_ep0_reinit()? The difference there is caused by the difference in > the arguments for usb_disable_endpoint() and usb_enable_endpoint(). > One takes an endpoint address while the other takes a pointer to a > struct usb_host_endpoint. Since the structure is for a control > endpoint, it naturally refers to both directions. > > If usb_disable_endpoint() were rewritten to accept a pointer to struct > usb_host_endpoint, then usb_ep0_reinit() would have to call it only > once. But the loop near the start of usb_disable_device() would have > to become more complicated. Yes, I was asking why usb_ep0_reinit() calls usb_disable_endpoint() for both directions of the control endpoint. I looked at making usb_ep0_reinit() call hcd->drop_endpoint(), hcd->usb_add_endpoint(), and hcd->check_bandwidth() for control endpoint 0. usb_ep0_reinit() is called too many times, for my purposes. It's called when the USB device address changes (possibly several times if the enumeration sequence fails). The xHCI driver doesn't care about address changes, because it's already involved through hcd->address_device(). Also, the return value of usb_ep0_reinit() would have to change if it called down to the xHCI bandwidth functions. The xHCI driver may run out of memory when it allocates structures for the evaluate context command. That means that new parts of the enumeration sequence may fail. Also, I'm unlikely to get your suggested patch included in 2.6.31, since it changes the signature of an exported function. If I change the max packet size when the enumeration sequence submits the next control transfer instead, that part of the enumeration sequence is already coded to look for out of memory errors, and the change doesn't touch any exported functions. In short, I'm still leaning towards my original patch. :) Sarah -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
