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

Michał Pecio
2025-04-08T10:18:24Z

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(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a63c793bac21..d2433807a397 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1986,7 +1986,7 @@ void usb_hcd_reset_endpoint(struct usb_device *udev,
struct usb_hcd *hcd = bus_to_hcd(udev->bus);

if (hcd->driver->endpoint_reset)
- hcd->driver->endpoint_reset(hcd, ep);
+ hcd->driver->endpoint_reset(hcd, udev, ep);
else {
int epnum = usb_endpoint_num(&ep->desc);
int is_out = usb_endpoint_dir_out(&ep->desc);
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 6d1d190c914d..813cdedb14ab 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1044,7 +1044,8 @@ ehci_endpoint_disable (struct usb_hcd *hcd, struct usb_host_endpoint *ep)
}

static void
-ehci_endpoint_reset(struct usb_hcd *hcd, struct usb_host_endpoint *ep)
+ehci_endpoint_reset(struct usb_hcd *hcd, struct usb_device *udev,
+ struct usb_host_endpoint *ep)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
struct ehci_qh *qh;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0452b8d65832..5bf89ba7e2b8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3161,11 +3161,10 @@ static void xhci_endpoint_disable(struct usb_hcd *hcd,
* resume. A new vdev will be allocated later by xhci_discover_or_reset_device()
*/

-static void xhci_endpoint_reset(struct usb_hcd *hcd,
+static void xhci_endpoint_reset(struct usb_hcd *hcd, struct usb_device *udev,
struct usb_host_endpoint *host_ep)
{
struct xhci_hcd *xhci;
- struct usb_device *udev;
struct xhci_virt_device *vdev;
struct xhci_virt_ep *ep;
struct xhci_input_control_ctx *ctrl_ctx;
@@ -3175,7 +3174,12 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
u32 ep_flag;
int err;

+ err = xhci_check_args(hcd, udev, host_ep, 1, true, __func__);
+ if (err <= 0)
+ return;
+
xhci = hcd_to_xhci(hcd);
+ vdev = xhci->devs[udev->slot_id];
ep_index = xhci_get_endpoint_index(&host_ep->desc);

/*
@@ -3185,28 +3189,13 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
*/
if (usb_endpoint_xfer_control(&host_ep->desc) && ep_index == 0) {

- udev = container_of(host_ep, struct usb_device, ep0);
- if (udev->speed != USB_SPEED_FULL || !udev->slot_id)
- return;
-
- vdev = xhci->devs[udev->slot_id];
- if (!vdev || vdev->udev != udev)
- return;
-
- xhci_check_ep0_maxpacket(xhci, vdev);
+ if (udev->speed == USB_SPEED_FULL)
+ xhci_check_ep0_maxpacket(xhci, vdev);

/* Nothing else should be done here for ep0 during ep reset */
return;
}

- if (!host_ep->hcpriv)
- return;
- udev = (struct usb_device *) host_ep->hcpriv;
- vdev = xhci->devs[udev->slot_id];
-
- if (!udev->slot_id || !vdev)
- return;
-
ep = &vdev->eps[ep_index];

spin_lock_irqsave(&xhci->lock, flags);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index ac95e7c89df5..179c85337eff 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -304,7 +304,7 @@ struct hc_driver {

/* (optional) reset any endpoint state such as sequence number
and current window */
- void (*endpoint_reset)(struct usb_hcd *hcd,
+ void (*endpoint_reset)(struct usb_hcd *hcd, struct usb_device *udev,
struct usb_host_endpoint *ep);

/* root hub support */
--
2.48.1