On Fri, Jul 08, 2011 at 05:45:24PM -0400, Alan Stern wrote: > On Thu, 7 Jul 2011, Sarah Sharp wrote: > > > In the upcoming patches, we'll use some stored endpoint information to > > make software keep track of the worst-case bandwidth schedule. We need to > > store several variables associated with each periodic endpoint: > > - the type of endpoint > > - Max Packet Size > > - Mult > > - Max ESIT payload > > - Max Burst Size (aka number of packets, stored in one-based form) > > - the endpoint interval (normalized to powers of 2 microframes) > > Some of this information is already stored in the endpoint descriptor > and endpoint companion descriptor: the type, maxpacket size, mult, and > max burst size. Also, according to my earlier suggestion, we should > add a max ESIT payload field or two to the usb_host_endpoint structure. > The only item this leaves for you to track explicitly is the interval. > (Of course, there's no reason you can't keep local copies of the > other items.) Yeah, I had a FIXME note in the original patch description that I removed when I figured out I couldn't just walk the USB tree. The problem is that if a device disconnects, the USB core will take the device lock, then try to take the bandwidth mutex to remove the endpoints. If the xHCI driver is trying to walk the tree to gather information about a different device's bandwidth change and tries to take that same device lock, we have deadlock. At this point I'm completely relying only information stored in the xHCI driver structures to avoid that deadlock. I've already discovered I was storing some usb_device pointers and usb_tt pointers, and changed the new bandwidth code not to do that. I could just store a pointer to the endpoint descriptors, but then there's no guarantees that it won't go away unless I take the device lock, right? Isn't storing the pointer just as dangerous as walking the USB tree? > I want to change the way usbcore enables and disables endpoints during > usb_set_interface() and usb_set_configuration(). There should be two > arrays, one containing pointers to the endpoints that are going to be > removed and the other containing pointers to the endpoints that are > going to be added. Both arrays will be passed to the HCD somehow. > That should make things quite a bit easier for xhci-hcd; it is > essentially the model used by the xHCI hardware. I don't see a problem with that change. We might be able to get rid of the reset bandwidth call if it's done that way, but I would have to think about it. > For error recovery we have to be able to reinstall the old set of > endpoints when something goes wrong. To do this, the first array will > have to keep track of more than just the pointers to the old endpoint > structures. At a minimum, for each periodic endpoint I'll need the old > interval, the old phase, and the old max ESIT payload. These values > can get filled in as the old endpoints are removed, and then they can > be used for reinstating the former schedule settings if necessary. Is this only necessary for your plan to have an interface for drivers to request a lower bandwidth than advertised in the endpoint? Or does this help the problem where the device stalls the Set Configuration request, and the USB core needs to revert to the old configuration and possibly rebind the drivers? I remember you mentioning it was basically impossible to go back to the old configuration when we were discussing the patch "USB: Free bandwidth when usb_disable_device is called." at the beginning of June. > What do you think? I think I may be failing to see how this helps the USB core or the xHCI driver in the long run. :) Sarah Sharp -- 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
