Re: Data toggles not reset on "set configuration" for ports handled by "xhci_hcd" driver

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 25.8.2020 11.00, Martin Thierer wrote:
>> Can you try the code below? It should force dropping and adding the endpoints
>> for the intrface(s) of that configuration, and xhci should reset the toggles.
>>
>> Completely untested, it compiles :)
> 
> Sorry, no, that doesn't work:
> 
> xhci_hcd 0000:00:14.0: Trying to add endpoint 0x83 without dropping it.
> BUG: kernel NULL pointer dereference, address: 0000000000000010

Ah, I see.
Endpoints weren't dropped on host side as pointer to the endpoints were cleaned up before this.
And the code to recover from a failed call got messed up as we removed some stuff it depends on.

Here's a second version. 
I'm again not able to test this at all from my current location, so it might fail because
of some silly mistake, but it compiles..

This version keeps endpoint pointers intact until endpoints are dropped from hcd side, 
it also removes the recover path (might need to fix one later) 

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 6197938dcc2d..e90e8781f872 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1537,9 +1537,21 @@ int usb_reset_configuration(struct usb_device *dev)
 	 * calls during probe() are fine
 	 */
 
+	/*
+	 * TEST2 flush and disable endpoints but leave the pointers intact until
+	 * usb_hcd_alloc_bandwidth() has dropped them from host controller side
+	 */
 	for (i = 1; i < 16; ++i) {
-		usb_disable_endpoint(dev, i, true);
-		usb_disable_endpoint(dev, i + USB_DIR_IN, true);
+		if (dev->ep_out[i]) {
+			dev->ep_out[i]->enabled = 0;
+			usb_hcd_flush_endpoint(dev, dev->ep_out[i]);
+			usb_hcd_disable_endpoint(dev, dev->ep_out[i]);
+		}
+		if (dev->ep_in[i]) {
+			dev->ep_in[i]->enabled = 0;
+			usb_hcd_flush_endpoint(dev, dev->ep_in[i]);
+			usb_hcd_disable_endpoint(dev, dev->ep_in[i]);
+		}
 	}
 
 	config = dev->actconfig;
@@ -1554,33 +1566,21 @@ int usb_reset_configuration(struct usb_device *dev)
 		return -ENOMEM;
 	}
 	/* Make sure we have enough bandwidth for each alternate setting 0 */
-	for (i = 0; i < config->desc.bNumInterfaces; i++) {
-		struct usb_interface *intf = config->interface[i];
-		struct usb_host_interface *alt;
+	/* TEST drop and re-add endpoints to clear toggle */
+	retval = usb_hcd_alloc_bandwidth(dev, config, NULL, NULL);
 
-		alt = usb_altnum_to_altsetting(intf, 0);
-		if (!alt)
-			alt = &intf->altsetting[0];
-		if (alt != intf->cur_altsetting)
-			retval = usb_hcd_alloc_bandwidth(dev, NULL,
-					intf->cur_altsetting, alt);
-		if (retval < 0)
-			break;
+	/* clear the endpoint pointers, FIXME check if needed */
+	for (i = 1; i < 16; ++i) {
+		dev->ep_out[i] = NULL;
+		dev->ep_in[i] = NULL;
 	}
-	/* If not, reinstate the old alternate settings */
+
 	if (retval < 0) {
-reset_old_alts:
-		for (i--; i >= 0; i--) {
-			struct usb_interface *intf = config->interface[i];
-			struct usb_host_interface *alt;
-
-			alt = usb_altnum_to_altsetting(intf, 0);
-			if (!alt)
-				alt = &intf->altsetting[0];
-			if (alt != intf->cur_altsetting)
-				usb_hcd_alloc_bandwidth(dev, NULL,
-						alt, intf->cur_altsetting);
-		}
+		/*
+		 * Can't walk backwards and set the old alts back as we no longer
+		 * set one interface at a time, but full configuration in one go
+		 */
+
 		usb_enable_lpm(dev);
 		mutex_unlock(hcd->bandwidth_mutex);
 		return retval;
@@ -1589,8 +1589,11 @@ int usb_reset_configuration(struct usb_device *dev)
 			USB_REQ_SET_CONFIGURATION, 0,
 			config->desc.bConfigurationValue, 0,
 			NULL, 0, USB_CTRL_SET_TIMEOUT);
-	if (retval < 0)
-		goto reset_old_alts;
+	if (retval < 0) {
+		usb_enable_lpm(dev);
+		mutex_unlock(hcd->bandwidth_mutex);
+		return retval;
+	}
 	mutex_unlock(hcd->bandwidth_mutex);
 
 	/* re-init hc/hcd interface/endpoint state */