| Message ID | 20240619054808.12861-1-ki.chiang65@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xhci: Don't issue Reset Device command to Etron xHCI host | expand |
On Wed, Jun 19, 2024 at 01:48:08PM +0800, Kuangyi Chiang wrote: > Sometimes the hub driver does not recognize the USB device connected > to the external USB2.0 hub when the system resumes from S4. > > This happens when the xHCI driver issue the Reset Device command to > inform the Etron xHCI host that the USB device has been reset. > > Seems that the Etron xHCI host can not perform this command correctly, > affecting the USB device. > > Instead, to avoid this, disabling slot ID and then enabling slot ID > is a workable solution to replace the Reset Device command. > > An easy way to issue these commands in sequence is to call > xhci_free_dev() and then xhci_alloc_dev(). > > Applying this patch then the issue is gone. > > Cc: <stable@vger.kernel.org> > Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> What commit id does this fix? > --- > Changes in v2: > - Change commit log > - Add a comment for the workaround > - Revert "global xhci_free_dev()" > - Remove XHCI_ETRON_HOST quirk bit > > drivers/usb/host/xhci.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 37eb37b0affa..c892750a89c5 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -3682,6 +3682,8 @@ void xhci_free_device_endpoint_resources(struct xhci_hcd *xhci, > xhci->num_active_eps); > } > > +static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev); > + > /* > * This submits a Reset Device Command, which will set the device state to 0, > * set the device address to 0, and disable all the endpoints except the default > @@ -3752,6 +3754,20 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, > SLOT_STATE_DISABLED) > return 0; > > + if (dev_is_pci(hcd->self.controller) && > + to_pci_dev(hcd->self.controller)->vendor == 0x1b6f) { Odd indentation :( Also, that's a specific value, shouldn't it be in a #define somewhere? > + /* > + * Disabling and then enabling device slot ID to inform xHCI > + * host that the USB device has been reset. > + */ > + xhci_free_dev(hcd, udev); > + ret = xhci_alloc_dev(hcd, udev); You are relying on the behavior of free/alloc here to disable/enable the slot id, why not just do that instead? What happens if the free/alloc call stops doing that? This feels very fragile to me. > + if (ret == 1) > + return 0; > + else > + return -EINVAL; Why -EINVAL? What value was wrong? thanks, greg k-h
Hi Greg, Greg KH <gregkh@linuxfoundation.org> 於 2024年6月19日 週三 下午2:15寫道： > > On Wed, Jun 19, 2024 at 01:48:08PM +0800, Kuangyi Chiang wrote: > > Sometimes the hub driver does not recognize the USB device connected > > to the external USB2.0 hub when the system resumes from S4. > > > > This happens when the xHCI driver issue the Reset Device command to > > inform the Etron xHCI host that the USB device has been reset. > > > > Seems that the Etron xHCI host can not perform this command correctly, > > affecting the USB device. > > > > Instead, to avoid this, disabling slot ID and then enabling slot ID > > is a workable solution to replace the Reset Device command. > > > > An easy way to issue these commands in sequence is to call > > xhci_free_dev() and then xhci_alloc_dev(). > > > > Applying this patch then the issue is gone. > > > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> > > What commit id does this fix? Fixes: 2a8f82c4ceaf ("USB: xhci: Notify the xHC when a device is reset.") However, this patch is a workaround for Etron xHCI hosts, should I add this in the commit message? > > > --- > > Changes in v2: > > - Change commit log > > - Add a comment for the workaround > > - Revert "global xhci_free_dev()" > > - Remove XHCI_ETRON_HOST quirk bit > > > > drivers/usb/host/xhci.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 37eb37b0affa..c892750a89c5 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -3682,6 +3682,8 @@ void xhci_free_device_endpoint_resources(struct xhci_hcd *xhci, > > xhci->num_active_eps); > > } > > > > +static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev); > > + > > /* > > * This submits a Reset Device Command, which will set the device state to 0, > > * set the device address to 0, and disable all the endpoints except the default > > @@ -3752,6 +3754,20 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, > > SLOT_STATE_DISABLED) > > return 0; > > > > + if (dev_is_pci(hcd->self.controller) && > > + to_pci_dev(hcd->self.controller)->vendor == 0x1b6f) { > > Odd indentation :( Oops, one tab is missing, right? I will modify it. > > Also, that's a specific value, shouldn't it be in a #define somewhere? OK, I will add a #define near xhci_discover_or_reset_device() in the same file. > > > + /* > > + * Disabling and then enabling device slot ID to inform xHCI > > + * host that the USB device has been reset. > > + */ > > + xhci_free_dev(hcd, udev); > > + ret = xhci_alloc_dev(hcd, udev); > > You are relying on the behavior of free/alloc here to disable/enable the > slot id, why not just do that instead? What happens if the free/alloc > call stops doing that? This feels very fragile to me. > These functions are helpers that can be used to enable/disable the slot id and allocate/free associated data structures, I think they should be called, right? Or you would like to call xhci_disable_slot() + xhci_alloc_dev(), as in commit 651aaf36a7d7 ("usb: xhci: Handle USB transaction error on address command"). If so, I will modify it and resend this patch. > > + if (ret == 1) > > + return 0; > > + else > > + return -EINVAL; > > Why -EINVAL? What value was wrong? I followed commit f0615c45ce5f ("USB: xHCI: change xhci_reset_device() to allocate new device") to return -EINVAL, I think it means running out of device slots. > > thanks, > > greg k-h Thanks, Kuangyi Chiang
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 37eb37b0affa..c892750a89c5 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3682,6 +3682,8 @@ void xhci_free_device_endpoint_resources(struct xhci_hcd *xhci, xhci->num_active_eps); } +static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev); + /* * This submits a Reset Device Command, which will set the device state to 0, * set the device address to 0, and disable all the endpoints except the default @@ -3752,6 +3754,20 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, SLOT_STATE_DISABLED) return 0; + if (dev_is_pci(hcd->self.controller) && + to_pci_dev(hcd->self.controller)->vendor == 0x1b6f) { + /* + * Disabling and then enabling device slot ID to inform xHCI + * host that the USB device has been reset. + */ + xhci_free_dev(hcd, udev); + ret = xhci_alloc_dev(hcd, udev); + if (ret == 1) + return 0; + else + return -EINVAL; + } + trace_xhci_discover_or_reset_device(slot_ctx); xhci_dbg(xhci, "Resetting device with slot ID %u\n", slot_id);
Sometimes the hub driver does not recognize the USB device connected to the external USB2.0 hub when the system resumes from S4. This happens when the xHCI driver issue the Reset Device command to inform the Etron xHCI host that the USB device has been reset. Seems that the Etron xHCI host can not perform this command correctly, affecting the USB device. Instead, to avoid this, disabling slot ID and then enabling slot ID is a workable solution to replace the Reset Device command. An easy way to issue these commands in sequence is to call xhci_free_dev() and then xhci_alloc_dev(). Applying this patch then the issue is gone. Cc: <stable@vger.kernel.org> Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> --- Changes in v2: - Change commit log - Add a comment for the workaround - Revert "global xhci_free_dev()" - Remove XHCI_ETRON_HOST quirk bit drivers/usb/host/xhci.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)