| Message ID | 20240612022256.7365-1-ki.chiang65@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xhci: Don't issue Reset Device command to Etron xHCI host | expand |
On Wed, Jun 12, 2024 at 10:22:56AM +0800, Kuangyi Chiang wrote: > Sometimes hub driver does not recognize USB device that is connected > to external USB2.0 Hub when system resumes from S4. > > This happens when xHCI driver issue Reset Device command to inform > Etron xHCI host that USB device has been reset. > > Seems that Etron xHCI host can not perform this command correctly, > affecting that USB device. > > Instead, to aviod this, xHCI driver should reassign device slot ID > by calling xhci_free_dev() and then xhci_alloc_dev(), the effect is > the same. > > Add XHCI_ETRON_HOST quirk flag to invoke workaround in > xhci_discover_or_reset_device(). > > Cc: <stable@vger.kernel.org> > Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> > --- > drivers/usb/host/xhci-pci.c | 2 ++ > drivers/usb/host/xhci.c | 11 ++++++++++- > drivers/usb/host/xhci.h | 2 ++ > 3 files changed, 14 insertions(+), 1 deletion(-) Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Wed, Jun 12, 2024 at 10:22:56AM +0800, Kuangyi Chiang wrote: > Sometimes hub driver does not recognize USB device that is connected > to external USB2.0 Hub when system resumes from S4. > > This happens when xHCI driver issue Reset Device command to inform > Etron xHCI host that USB device has been reset. > > Seems that Etron xHCI host can not perform this command correctly, > affecting that USB device. > > Instead, to aviod this, xHCI driver should reassign device slot ID > by calling xhci_free_dev() and then xhci_alloc_dev(), the effect is > the same. How is freeing and then allocating the device doing anything? > > Add XHCI_ETRON_HOST quirk flag to invoke workaround in > xhci_discover_or_reset_device(). > > Cc: <stable@vger.kernel.org> > Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> > --- > drivers/usb/host/xhci-pci.c | 2 ++ > drivers/usb/host/xhci.c | 11 ++++++++++- > drivers/usb/host/xhci.h | 2 ++ > 3 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index 05881153883e..c7a88340a6f8 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -395,11 +395,13 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > pdev->device == PCI_DEVICE_ID_EJ168) { > xhci->quirks |= XHCI_RESET_ON_RESUME; > xhci->quirks |= XHCI_BROKEN_STREAMS; > + xhci->quirks |= XHCI_ETRON_HOST; > } > if (pdev->vendor == PCI_VENDOR_ID_ETRON && > pdev->device == PCI_DEVICE_ID_EJ188) { > xhci->quirks |= XHCI_RESET_ON_RESUME; > xhci->quirks |= XHCI_BROKEN_STREAMS; > + xhci->quirks |= XHCI_ETRON_HOST; > } > > if (pdev->vendor == PCI_VENDOR_ID_RENESAS && > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 37eb37b0affa..aba4164b0873 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -3752,6 +3752,15 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, > SLOT_STATE_DISABLED) > return 0; > > + if (xhci->quirks & XHCI_ETRON_HOST) { > + xhci_free_dev(hcd, udev); > + ret = xhci_alloc_dev(hcd, udev); Wait, why are you freeing and then allocating the same device? That needs a lot of documentation here. > + if (ret == 1) > + return 0; And why does the function return 1? > + else > + return -EINVAL; > + } > + > trace_xhci_discover_or_reset_device(slot_ctx); > > xhci_dbg(xhci, "Resetting device with slot ID %u\n", slot_id); > @@ -3862,7 +3871,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, > * disconnected, and all traffic has been stopped and the endpoints have been > * disabled. Free any HC data structures associated with that device. > */ > -static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) > +void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) Why is this now global if you are only calling it in the same file? > { > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > struct xhci_virt_device *virt_dev; > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 30415158ed3c..f46b4dcb0613 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1627,6 +1627,7 @@ struct xhci_hcd { > #define XHCI_RESET_TO_DEFAULT BIT_ULL(44) > #define XHCI_ZHAOXIN_TRB_FETCH BIT_ULL(45) > #define XHCI_ZHAOXIN_HOST BIT_ULL(46) > +#define XHCI_ETRON_HOST BIT_ULL(47) Defining bit quirks just based on the vendor seems wrong to me, as that information can be determined when needed at any time just by the pci id, right? So why is a quirk bit needed? Shouldn't the quirk bit say what the broken functionality is? Same probably for the XHCI_ZHAOXIN_HOST, but that's not your issue to solve... > > unsigned int num_active_eps; > unsigned int limit_active_eps; > @@ -1863,6 +1864,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg); > irqreturn_t xhci_irq(struct usb_hcd *hcd); > irqreturn_t xhci_msi_irq(int irq, void *hcd); > int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev); > +void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev); Should not be needed. thanks, greg k-h
Hi Greg, Greg KH <gregkh@linuxfoundation.org> 於 2024年6月12日 週三 下午3:07寫道： > > On Wed, Jun 12, 2024 at 10:22:56AM +0800, Kuangyi Chiang wrote: > > Sometimes hub driver does not recognize USB device that is connected > > to external USB2.0 Hub when system resumes from S4. > > > > This happens when xHCI driver issue Reset Device command to inform > > Etron xHCI host that USB device has been reset. > > > > Seems that Etron xHCI host can not perform this command correctly, > > affecting that USB device. > > > > Instead, to aviod this, xHCI driver should reassign device slot ID > > by calling xhci_free_dev() and then xhci_alloc_dev(), the effect is > > the same. > > How is freeing and then allocating the device doing anything? To replace the Reset Device command, I found another way to inform Etron xHCI host that the USB device has been reset, which is to issue the Disable Slot command and then the Enable Slot command to Etron xHCI host. The easy way to issue these commands in sequence is by calling xhci_free_dev() and then xhci_alloc_dev(). Apply this patch then the issue is gone. > > > > > Add XHCI_ETRON_HOST quirk flag to invoke workaround in > > xhci_discover_or_reset_device(). > > > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> > > --- > > drivers/usb/host/xhci-pci.c | 2 ++ > > drivers/usb/host/xhci.c | 11 ++++++++++- > > drivers/usb/host/xhci.h | 2 ++ > > 3 files changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > > index 05881153883e..c7a88340a6f8 100644 > > --- a/drivers/usb/host/xhci-pci.c > > +++ b/drivers/usb/host/xhci-pci.c > > @@ -395,11 +395,13 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > > pdev->device == PCI_DEVICE_ID_EJ168) { > > xhci->quirks |= XHCI_RESET_ON_RESUME; > > xhci->quirks |= XHCI_BROKEN_STREAMS; > > + xhci->quirks |= XHCI_ETRON_HOST; > > } > > if (pdev->vendor == PCI_VENDOR_ID_ETRON && > > pdev->device == PCI_DEVICE_ID_EJ188) { > > xhci->quirks |= XHCI_RESET_ON_RESUME; > > xhci->quirks |= XHCI_BROKEN_STREAMS; > > + xhci->quirks |= XHCI_ETRON_HOST; > > } > > > > if (pdev->vendor == PCI_VENDOR_ID_RENESAS && > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 37eb37b0affa..aba4164b0873 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -3752,6 +3752,15 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, > > SLOT_STATE_DISABLED) > > return 0; > > > > + if (xhci->quirks & XHCI_ETRON_HOST) { > > + xhci_free_dev(hcd, udev); > > + ret = xhci_alloc_dev(hcd, udev); > > Wait, why are you freeing and then allocating the same device? That > needs a lot of documentation here. OK, I will add a comment here. > > > + if (ret == 1) > > + return 0; > > And why does the function return 1? xhci_alloc_dev() function returns 1 on success. > > > + else > > + return -EINVAL; > > + } > > + > > trace_xhci_discover_or_reset_device(slot_ctx); > > > > xhci_dbg(xhci, "Resetting device with slot ID %u\n", slot_id); > > @@ -3862,7 +3871,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, > > * disconnected, and all traffic has been stopped and the endpoints have been > > * disabled. Free any HC data structures associated with that device. > > */ > > -static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) > > +void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) > > Why is this now global if you are only calling it in the same file? Try to fix compile error like this: drivers/usb/host/xhci.c: In function ‘xhci_discover_or_reset_device’: drivers/usb/host/xhci.c:3756:3: error: implicit declaration of function ‘xhci_free_dev’ [-Werror=implicit-function-declaration] OK, I will revert it and put xhci_free_dev() function declaration near xhci_discover_or_reset_device() in the same file. > > > > { > > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > struct xhci_virt_device *virt_dev; > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > > index 30415158ed3c..f46b4dcb0613 100644 > > --- a/drivers/usb/host/xhci.h > > +++ b/drivers/usb/host/xhci.h > > @@ -1627,6 +1627,7 @@ struct xhci_hcd { > > #define XHCI_RESET_TO_DEFAULT BIT_ULL(44) > > #define XHCI_ZHAOXIN_TRB_FETCH BIT_ULL(45) > > #define XHCI_ZHAOXIN_HOST BIT_ULL(46) > > +#define XHCI_ETRON_HOST BIT_ULL(47) > > Defining bit quirks just based on the vendor seems wrong to me, as that > information can be determined when needed at any time just by the pci > id, right? So why is a quirk bit needed? > > Shouldn't the quirk bit say what the broken functionality is? Same > probably for the XHCI_ZHAOXIN_HOST, but that's not your issue to > solve... The XHCI_ETRON_HOST quirk bit is used to solve more issues encountered only by Etron xHCI host. Would you like me to check the PCI ID like this? if (dev_is_pci(hcd->self.controller) && to_pci_dev(hcd->self.controller)->vendor == 0x1b6f) { ... } If so, I will modify it and resend this patch. > > > > > unsigned int num_active_eps; > > unsigned int limit_active_eps; > > @@ -1863,6 +1864,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg); > > irqreturn_t xhci_irq(struct usb_hcd *hcd); > > irqreturn_t xhci_msi_irq(int irq, void *hcd); > > int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev); > > +void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev); > > Should not be needed. OK, I will remove it and resend this patch. > > thanks, > > greg k-h Thanks, Kuangyi Chiang
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 05881153883e..c7a88340a6f8 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -395,11 +395,13 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) pdev->device == PCI_DEVICE_ID_EJ168) { xhci->quirks |= XHCI_RESET_ON_RESUME; xhci->quirks |= XHCI_BROKEN_STREAMS; + xhci->quirks |= XHCI_ETRON_HOST; } if (pdev->vendor == PCI_VENDOR_ID_ETRON && pdev->device == PCI_DEVICE_ID_EJ188) { xhci->quirks |= XHCI_RESET_ON_RESUME; xhci->quirks |= XHCI_BROKEN_STREAMS; + xhci->quirks |= XHCI_ETRON_HOST; } if (pdev->vendor == PCI_VENDOR_ID_RENESAS && diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 37eb37b0affa..aba4164b0873 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3752,6 +3752,15 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, SLOT_STATE_DISABLED) return 0; + if (xhci->quirks & XHCI_ETRON_HOST) { + 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); @@ -3862,7 +3871,7 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd, * disconnected, and all traffic has been stopped and the endpoints have been * disabled. Free any HC data structures associated with that device. */ -static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) +void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) { struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct xhci_virt_device *virt_dev; diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 30415158ed3c..f46b4dcb0613 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1627,6 +1627,7 @@ struct xhci_hcd { #define XHCI_RESET_TO_DEFAULT BIT_ULL(44) #define XHCI_ZHAOXIN_TRB_FETCH BIT_ULL(45) #define XHCI_ZHAOXIN_HOST BIT_ULL(46) +#define XHCI_ETRON_HOST BIT_ULL(47) unsigned int num_active_eps; unsigned int limit_active_eps; @@ -1863,6 +1864,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg); irqreturn_t xhci_irq(struct usb_hcd *hcd); irqreturn_t xhci_msi_irq(int irq, void *hcd); int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev); +void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev); int xhci_alloc_tt_info(struct xhci_hcd *xhci, struct xhci_virt_device *virt_dev, struct usb_device *hdev,
Sometimes hub driver does not recognize USB device that is connected to external USB2.0 Hub when system resumes from S4. This happens when xHCI driver issue Reset Device command to inform Etron xHCI host that USB device has been reset. Seems that Etron xHCI host can not perform this command correctly, affecting that USB device. Instead, to aviod this, xHCI driver should reassign device slot ID by calling xhci_free_dev() and then xhci_alloc_dev(), the effect is the same. Add XHCI_ETRON_HOST quirk flag to invoke workaround in xhci_discover_or_reset_device(). Cc: <stable@vger.kernel.org> Signed-off-by: Kuangyi Chiang <ki.chiang65@gmail.com> --- drivers/usb/host/xhci-pci.c | 2 ++ drivers/usb/host/xhci.c | 11 ++++++++++- drivers/usb/host/xhci.h | 2 ++ 3 files changed, 14 insertions(+), 1 deletion(-)