linux-kernel - Re: [PATCH v2] xhci: Add quirk to workaround the errata seen on Cavium Thunder-X2 Soc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3b009211-6c86-ffdb-424f-5ed09b01e772@caviumnetworks.com>
Date:   Tue, 30 Oct 2018 09:50:19 +0000
From:   George Cherian <gcherian@...iumnetworks.com>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>,
        "Cherian, George" <George.Cherian@...ium.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "mathias.nyman@...el.com" <mathias.nyman@...el.com>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>
Subject: Re: [PATCH v2] xhci: Add quirk to workaround the errata seen on
 Cavium Thunder-X2 Soc

Hi Mathias,

Thanks for the review.

On 10/30/2018 03:20 PM, Mathias Nyman wrote:
> 
> On 29.10.2018 18:54, Cherian, George wrote:
>> Implement workaround for ThunderX2 Errata-129 (documented in
>> CN99XX Known Issues" available at Cavium support site).
>> As per ThunderX2errata-129, USB 2 device may come up as USB 1
>> if a connection to a USB 1 device is followed by another connection to
>> a USB 2 device, the link will come up as USB 1 for the USB 2 device.
>>
>> Resolution: Reset the PHY after the USB 1 device is disconnected.
>> The PHY reset sequence is done using private registers in XHCI register
>> space. After the PHY is reset we check for the PLL lock status and retry
>> the operation if it fails. From our tests, retrying 4 times is 
>> sufficient.
>>
>> Add a new quirk flag XHCI_RESET_PLL_ON_DISCONNECT to invoke the 
>> workaround
>> in handle_xhci_port_status().
>>
>> Signed-off-by: George Cherian <george.cherian@...ium.com>
>> ---
>>   drivers/usb/host/xhci-pci.c  |  5 +++++
>>   drivers/usb/host/xhci-ring.c | 35 ++++++++++++++++++++++++++++++++++-
>>   drivers/usb/host/xhci.h      |  1 +
>>   3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 51dd8e0..334c009 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -231,6 +231,11 @@ static void xhci_pci_quirks(struct device *dev, 
>> struct xhci_hcd *xhci)
>>       if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
>>               xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
>>
>> +     if ((pdev->vendor == PCI_VENDOR_ID_BROADCOM ||
>> +          pdev->vendor == PCI_VENDOR_ID_CAVIUM) &&
>> +          pdev->device == 0x9026)
>> +             xhci->quirks |= XHCI_RESET_PLL_ON_DISCONNECT;
>> +
>>       if (xhci->quirks & XHCI_RESET_ON_RESUME)
>>               xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
>>                               "QUIRK: Resetting on resume");
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index f0a99aa..342dd5ac 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1517,6 +1517,35 @@ static void handle_device_notification(struct 
>> xhci_hcd *xhci,
>>               usb_wakeup_notification(udev->parent, udev->portnum);
>>   }
>>
>> +/*
>> + * Quirk hanlder for errata seen on Cavium ThunderX2 processor XHCI
>> + * Controller.
>> + * As per ThunderX2errata-129 USB 2 device may come up as USB 1
>> + * If a connection to a USB 1 device is followed by another connection
>> + * to a USB 2 device.
>> + *
>> + * Reset the PHY after the USB device is disconnected if device speed
>> + * is less than HCD_USB3.
>> + * Retry the reset sequence max of 4 times checking the PLL lock status.
>> + *
>> + */
>> +static void xhci_handle_tx2_wrapper_reset(struct xhci_hcd *xhci)
> 
> Maybe rename this to make it more obvious it's about a quirk.
> xhci_cavium_reset_phy_quirk() maybe?
Yes will do
> 
>> +{
>> +     struct usb_hcd *hcd = xhci_to_hcd(xhci);
>> +     u32 pll_lock_check;
>> +     u32 retry_count = 4;
>> +
>> +     do {
>> +             /* Assert PHY reset */
>> +             writel(0x6F, hcd->regs + 0x1048);
>> +             udelay(10);
>> +             /* De-assert the PHY reset */
>> +             writel(0x7F, hcd->regs + 0x1048);
>> +             udelay(200);
>> +             pll_lock_check = readl(hcd->regs + 0x1070);
>> +     } while (!(pll_lock_check & 0x1) && --retry_count);
>> +}
>> +
>>   static void handle_port_status(struct xhci_hcd *xhci,
>>               union xhci_trb *event)
>>   {
>> @@ -1642,8 +1671,12 @@ static void handle_port_status(struct xhci_hcd 
>> *xhci,
>>               goto cleanup;
>>       }
>>
>> -     if (hcd->speed < HCD_USB3)
>> +     if (hcd->speed < HCD_USB3) {
>>               xhci_test_and_clear_bit(xhci, port, PORT_PLC);
>> +             if ((portsc & PORT_CSC) && !(portsc & 0x1) &&
>> +                 (xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT))
>> +                     xhci_handle_tx2_wrapper_reset(xhci);
>> +     }
> 
> Check for the quirk first, this way I can ignore the rest of the code when
> debugging other issues,  and use PORT_CONNECT instead of 0x1
> 
> something like:
> 
> if ((xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT) &&
>      (portsc & PORT_CSC) && !(portsc & PORT_CONNECT))

Yes I will do the changes and post new version.
> 
> Thanks
> -Mathias

Regards
-George