Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 2 May 2018 10:06:31 +0200
From:   Domenico Andreoli <domenico.andreoli@linux.com>
To:     Mathias Nyman <mathias.nyman@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>
Cc:     Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Marc Zyngier <marc.zyngier@arm.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Subject: Regression due to "Workaround for uPD72020x USB3 chips"
Message-ID: <20180502080631.sriwquzcky6t24c2@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Dear all,

  my home machine stopped to boot starting from kernel version 4.12.7.

The last message I read is about resetting some USB3 bus. It's 100%
reproducible also with any recent kernel up to 4.17.0-rc3.

I bisected down to the following commit:

commit 0e1f0eaed6c20db41ff61e024b361ee3ec9d686c (tag: my_broken_xhci)
Author: Marc Zyngier <marc.zyngier@arm.com>
Date:   Tue Aug 1 20:11:08 2017 -0500

    xhci: Reset Renesas uPD72020x USB controller for 32-bit DMA issue
    
    commit 8466489ef5ba48272ba4fa4ea9f8f403306de4c7 upstream.
    
    The Renesas uPD72020x XHCI controller seems to suffer from a really
    annoying bug, where it may retain some of its DMA programming across a XHCI
    reset, and despite the driver correctly programming new DMA addresses.
    This is visible if the device has been using 64-bit DMA addresses, and is
    then switched to using 32-bit DMA addresses.  The top 32 bits of the
    address (now zero) are ignored are replaced by the 32 bits from the
    *previous* programming.  Sticking with 64-bit DMA always works, but doesn't
    seem very appropriate.
    
    A PCI reset of the device restores the normal functionality, which is done
    at probe time.  Unfortunately, this has to be done before any quirk has
    been discovered, hence the intrusive nature of the fix.
    
    Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git drivers/usb/host/pci-quirks.c drivers/usb/host/pci-quirks.c
index 5f4ca7890435..c8f38649f749 100644
--- drivers/usb/host/pci-quirks.c
+++ drivers/usb/host/pci-quirks.c
@@ -1157,3 +1157,23 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
                        PCI_CLASS_SERIAL_USB, 8, quirk_usb_early_handoff);
+
+bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
+{
+       /*
+        * Our dear uPD72020{1,2} friend only partially resets when
+        * asked to via the XHCI interface, and may end up doing DMA
+        * at the wrong addresses, as it keeps the top 32bit of some
+        * addresses from its previous programming under obscure
+        * circumstances.
+        * Give it a good wack at probe time. Unfortunately, this
+        * needs to happen before we've had a chance to discover any
+        * quirk, or the system will be in a rather bad state.
+        */
+       if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+           (pdev->device == 0x0014 || pdev->device == 0x0015))
+               return true;
+
+       return false;
+}
+EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
diff --git drivers/usb/host/pci-quirks.h drivers/usb/host/pci-quirks.h
index 655994480198..5582cbafecd4 100644
--- drivers/usb/host/pci-quirks.h
+++ drivers/usb/host/pci-quirks.h
@@ -15,6 +15,7 @@ void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
 void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
 void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
+bool usb_xhci_needs_pci_reset(struct pci_dev *pdev);
 #else
 struct pci_dev;
 static inline void usb_amd_quirk_pll_disable(void) {}
diff --git drivers/usb/host/xhci-pci.c drivers/usb/host/xhci-pci.c
index 1ef622ededfd..cefa223f9f08 100644
--- drivers/usb/host/xhci-pci.c
+++ drivers/usb/host/xhci-pci.c
@@ -285,6 +285,13 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
        driver = (struct hc_driver *)id->driver_data;
 
+       /* For some HW implementation, a XHCI reset is just not enough... */
+       if (usb_xhci_needs_pci_reset(dev)) {
+               dev_info(&dev->dev, "Resetting\n");
+               if (pci_reset_function_locked(dev))
+                       dev_warn(&dev->dev, "Reset failed");
+       }
+
        /* Prevent runtime suspending between USB-2 and USB-3 initialization */
        pm_runtime_get_noresume(&dev->dev);
 
---

Commenting out the call to pci_reset_function_locked() makes 4.17.0-rc3
boot again so I think this bug qualifies as regression.

I investigated a bit, the culprit is pci_reset_secondary_bus().

pci_reset_function_locked ->
  __pci_reset_function_locked ->
    pci_reset_bridge_secondary_bus ->
      pcibios_reset_secondary_bus ->
        pci_reset_secondary_bus ->>>>>> here the system dies/hangs/stops

I can read the printk I put right before PCI_BRIDGE_CTL_BUS_RESET is
written in PCI_BRIDGE_CONTROL. I cannot read the printk I put right
after.

It seems my system doesn't like that PCI reset at all. I cannot swear
that it is completely frozen, some disk activity might be happening
shortly after, but my only option is to power cycle.

I cannot debug easily. I tried to boot with a patched module and just
attempted to work at it on a live machine but as soon I unload the
module I'm left without keyboard/mouse (apparently all the accessible
USB ports are going through the xhci-pci module) and at the moment I
cannot go via network.

This is the output of lspci -vt:

-[0000:00]-+-00.0  Intel Corporation 4th Gen Core Processor DRAM Controller
           +-01.0-[01]----00.0  NVIDIA Corporation GM108M [GeForce 840M]
           +-02.0  Intel Corporation Xeon E3-1200 v3/4th Gen Core Processor Integrated Graphics Controller
           +-03.0  Intel Corporation Xeon E3-1200 v3/4th Gen Core Processor HD Audio Controller
           +-14.0  Intel Corporation 8 Series/C220 Series Chipset Family USB xHCI
           +-16.0  Intel Corporation 8 Series/C220 Series Chipset Family MEI Controller #1
           +-1a.0  Intel Corporation 8 Series/C220 Series Chipset Family USB EHCI #2
           +-1b.0  Intel Corporation 8 Series/C220 Series Chipset High Definition Audio Controller
           +-1c.0-[02]--
           +-1c.2-[03]----00.0  Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller
           +-1c.3-[04]----00.0  Realtek Semiconductor Co., Ltd. RTS5229 PCI Express Card Reader
           +-1c.4-[05]----00.0  Realtek Semiconductor Co., Ltd. RTL8821AE 802.11ac PCIe Wireless Network Adapter
           +-1c.5-[06]----00.0  Renesas Technology Corp. uPD720202 USB 3.0 Host Controller
           +-1d.0  Intel Corporation 8 Series/C220 Series Chipset Family USB EHCI #1
           +-1f.0  Intel Corporation C220 Series Chipset Family H81 Express LPC Controller
           +-1f.2  Intel Corporation 8 Series/C220 Series Chipset Family 6-port SATA Controller 1 [AHCI mode]
           \-1f.3  Intel Corporation 8 Series/C220 Series Chipset Family SMBus Controller

The call to pci_reset_secondary_bus() is correctly applied to -1c.5-.

I suspect that -1c.5- might benefit from having PCI_DEV_FLAGS_NO_BUS_RESET
in its dev->dev_flags but I'm not sure that it's the proper fix and how
exactly it could end up there.

Any suggestion on how to proceed further?

Please ask more info if needed.

Kind regards,
Domenico

-- 
3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13