On Mon, 6 Jun 2011, Sarah Sharp wrote: > Tanya ran into an issue when trying to switch a UAS device from the BOT > configuration to the UAS configuration via the bConfigurationValue sysfs > file. Before installing the UAS configuration, set_bConfigurationValue() > calls usb_disable_device(). That function is supposed to remove all host > controller resources associated with that device, but it leaves some state > in the xHCI host controller. > > usb_disable_device() goes through all the motions of unbinding the drivers > attached to active interfaces and removing the USB core structures > associated with those interfaces, but it doesn't actually remove the > endpoints from the internal xHCI host controller bandwidth structures. > > When usb_disable_device() calls usb_disable_endpoint() with reset_hardware > set to true, the entries in udev->ep_out and udev->ep_in will be set to > NULL. Usually, when the USB core installs a new configuration, > usb_hcd_alloc_bandwidth() will drop all non-NULL endpoints in udev->ep_out > and udev->ep_in before adding any new endpoints. However, when the new > UAS configuration was added, all those entries were null, so none of the > old endpoints in the BOT configuration were dropped. > > The xHCI driver blindly added the UAS configuration endpoints, and some of > the endpoint addresses overlapped with the old BOT configuration > endpoints. This caused the xHCI host to reject the Configure Endpoint > command. Now that the xHCI driver code is cleaned up to reject a > double-add of active endpoints, we need to fix the USB core to properly > drop old endpoints in usb_disable_device(). > > If the host controller driver needs bandwidth checking support, make > usb_disable_device() call usb_disable_endpoint() with > reset_hardware set to false, drop the endpoints from the xHCI host > controller, and then call usb_disable_endpoint() again with > reset_hardware set to true. > > The first call to usb_disable_endpoint() will cancel any pending URBs and > wait on them to be freed in usb_hcd_disable_endpoint(), but will keep the > pointers in udev->ep_out and udev->ep in intact. Then > usb_hcd_alloc_bandwidth() will use those pointers to know which endpoints > to drop. > > The final call to usb_disable_endpoint() will do two things: > > 1. It will call usb_hcd_disable_endpoint() again, which should be harmless > since the ep->urb_list should be empty after the first call to > usb_disable_endpoint() returns. > > 2. It will set the entries in udev->ep_out and udev->ep in to NULL, and call > usb_hcd_disable_endpoint(). That call will have no effect, since the xHCI > driver doesn't set the endpoint_disable function pointer. > > Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> > Reported-by: Tanya Brokhman <tlinder@xxxxxxxxxxxxxx> > Cc: ablay@xxxxxxxxxxxxxx > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > --- > > Alan, > > Does this patch look correct? I was struggling a bit to make sure the > USB core behavior remained the same, while still correctly dropping the > old configuration's endpoints from the xHCI driver. How about just unconditionally calling usb_hcd_alloc_bandwidth() before looping through the endpoint arrays? Alternatively, how about making xhci-hcd's endpoint_disable method responsible for releasing the endpoint's bandwidth reservation? 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
