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 18.10, Alan Stern wrote:
> On Tue, Aug 25, 2020 at 02:53:56PM +0300, Mathias Nyman wrote:
>> 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]);
>> +		}
>>  	}
> 
> There's got to be a better way to do this, something that doesn't 
> involve so much code duplication.  For instance, maybe we could make 
> this routine and usb_set_configuration() both call a new 
> __usb_set_config(), with an extra flag telling the routine whether to 
> change the interface devices and bindings.

I agree that this needs cleaning up, this code was intended for testing.

It allows us to call usb_hcd_alloc_bandwidth() once with a configuration
and with the old endpoint pointers still intact, leading to one configure
endpoint command for xhci with the relevant drop and add endpoint flags set,
all in one go.

Looks like the last part usb_disable_device() does similar endpoint code
churning to flush, disable, drop, and remove endpoints. May we could start
by turning that code into some useful helper first?

-Mathias


maybe we could start