Hi Sarah, I can't say for sure just yet but I'm optimistic. It seems that as far as the host is concerned - the issue is fixed. The device is stuck on the other hand so I'm debugging that. I'll update you as soon as I have some results. One thing I did notice is that it takes MS longer to enumerate with these 2 patches. Can it be because debugging is enabled? Thanks, Tanya Brokhman --- Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Sarah Sharp > Sent: Monday, June 06, 2011 10:43 AM > To: Tanya Brokhman > Cc: linux-usb@xxxxxxxxxxxxxxx; ablay@xxxxxxxxxxxxxx; Alan Stern > Subject: [RFT 2/2] USB: Free bandwidth when usb_disable_device is > called. > > 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. > > Sarah > > drivers/usb/core/message.c | 22 +++++++++++++++++++--- > 1 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index 5701e85..f18e306 100644 > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -1139,6 +1139,7 @@ void usb_disable_interface(struct usb_device > *dev, struct usb_interface *intf, > void usb_disable_device(struct usb_device *dev, int skip_ep0) > { > int i; > + struct usb_hcd *hcd = bus_to_hcd(dev->bus); > > /* getting rid of interfaces will disconnect > * any drivers bound to them (a key side effect) > @@ -1172,9 +1173,24 @@ void usb_disable_device(struct usb_device *dev, > int skip_ep0) > > dev_dbg(&dev->dev, "%s nuking %s URBs\n", __func__, > skip_ep0 ? "non-ep0" : "all"); > - for (i = skip_ep0; i < 16; ++i) { > - usb_disable_endpoint(dev, i, true); > - usb_disable_endpoint(dev, i + USB_DIR_IN, true); > + if (!hcd->driver->check_bandwidth) { > + for (i = skip_ep0; i < 16; ++i) { > + usb_disable_endpoint(dev, i, true); > + usb_disable_endpoint(dev, i + USB_DIR_IN, true); > + } > + } else { > + /* First pass: Cancel URBs, leave endpoint pointers intact. > */ > + for (i = skip_ep0; i < 16; ++i) { > + usb_disable_endpoint(dev, i, false); > + usb_disable_endpoint(dev, i + USB_DIR_IN, false); > + } > + /* Remove endpoints from the host controller internal state > */ > + usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL); > + /* Second pass: remove endpoint pointers */ > + for (i = skip_ep0; i < 16; ++i) { > + usb_disable_endpoint(dev, i, true); > + usb_disable_endpoint(dev, i + USB_DIR_IN, true); > + } > } > } > > -- > 1.7.4.1 > > -- > 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 -- 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
