Re: [PATCH] usb: Fix out of sync data toggle if a configured device is reconfigured

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

On 31.8.2020 15.02, Felipe Balbi wrote:
> 
> Hi,
> 
> Mathias Nyman <mathias.nyman@linux.intel.com> writes:
>> Userspace drivers that use a SetConfiguration() request to "lightweight"
>> reset a already configured usb device might cause data toggles to get out
>         ^
>         an

true

> 
>> of sync between the device and host, and the device becomes unusable.
>>
>> The xHCI host requires endpoints to be dropped and added back to reset the
>> toggle. USB core avoids these otherwise extra steps if the current active
>> configuration is the same as the new requested configuration.
>>
>> A SetConfiguration() request will reset the device side data toggles.
>> Make sure usb core drops and adds back the endpoints in this case.
>>
>> To avoid code duplication split the current usb_disable_device() function
>> and reuse the endpoint specific part.
> 
> it looks like the refactoring is unrelated to the bug fix, perhaps? But
> then again, it would mean adding more duplication just for the sake of
> keeping bug fixes as pure bug fixes. No strong opinion.
> 

I like it like this :) 
Avoids code duplication, and is simple enough for stable releases compared
to complete usb_set_configuration() and usb_reset_configuration(), which was an option. 

>> Cc: stable <stable@vger.kernel.org>
> 
> missing fixes?

Not really, this has just never really worked, nothing broke it.
Closest would be one of the patches that add usb3 specific code to
usb_reset_configuration() in 2.6 kernel.

> 
>> Tested-by: Martin Thierer <mthierer@gmail.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>  drivers/usb/core/message.c | 92 ++++++++++++++++++--------------------
>>  1 file changed, 43 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index 6197938dcc2d..a1f67efc261f 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
>> @@ -1205,6 +1205,35 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf,
>>  	}
>>  }
>>  
>> +/*
>> + * usb_disable_device_endpoints -- Disable all endpoints for a device
>> + * @dev: the device whose endpoints are being disabled
>> + * @skip_ep0: 0 to disable endpoint 0, 1 to skip it.
>> + */
>> +static void usb_disable_device_endpoints(struct usb_device *dev, int skip_ep0)
>> +{
>> +	struct usb_hcd *hcd = bus_to_hcd(dev->bus);
>> +	int i;
>> +
>> +	if (hcd->driver->check_bandwidth) {
>> +
> 
> maybe remove this blank line?

yes, this one was unintentional.

> 
>> +		/* 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);
>> +		}
> 
> maybe a blank line here?

Here I didn't want to alter the original style, like the next one. 

> 
>> +		/* Remove endpoints from the host controller internal state */
>> +		mutex_lock(hcd->bandwidth_mutex);
>> +		usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
>> +		mutex_unlock(hcd->bandwidth_mutex);
>> +	}
> 
> maybe a blank line here?
> 
>> +	/* 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);
>> +	}
>> +}
>> +
>>  /**
>>   * usb_disable_device - Disable all the endpoints for a USB device
>>   * @dev: the device whose endpoints are being disabled
>> @@ -1522,6 +1536,9 @@ EXPORT_SYMBOL_GPL(usb_set_interface);
>>   * The caller must own the device lock.
>>   *
>>   * Return: Zero on success, else a negative error code.
>> + *
>> + * If this routine fails the device will probably be in an unusable state
>> + * with endpoints disabled, and interfaces only partially enabled.
> 
> should you force U3 in that case?
> 

It didn't use to in the failing case, nor does usb_set_configuration() on failure.
I assumed the caller would handle a failure in their preferred way.
Probably reset the device, or perhaps as you suggest set it to U3 with
a usb_autosuspend_device()?

-Mathias