On 31.8.2020 15.02, Felipe Balbi wrote:
>
> Hi,
>
> Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
>> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
>> ---
>> 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
