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
