Re: usb HC busted?

[Mathias Nyman <mathias.nyman-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On 19.07.2018 17:57, Alan Stern wrote:
> On Thu, 19 Jul 2018, Mathias Nyman wrote:
> 
>> xhci driver will set up all the endpoints for the new altsetting already in
>> usb_hcd_alloc_bandwidth().
>>
>> New endpoints will be ready and rings running after this. I don't know the exact
>> history behind this, but I assume it is because xhci does all of the steps to
>> drop/add, disable/enable endpoints and check bandwidth in a single configure
>> endpoint command, that will return errors if there is not enough bandwidth.
> 
> That's right; Sarah and I spent some time going over this while she was
> working on it.  But it looks like the approach isn't adequate.
> 
>> This command is issued in hcd->driver->check_bandwidth()
>> This means that xhci doesn't really do much in hcd->driver->endpoint_disable or
>> hcd->driver->endpoint_enable
>>
>> It also means that xhci driver assumes rings are empty when
>> hcd->driver->check_bandwidth is called. It will bluntly free dropped rings.
>> If there are URBs left on a endpoint ring that was dropped+added
>> (freed+reallocated) then those URBs will contain pointers to freed ring,
>> causing issues when usb_hcd_flush_endpoint() cancels those URBs.
>>
>> usb_set_interface()
>>     usb_hcd_alloc_bandwidth()
>>       hcd->driver->drop_endpoint()
>>       hcd->driver->add_endpoint() // allocates new rings
>>       hcd->driver->check_bandwidth() // issues configure endpoint command, free rings.
>>     usb_disable_interface(iface, true)
>>       usb_disable_endpoint()
>>         usb_hcd_flush_endpoint() // will access freed ring if URBs found!!
>>         usb_hcd_disable_endpoint()
>>           hcd->driver->endpoint_disable()  // xhci does nothing
>>     usb_enable_interface(iface, true)
>>       usb_enable_endpoint(ep_addrss, true) // not really doing much on xhci side.
>>
>> As first aid I could try to implement checks that make sure the flushed URBs
>> trb pointers really are on the current endpoint ring, and also add some warning
>> if we are we are dropping endpoints with URBs still queued.
>>
>> But we need to fix this properly as well.
>> xhci needs to be more in sync with usb core in usb_set_interface(), currently xhci
>> has the altssetting up and running when usb core hasn't event started flushing endpoints.
> 
> Absolutely.  The core tries to be compatible with host controller
> drivers that either allocate bandwidth as it is requested or else
> allocate bandwidth all at once when an altsetting is installed.
> 
> xhci-hcd falls into the second category.  However, this approach
> requires the bandwidth verification for the new altsetting to be
> performed before the old altsetting has been disabled, and the xHCI
> hardware can't do this.
> 
> We may need to change the core so that the old endpoints are disabled
> before the bandwidth check is done, instead of after.  Of course, this
> leads to an awkward situation if the check fails -- we'd probably have
> to go back and re-install the old altsetting.

That would help xhci a lot.

If we want to avoid the awkward altsetting re-install after bandwidth failure
then adding a extra endpoint flush before checking the bandwidth would already help a lot.

The endpoint disabling can then be remain after bandwidth checking.
Does that work for other host controllers?

-Mathias