| Message ID | 20250415111003.064e0ab8@foxbook (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: hcd: Add a usb_device argument to hc_driver.endpoint_reset() | expand |
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
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 */
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(-)