Re: [PATCH RFC RFT] usb: hcd: Add a usb_device argument to hc_driver.endpoint_reset()

[Michał Pecio]

On Wed, 9 Apr 2025 10:13:50 -0400, Alan Stern wrote:
> The core does not explicitly flush endpoints before resetting a
> device. However, it does notify the class drivers' pre_reset
> callback, which is supposed to unlink all the URBs used by that
> driver.  If a driver doesn't have a pre_reset callback, the core
> unbinds it from the device (which will unlink all its URBs).  _If_
> everything is working properly, there shouldn't be any outstanding
> URBs when the reset takes place.

Thank you for clarification. This doesn't look too bad and I currently
have no concrete cases of the mechanism failing to work.
 
> Either way, though, the core doesn't invoke the HCD's endpoint_reset
> or endpoint_disable callback before the reset.  If you think the core
> needs to do more, or needs to issue the callbacks in a different
> order, let me know.

The problem is a matter of mismatched expectations: the core treats
endpoint_disable() as temporary, because "classic" HCDs free their
ep->hcpriv and recreate it quietly on the next URBs submission. And
their endpoint_reset() during this time simply does nothing.

But xhci considers it more permanent, like the last thing before
drop_endpoint(). It too clears ep->hcpriv, but here hcpriv is not
the endpoint state, it's usb_device pointer saved by add_endpoint()
and required for operation. The driver drops it and it's screwed.

Moving all usb_disable_interface() calls before their corresponding
usb_hcd_alloc_bandwidth() would meet xhci expectations, but IDK if
it would work in general. As far as I see in usb_set_interface() for
example, the control request is only done after successful bandwidth
allocation and the interface swap only after a successful request.

My patch addresses the problem from xhci side, by adapting to core
expectations. As far as I see, only xhci_endpoint_reset() is broken
by this add_endpoint() -> endpoint_disable() sequence, so I fix it
by removing the dependence on ep->hcpriv. Alternatively, we could
stop clearing ep->hcpriv, but I'm not sure if it's reliable. No HCD
depends on ep->hcpriv being preserved after endpoint_disable().

I will do a v2 because Mathias expressed interest in this patch for
cleanup's sake alone, but the kernel test robot found build issues.

Regards,
Michal