Re: [PATCH v2] xhci: Don't issue Reset Device command to Etron xHCI host

[Greg KH <gregkh@linuxfoundation.org>]

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