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

[Mathias Nyman]

On 8.4.2025 13.18, Michał Pecio wrote:
> xHCI needs usb_device in this callback so it employed some hacks that
> proved unreliable in the long term and made the code a little tricky.
>
> Make USB core supply it directly and simplify xhci_endpoint_reset().
> Use xhci_check_args() to prevent resetting emulated endpoints of root
> hubs and to deduplicate argument validation and improve debuggability.
>
> Update ehci_endpoint_reset(), which is the only other such callback,
> to accept (and ignore) the new argument.
>
> This fixes the root cause of a 6.15-rc1 regression reported by Paul,
> which I was able to reproduce locally. It also solves the general
> problem of xhci_endpoint_reset() becoming a no-op after device reset
> or changing configuration or altsetting. Although nobody complained
> because halted endpoints are reset automatically by xhci_hcd, it was
> a bug - sometimes class drivers want to reset not halted endpoints.
>
> Reported-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-usb/c279bd85-3069-4841-b1be-20507ac9f2d7@xxxxxxxxxxxxx/
> Signed-off-by: Michal Pecio <michal.pecio@xxxxxxxxx>
> ---
>
> Is such change acceptable to interested parties?

Looks like an improvement, and will help clear the EP_STALLED flag
eventually in device reset case.

There are some issues still unsolved due to how xhci endpoints end up being handled
in usb core usb_reset_and_verify_device().

usb_reset_and_verify_device()
  hub_port_init()
    hcd->driver->reset_device(hcd, udev);          /*1 xhci frees ep rings, loses td_list heads */
  usb_hcd_alloc_bandwidth(new_config, NULL, NULL)  /*2 xhci drop+add ep, allocated new ep rings */
  usb_control_msg(udev, usb_sndctrlpipe(...,USB_REQ_SET_CONFIGURATION,...)
  for (interfaces) {
    if (AlternateSetting == 0) {
      usb_disable_interface()  /*3 flush urbs, ->xhci_urb_dequeue() */
      usb_enable_interface()   /*4 clear EP_STALLED flag */
    } else {
      usb_set_interface()
    }

1. driver->reset_device will free all xhci endpoint rings, and lose td_list head, but
   keep cancelled_td_list and ep->ep_state flags. xHC issues reset device command
   setting all internal ep states in xci to "disabled".

2. usb hcd_alloc_bandwith will drop+add xhci endpoints for this configuration,
   allocate new endpoint rings, and inits new td_list head.
   Old cancelled_td_list and ep_state flags are still set, not matching ring.

3. usb_disable_interface() will flush all pending URBs calling xhci_urb_dequeue().
   xhci_urb_dequeue() makes decision based on stale ep_state flags.
   May start to cancel/giveback urbs in old cancelled_td_list for tds that existed
   on old freed ring. will also set host_ep->hcpriv to null

4. usb_enable_interface() calls xhci_endpoint_reset() that finally clears
   the EP_STALLED flag (udev now found thanks to this patch)

Disabling endpoints, like calling usb_disable_interface() in step 3 should be
done before calling  usb_hcd_alloc_bandwith().
This was fixed in other places such as usb_set_interface() and usb_reset_config()

We might need to clean up ep_state flags and give back cancelled URBs in
xhci_discover_or_reset_device() after the reset device xhci command completion.

Thanks
Mathias