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

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@molgen.mpg.de>
Closes: https://lore.kernel.org/linux-usb/c279bd85-3069-4841-b1be-20507ac9f2d7@molgen.mpg.de/
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---

Is such change acceptable to interested parties?

It solves the problem completely for me, because as Alan said,
core calls endpoint_reset() after installing a new config or alt
to notify HCDs that ignore reset_device(), and also those which
implement it incompletely, I guess ;)

Unlike clearing EP_STALLED on reset_device() or drop_endpoint(),
this also fixes cases when another STALL happens after device
reset and the device is not reset again. For example, I see that
when I insert a card after the original problem happens.

At this point I can insert or remove the card, plug or unplug
the reader and reload ums-realtek in any order, it all works.

Paul, could you check if this patch works on your hardware too?

 drivers/usb/core/hcd.c      |  2 +-
 drivers/usb/host/ehci-hcd.c |  3 ++-
 drivers/usb/host/xhci.c     | 27 ++++++++-------------------
 include/linux/usb/hcd.h     |  2 +-
 4 files changed, 12 insertions(+), 22 deletions(-)

On Tue, 8 Apr 2025 16:55:24 +0300, Mathias Nyman wrote:
> 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()

I haven't thought about it, but you are right. This means that my
commit message is wrong to suggest that the problem occurs after
altsetting changes, it is apparently unique to device reset case.

> 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.

I guess it wouldn't be strictly necessary if core flushed all endpoints
before resetting. I frankly always assumed that it does so, because it
also makes sense for other HCDs which don't even define reset_device().

There seems to be nothing stopping them from talking to a device while
it is undergoing a reset and re-configuration, seems a little risky
given that HW isn't exactly free of its own bugs.

Speaking of xhci, I wonder if this could also be responsible for some
xHC crashes during enumeration. I recall that those Etron quirk patches
mentioned events for a disabled endpoint and "HC died" in some cases.

Regards,
Michal

On Wed, Apr 09, 2025 at 12:18:19PM +0200, Michał Pecio wrote:
> On Tue, 8 Apr 2025 16:55:24 +0300, Mathias Nyman wrote:
> > 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()
> 
> I haven't thought about it, but you are right. This means that my
> commit message is wrong to suggest that the problem occurs after
> altsetting changes, it is apparently unique to device reset case.
> 
> > 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.
> 
> I guess it wouldn't be strictly necessary if core flushed all endpoints
> before resetting. I frankly always assumed that it does so, because it
> also makes sense for other HCDs which don't even define reset_device().
> 
> There seems to be nothing stopping them from talking to a device while
> it is undergoing a reset and re-configuration, seems a little risky
> given that HW isn't exactly free of its own bugs.

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.

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.

Alan Stern

> Speaking of xhci, I wonder if this could also be responsible for some
> xHC crashes during enumeration. I recall that those Etron quirk patches
> mentioned events for a disabled endpoint and "HC died" in some cases.
> 
> Regards,
> Michal