On Sat, Jul 09, 2011 at 05:12:27PM -0400, Alan Stern wrote: > On Fri, 8 Jul 2011, Sarah Sharp wrote: > > > > (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? > > It's not quite that simple. The endpoint descriptors go away when the > device is disconnected. But before that happens, usbcore will > deallocate the device's bandwidth by calling usb_disable_device. > Therefore you shouldn't ever need to access an endpoint descriptor > that's about to disappear. > > To answer your question: No, storing a pointer isn't really the same as > locking the devices. For instance, you can guarantee that a usb_device > structure won't be deallocated simply by taking a reference to it > (usb_get_dev). But this doesn't guarantee that the device's list of > children won't change. > > However, you should never need to walk the device tree in the first > place. What you should do (and I say this without having yet looked at > the code you posted) is save a private data structure for each enabled > endpoint, containing whatever scheduling parameters you need, such as > possibly a pointer to the usb_host_endpoint structure. Yes, there's already a structure in the xhci_virt_ep that stores that information, so I'm not walking the tree. I can change it to include a pointer to the endpoint descriptor, rather than storing duplicate copies of the max packet size and number of additional opportunities per microframe. I might also need to store a pointer to the SuperSpeed endpoint companion descriptor eventually. > In addition, for each scheduling domain you should store an array > holding the number of microseconds allocated for each frame or > microframe. The length of the array doesn't have to be terribly long. > Perhaps 256 uframes for HS/SS and 32 frames for FS/LS (which is what > uhci-hcd and ohci-hcd use). Any endpoint with a period longer than > that would get charged with more bandwidth than it actually uses, but I > don't think this will matter much. I don't actually have control over which endpoints are scheduled in which microframe with the Intel hardware. That's handled dynamically in the hardware, so I can't assume a particular periodic endpoint is going to be scheduled in a particular microframe. Instead, the bandwidth algorithm computes the worst possible microframe (or at least no worse than the worst microframe). For each bandwidth domain, I have a table (xhci_interval_bw_table) that stores information about endpoints that have the same periodic interval. That's used to calculate the most packed microframe, see if it's possible to shuffle transfers out of over-packed microframes and still meet the interval contract, and finally calculate the overall worst case bandwidth used. > Some extra information might also be needed. For example, ehci-hcd > will need to keep a sorted list of all active periodic endpoints below > each TT; xhci-hcd might need something similar. Yes, I also need to keep track of which endpoints are below each TT. It's further exasperated by the fact that the USB core only assigns one usb_tt structure per HS hub, not per port on a multi-TT hub. I really only need to keep periodic endpoints in the list, and I have the list sorted with the greatest max packet size first, which makes it easy for the bandwidth checking algorithm. > I wonder if we might not put some of this stuff in the core. It should > be generally useful. Yes, some of it might be useful in the USB core. However, I'm under a bit of pressure from the distros. They really didn't like backporting the big USB core changes for the USB 3.0 hub patches, because they then needed to revalidate all the other host controller drivers. (You could argue they should just take the latest upstream kernel, but that's not an argument I'm going to win.) So to try and make it more palatable for them to take the Intel xHCI support patches, I'd like to do a gradual patchset that adds the bandwidth checking to the xHCI driver, and then converts the code to use any shared values in the USB core. It will probably look pretty silly in the long run, but that's the sort of rock-and-hard place I'm stuck in. I'm not opposed to making big USB core changes and getting EHCI to do static bandwidth checking in the long run, but I'd like to get the xHCI driver working for the Intel xHCI hosts that need software bandwidth before we focus on improving other drivers. > > > 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. :) > > In the long run it allows us to do bandwidth allocation at the right > time and in the right way (i.e., as described in the USB spec), instead > of struggling to make two different approaches both work. > > If you're more concerned about the short term, we ought to be able to > avoid the most recent set of problems by making usb_disable_device > acquire the bandwidth_mutex rather than having the caller do it. The > idea is that we first unbind all the drivers and unregister all the > interfaces, then hold the mutex while releasing the old bandwidth and > allocating the new. Yes, that's probably a better solution. Do you want to make that patch or should I? 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
