Re: [PATCH] usb: xhci: add quirk flag for broken stop command on AMD platforms

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 18/05/17 11:53, Shyam Sundar S K wrote:
> on AMD platforms with SNPS 3.1 USB controller has an issue
> if the stop EP command is issued when the controller is not
> in running state. If issued, it is leading to a critical RTL bug
> because of which controller goes into irrecoverable state.
> 
> This patch adds a appropriate checks to make sure that scenario
> does not happen.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@xxxxxxx>
> ---
>  drivers/usb/host/xhci-hub.c | 36 ++++++++++++++++++++++++++++++++++--
>  drivers/usb/host/xhci-pci.c |  8 ++++++++
>  drivers/usb/host/xhci.h     |  1 +
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 5e3e9d4..fec849f 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -380,6 +380,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>  {
>  	struct xhci_virt_device *virt_dev;
>  	struct xhci_command *cmd;
> +	struct xhci_ep_ctx *ep_ctx;
>  	unsigned long flags;
>  	int ret;
>  	int i;
> @@ -407,11 +408,42 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>  				return -ENOMEM;
>  
>  			}
> -			xhci_queue_stop_endpoint(xhci, command, slot_id, i,
> +
> +			/* on AMD platforms with SNPS 3.1 USB controller has
> +			 * an issue if the stop EP command is issued when
> +			 * the controller is not in running state.
> +			 * If issued, its leading to a RTL bug because of
> +			 * which controller goes to a bad state.
> +			 * Adding a conditional check to prevent this.
> +			 */
> +
> +			if (xhci->quirks & XHCI_BROKEN_STOP) {
> +				ep_ctx = xhci_get_ep_ctx(xhci,
> +						virt_dev->out_ctx, i);
> +
> +				if (GET_EP_CTX_STATE(ep_ctx) ==
> +							EP_STATE_RUNNING) {

In the comment you mention that we need to avoid the stop EP command when
controller is not running but here you are checking if EP is not running.

Is Controller not running and EP not running the same?

If we're not queuing the stop endpoint command do we need still to allocate it
and ring the doorbell?

> +					xhci_queue_stop_endpoint(xhci, command,
> +							slot_id, i, suspend);
> +				}
> +			} else {
> +				xhci_queue_stop_endpoint(xhci, command,
> +						slot_id, i, suspend);
> +			}
> +		}
> +	}
> +
> +	if (xhci->quirks & XHCI_BROKEN_STOP) {
> +		ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->out_ctx, 0);
> +
> +		if (GET_EP_CTX_STATE(ep_ctx) ==  EP_STATE_RUNNING) {
> +			xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0,
>  						 suspend);
>  		}
> +	} else {
> +		xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
>  	}
> -	xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
> +
>  	xhci_ring_cmd_db(xhci);
>  	spin_unlock_irqrestore(&xhci->lock, flags);

What happens to the wait for completion code that is right after this?
Since we didn't send the command there is no point waiting for it to complete right?

>  
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 7b86508..403de8f 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -52,6 +52,8 @@
>  #define PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI		0x0aa8
>  #define PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI		0x1aa8
>  #define PCI_DEVICE_ID_INTEL_APL_XHCI			0x5aa8
> +#define PCI_DEVICE_ID_AMD_XHCI_30			0x15e0
> +#define PCI_DEVICE_ID_AMD_XHCI_31			0x15e1
>  
>  static const char hcd_name[] = "xhci_hcd";
>  
> @@ -205,6 +207,12 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  	if (xhci->quirks & XHCI_RESET_ON_RESUME)
>  		xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
>  				"QUIRK: Resetting on resume");
> +
> +	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +	    (pdev->device == PCI_DEVICE_ID_AMD_XHCI_30 ||
> +		pdev->device == PCI_DEVICE_ID_AMD_XHCI_31))
> +		xhci->quirks |= XHCI_BROKEN_STOP;
> +
>  }
>  
>  #ifdef CONFIG_ACPI
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 73a28a9..c6fa3ca 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1819,6 +1819,7 @@ struct xhci_hcd {
>  /* For controller with a broken Port Disable implementation */
>  #define XHCI_BROKEN_PORT_PED	(1 << 25)
>  #define XHCI_LIMIT_ENDPOINT_INTERVAL_7	(1 << 26)
> +#define XHCI_BROKEN_STOP	(1 << 27)
>  
>  	unsigned int		num_active_eps;
>  	unsigned int		limit_active_eps;
> 

-- 
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux