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@xxxxxxxxxxxxxxx> > Signed-off-by: Kuangyi Chiang <ki.chiang65@xxxxxxxxx> 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