[RFT v2 2/2] USB: Free bandwidth when usb_disable_device is called.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Note that usb_disable_device() will now need to be called with
hcd->bandwidth_mutex held.

Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Reported-by: Tanya Brokhman <tlinder@xxxxxxxxxxxxxx>
Cc: ablay@xxxxxxxxxxxxxx
Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
---

Hi Tanya,

The patch now has proper locking for the bus bandwidth mutex, and has
been updated with some less-ugly-code-but-still-not-pretty-code for
Alan. :)  Please let me know if it fixes your issue.  Even if your UAS
device hangs after the second Configure Endpoint command is submitted,
it would be helpful to know that the code fixes the xHCI issue.

Sarah Sharp

 drivers/usb/core/hub.c     |    3 +++
 drivers/usb/core/message.c |   15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 79a58c3..4372ae3 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1632,6 +1632,7 @@ void usb_disconnect(struct usb_device **pdev)
 {
 	struct usb_device	*udev = *pdev;
 	int			i;
+	struct usb_hcd		*hcd = bus_to_hcd(udev->bus);
 
 	if (!udev) {
 		pr_debug ("%s nodev\n", __func__);
@@ -1659,7 +1660,9 @@ void usb_disconnect(struct usb_device **pdev)
 	 * so that the hardware is now fully quiesced.
 	 */
 	dev_dbg (&udev->dev, "unregistering device\n");
+	mutex_lock(hcd->bandwidth_mutex);
 	usb_disable_device(udev, 0);
+	mutex_unlock(hcd->bandwidth_mutex);
 	usb_hcd_synchronize_unlinks(udev);
 
 	usb_remove_ep_devs(&udev->ep0);
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 5701e85..64c7ab4 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1135,10 +1135,13 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf,
  * Deallocates hcd/hardware state for the endpoints (nuking all or most
  * pending urbs) and usbcore state for the interfaces, so that usbcore
  * must usb_set_configuration() before any interfaces could be used.
+ *
+ * Must be called with hcd->bandwidth_mutex held.
  */
 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,6 +1175,16 @@ 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");
+	if (hcd->driver->check_bandwidth) {
+		/* 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);
@@ -1727,6 +1740,7 @@ free_interfaces:
 	/* if it's already configured, clear out old state first.
 	 * getting rid of old interfaces means unbinding their drivers.
 	 */
+	mutex_lock(hcd->bandwidth_mutex);
 	if (dev->state != USB_STATE_ADDRESS)
 		usb_disable_device(dev, 1);	/* Skip ep0 */
 
@@ -1739,7 +1753,6 @@ free_interfaces:
 	 * host controller will not allow submissions to dropped endpoints.  If
 	 * this call fails, the device state is unchanged.
 	 */
-	mutex_lock(hcd->bandwidth_mutex);
 	ret = usb_hcd_alloc_bandwidth(dev, cp, NULL, NULL);
 	if (ret < 0) {
 		mutex_unlock(hcd->bandwidth_mutex);
-- 
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux