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
