xHCI needs usb_device here, so it stored it in host_endpoint.hcpriv,
which proved problematic due to some unexpected call sequences from
USB core, and generally made the code more complex than it has to be.

Make USB core supply it directly and simplify xhci_endpoint_reset().
Use the xhci_check_args() helper for preventing resets of emulated
root hub endpoints and for argument validation.

Update other drivers which also define such callback to accept the
new argument and ignore it, as it seems to be of no use for them.

This fixes a 6.15-rc1 regression reported by Paul, which I was able
to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
endpoint_disable() not followed by add_endpoint(). If a configured
device is reset, stalling endpoints start to get stuck permanently.

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

New in v2:
Updated more HCDs after a kernel test robot build failure report.
This time I searched the whole tree and no more HCDs should be left.

 drivers/usb/core/hcd.c            |  2 +-
 drivers/usb/dwc2/hcd.c            |  1 +
 drivers/usb/fotg210/fotg210-hcd.c |  2 +-
 drivers/usb/host/ehci-hcd.c       |  3 ++-
 drivers/usb/host/xhci.c           | 27 ++++++++-------------------
 include/linux/usb/hcd.h           |  2 +-
 6 files changed, 14 insertions(+), 23 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/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 60ef8092259a..914a35c34db3 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4861,6 +4861,7 @@ static void _dwc2_hcd_endpoint_disable(struct usb_hcd *hcd,
  * routine.
  */
 static void _dwc2_hcd_endpoint_reset(struct usb_hcd *hcd,
+				     struct usb_device *udev,
 				     struct usb_host_endpoint *ep)
 {
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
diff --git a/drivers/usb/fotg210/fotg210-hcd.c b/drivers/usb/fotg210/fotg210-hcd.c
index 64c4965a160f..7163fce145fe 100644
--- a/drivers/usb/fotg210/fotg210-hcd.c
+++ b/drivers/usb/fotg210/fotg210-hcd.c
@@ -5426,7 +5426,7 @@ static void fotg210_endpoint_disable(struct usb_hcd *hcd,
 	spin_unlock_irqrestore(&fotg210->lock, flags);
 }
 
-static void fotg210_endpoint_reset(struct usb_hcd *hcd,
+static void fotg210_endpoint_reset(struct usb_hcd *hcd, struct usb_device *dev,
 		struct usb_host_endpoint *ep)
 {
 	struct fotg210_hcd *fotg210 = hcd_to_fotg210(hcd);
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 52d1693f4c2e..f67cd7c6422b 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

On 15.4.2025 12.10, Michal Pecio wrote:
> xHCI needs usb_device here, so it stored it in host_endpoint.hcpriv,
> which proved problematic due to some unexpected call sequences from
> USB core, and generally made the code more complex than it has to be.
> 
> Make USB core supply it directly and simplify xhci_endpoint_reset().
> Use the xhci_check_args() helper for preventing resets of emulated
> root hub endpoints and for argument validation.
> 
> Update other drivers which also define such callback to accept the
> new argument and ignore it, as it seems to be of no use for them.
> 
> This fixes a 6.15-rc1 regression reported by Paul, which I was able
> to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
> endpoint_disable() not followed by add_endpoint(). If a configured
> device is reset, stalling endpoints start to get stuck permanently.
> 
> 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>
> ---

All xhci changes look good to me

Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>

On Thu, 17 Apr 2025 11:54:19 +0300, Mathias Nyman wrote:
> On 15.4.2025 12.10, Michal Pecio wrote:
> > xHCI needs usb_device here, so it stored it in host_endpoint.hcpriv,
> > which proved problematic due to some unexpected call sequences from
> > USB core, and generally made the code more complex than it has to
> > be.
> > 
> > Make USB core supply it directly and simplify xhci_endpoint_reset().
> > Use the xhci_check_args() helper for preventing resets of emulated
> > root hub endpoints and for argument validation.
> > 
> > Update other drivers which also define such callback to accept the
> > new argument and ignore it, as it seems to be of no use for them.
> > 
> > This fixes a 6.15-rc1 regression reported by Paul, which I was able
> > to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
> > endpoint_disable() not followed by add_endpoint(). If a configured
> > device is reset, stalling endpoints start to get stuck permanently.
> > 
> > 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>
> > ---  
> 
> All xhci changes look good to me
> 
> Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>

Thank you for the review.

I guess I should update the commit message, though?

Technically, the regression will be closed by the next usb-linus merge
due to EP_STALLED reverts, while this patch really fixes and old hidden
bug which I could probably do a better job at explaining.

Greg would like a "Fixes". I think the problem started somewhere here:
f5249461b504 xhci: Clear the host side toggle manually when endpoint is soft reset
18b74067ac78 xhci: Fix use-after-free regression in xhci clear hub TT implementation

The former introduced an endpoint_reset() which depends on ep->hcpriv,
the latter introduced an endpoint_disable() which clears ep->hcpriv.
Which of them was wrong depends on whether it is legal to expect hcpriv
to be preserved after endpoint_disable(), I honestly don't know.

I also don't know if it will make sense to fix this in stable, since
nobody apparently noticed before EP_STALLED. But a class driver which
tries to clear a not halted EP on a device that had been reset in the
past could create toggle mismatch. I have not yet found such a driver.

Michal

On Tue, Apr 15, 2025 at 11:10:03AM +0200, Michal Pecio wrote:
> xHCI needs usb_device here, so it stored it in host_endpoint.hcpriv,
> which proved problematic due to some unexpected call sequences from
> USB core, and generally made the code more complex than it has to be.
> 
> Make USB core supply it directly and simplify xhci_endpoint_reset().
> Use the xhci_check_args() helper for preventing resets of emulated
> root hub endpoints and for argument validation.
> 
> Update other drivers which also define such callback to accept the
> new argument and ignore it, as it seems to be of no use for them.
> 
> This fixes a 6.15-rc1 regression reported by Paul, which I was able
> to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
> endpoint_disable() not followed by add_endpoint(). If a configured
> device is reset, stalling endpoints start to get stuck permanently.

As this fixes a bug, can you add a Fixes: tag with the needed
information?

thanks,

greg k-h

On Tue, 15 Apr 2025 14:26:26 +0200, Greg Kroah-Hartman wrote:
> > This fixes a 6.15-rc1 regression reported by Paul, which I was able
> > to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
> > endpoint_disable() not followed by add_endpoint(). If a configured
> > device is reset, stalling endpoints start to get stuck permanently.
>
> As this fixes a bug, can you add a Fixes: tag with the needed
> information?

Hi Greg,

Sorry for bothering you, the real bug is that I forgot to carry over
the RFC tag from v1.

The 6.15 regression is currently solved by reverts Mathias sent you.

The underlying bug is much older, I would have to research where it
went wrong exactly. It was very obscure; a class driver would need to:

1. call usb_set_interface(), usb_reset_device() or something like that
2. submit some URBs to make the toggle/sequence state non-zero
3. call usb_clear_halt() on a not yet halted endpoint

Then the host endpoint wouldn't be reset, but the device would.

I know of drivers which do 1 and 2 or even 2 and 3, but I have not
yet encountered a driver doing all three in this order.

Regards,
Michal

On Wed, Apr 16, 2025 at 08:29:58AM +0200, Michał Pecio wrote:
> On Tue, 15 Apr 2025 14:26:26 +0200, Greg Kroah-Hartman wrote:
> > > This fixes a 6.15-rc1 regression reported by Paul, which I was able
> > > to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
> > > endpoint_disable() not followed by add_endpoint(). If a configured
> > > device is reset, stalling endpoints start to get stuck permanently.
> >
> > As this fixes a bug, can you add a Fixes: tag with the needed
> > information?
> 
> Hi Greg,
> 
> Sorry for bothering you, the real bug is that I forgot to carry over
> the RFC tag from v1.
> 
> The 6.15 regression is currently solved by reverts Mathias sent you.

Oh good!

> The underlying bug is much older, I would have to research where it
> went wrong exactly. It was very obscure; a class driver would need to:
> 
> 1. call usb_set_interface(), usb_reset_device() or something like that
> 2. submit some URBs to make the toggle/sequence state non-zero
> 3. call usb_clear_halt() on a not yet halted endpoint
> 
> Then the host endpoint wouldn't be reset, but the device would.
> 
> I know of drivers which do 1 and 2 or even 2 and 3, but I have not
> yet encountered a driver doing all three in this order.

Ick, I don't think we want the individual drivers to have to do this,
the host controller _should_ handle it as you are trying to do here.

Anyway, I'll let this one be on the list for now and wait for others to
review.

thanks,

greg k-h