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. 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. 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. I wonder if we might not put some of this stuff in the core. It should be generally useful. > > 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? No, it's part of my plan to implement static bandwidth allocation (allocate bandwidth for all the endpoints when an altsetting is installed) instead of dynamic allocation (allocate bandwidth for an endpoint when an URB is submitted and the endpoint's queue is empty, release the bandwidth whenever the last URB in the queue completes), which is what [eou]hci-hcd do now. Probably other HCDs too, but I'm not so familiar with them. > 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. Yes, that is basically impossible. However this does help with the related problem of how to recover from a failed Set-Interface request. > > 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. Alan Stern -- 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
