From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C30E9C28D1E for ; Thu, 6 Jun 2019 11:24:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8EA8B20868 for ; Thu, 6 Jun 2019 11:24:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727190AbfFFLYg (ORCPT ); Thu, 6 Jun 2019 07:24:36 -0400 Received: from mga06.intel.com ([134.134.136.31]:46092 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726961AbfFFLYg (ORCPT ); Thu, 6 Jun 2019 07:24:36 -0400 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jun 2019 04:24:35 -0700 X-ExtLoop1: 1 Received: from lahna.fi.intel.com (HELO lahna) ([10.237.72.157]) by fmsmga001.fm.intel.com with SMTP; 06 Jun 2019 04:24:31 -0700 Received: by lahna (sSMTP sendmail emulation); Thu, 06 Jun 2019 14:24:30 +0300 Date: Thu, 6 Jun 2019 14:24:30 +0300 From: Mika Westerberg To: "Rafael J. Wysocki" Cc: Bjorn Helgaas , "Rafael J. Wysocki" , Len Brown , Lukas Wunner , Keith Busch , Alex Williamson , Alexandru Gagniuc , ACPI Devel Maling List , Linux PCI Subject: Re: [PATCH 1/3] PCI: Add missing link delays required by the PCIe spec Message-ID: <20190606112430.GZ2781@lahna.fi.intel.com> References: <20190605145820.37169-1-mika.westerberg@linux.intel.com> <20190605145820.37169-2-mika.westerberg@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.11.4 (2019-03-13) Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Thu, Jun 06, 2019 at 10:40:48AM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 5, 2019 at 4:58 PM Mika Westerberg > wrote: > > > > Currently Linux does not follow PCIe spec regarding the required delays > > after reset. A concrete example is a Thunderbolt add-in-card that > > consists of a PCIe switch and two PCIe endpoints: > > > > +-1b.0-[01-6b]----00.0-[02-6b]--+-00.0-[03]----00.0 TBT controller > > +-01.0-[04-36]-- DS hotplug port > > +-02.0-[37]----00.0 xHCI controller > > \-04.0-[38-6b]-- DS hotplug port > > > > The root port (1b.0) and the PCIe switch downstream ports are all PCIe > > gen3 so they support 8GT/s link speeds. > > > > We wait for the PCIe hierarchy to enter D3cold (runtime): > > > > pcieport 0000:00:1b.0: power state changed by ACPI to D3cold > > > > When it wakes up from D3cold, according to the PCIe 4.0 section 5.8 the > > PCIe switch is put to reset and its power is re-applied. This means that > > we must follow the rules in PCIe 4.0 section 6.6.1. > > > > For the PCIe gen3 ports we are dealing with here, the following applies: > > > > With a Downstream Port that supports Link speeds greater than 5.0 > > GT/s, software must wait a minimum of 100 ms after Link training > > completes before sending a Configuration Request to the device > > immediately below that Port. Software can determine when Link training > > completes by polling the Data Link Layer Link Active bit or by setting > > up an associated interrupt (see Section 6.7.3.3). > > > > Translating this into the above topology we would need to do this (DLLLA > > stands for Data Link Layer Link Active): > > > > pcieport 0000:00:1b.0: wait for 100ms after DLLLA is set before access to 0000:01:00.0 > > pcieport 0000:02:00.0: wait for 100ms after DLLLA is set before access to 0000:03:00.0 > > pcieport 0000:02:02.0: wait for 100ms after DLLLA is set before access to 0000:37:00.0 > > > > I've instrumented the kernel with additional logging so we can see the > > actual delays the kernel performs: > > > > pcieport 0000:00:1b.0: power state changed by ACPI to D0 > > pcieport 0000:00:1b.0: waiting for D3cold delay of 100 ms > > pcieport 0000:00:1b.0: waking up bus > > pcieport 0000:00:1b.0: waiting for D3hot delay of 10 ms > > pcieport 0000:00:1b.0: restoring config space at offset 0x2c (was 0x60, writing 0x60) > > ... > > pcieport 0000:00:1b.0: PME# disabled > > pcieport 0000:01:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > ... > > pcieport 0000:01:00.0: PME# disabled > > pcieport 0000:02:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > ... > > pcieport 0000:02:00.0: PME# disabled > > pcieport 0000:02:01.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > ... > > pcieport 0000:02:01.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) > > pcieport 0000:02:01.0: PME# disabled > > pcieport 0000:02:02.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > ... > > pcieport 0000:02:02.0: PME# disabled > > pcieport 0000:02:04.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > ... > > pcieport 0000:02:04.0: PME# disabled > > pcieport 0000:02:01.0: PME# enabled > > pcieport 0000:02:01.0: waiting for D3hot delay of 10 ms > > pcieport 0000:02:04.0: PME# enabled > > pcieport 0000:02:04.0: waiting for D3hot delay of 10 ms > > thunderbolt 0000:03:00.0: restoring config space at offset 0x14 (was 0x0, writing 0x8a040000) > > ... > > thunderbolt 0000:03:00.0: PME# disabled > > xhci_hcd 0000:37:00.0: restoring config space at offset 0x10 (was 0x0, writing 0x73f00000) > > ... > > xhci_hcd 0000:37:00.0: PME# disabled > > > > For the switch upstream port (01:00.0) we wait for 100ms but not taking > > into account the DLLLA requirement. We then wait 10ms for D3hot -> D0 > > transition of the root port and the two downstream hotplug ports. This > > means that we deviate from what the spec requires. > > > > Performing the same check for system sleep (s2idle) transitions we can > > see following when resuming from s2idle: > > > > pcieport 0000:00:1b.0: power state changed by ACPI to D0 > > pcieport 0000:00:1b.0: restoring config space at offset 0x2c (was 0x60, writing 0x60) > > ... > > pcieport 0000:01:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > ... > > pcieport 0000:02:02.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > pcieport 0000:02:02.0: restoring config space at offset 0x2c (was 0x0, writing 0x0) > > pcieport 0000:02:01.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > pcieport 0000:02:04.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > pcieport 0000:02:02.0: restoring config space at offset 0x28 (was 0x0, writing 0x0) > > pcieport 0000:02:00.0: restoring config space at offset 0x3c (was 0x1ff, writing 0x201ff) > > pcieport 0000:02:02.0: restoring config space at offset 0x24 (was 0x10001, writing 0x1fff1) > > pcieport 0000:02:01.0: restoring config space at offset 0x2c (was 0x0, writing 0x60) > > pcieport 0000:02:02.0: restoring config space at offset 0x20 (was 0x0, writing 0x73f073f0) > > pcieport 0000:02:04.0: restoring config space at offset 0x2c (was 0x0, writing 0x60) > > pcieport 0000:02:01.0: restoring config space at offset 0x28 (was 0x0, writing 0x60) > > pcieport 0000:02:00.0: restoring config space at offset 0x2c (was 0x0, writing 0x0) > > pcieport 0000:02:02.0: restoring config space at offset 0x1c (was 0x101, writing 0x1f1) > > pcieport 0000:02:04.0: restoring config space at offset 0x28 (was 0x0, writing 0x60) > > pcieport 0000:02:01.0: restoring config space at offset 0x24 (was 0x10001, writing 0x1ff10001) > > pcieport 0000:02:00.0: restoring config space at offset 0x28 (was 0x0, writing 0x0) > > pcieport 0000:02:02.0: restoring config space at offset 0x18 (was 0x0, writing 0x373702) > > pcieport 0000:02:04.0: restoring config space at offset 0x24 (was 0x10001, writing 0x49f12001) > > pcieport 0000:02:01.0: restoring config space at offset 0x20 (was 0x0, writing 0x73e05c00) > > pcieport 0000:02:00.0: restoring config space at offset 0x24 (was 0x10001, writing 0x1fff1) > > pcieport 0000:02:04.0: restoring config space at offset 0x20 (was 0x0, writing 0x89f07400) > > pcieport 0000:02:01.0: restoring config space at offset 0x1c (was 0x101, writing 0x5151) > > pcieport 0000:02:00.0: restoring config space at offset 0x20 (was 0x0, writing 0x8a008a00) > > pcieport 0000:02:02.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) > > pcieport 0000:02:04.0: restoring config space at offset 0x1c (was 0x101, writing 0x6161) > > pcieport 0000:02:01.0: restoring config space at offset 0x18 (was 0x0, writing 0x360402) > > pcieport 0000:02:00.0: restoring config space at offset 0x1c (was 0x101, writing 0x1f1) > > pcieport 0000:02:04.0: restoring config space at offset 0x18 (was 0x0, writing 0x6b3802) > > pcieport 0000:02:02.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) > > pcieport 0000:02:00.0: restoring config space at offset 0x18 (was 0x0, writing 0x30302) > > pcieport 0000:02:01.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) > > pcieport 0000:02:04.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) > > pcieport 0000:02:00.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020) > > pcieport 0000:02:01.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) > > pcieport 0000:02:04.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) > > pcieport 0000:02:00.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100407) > > xhci_hcd 0000:37:00.0: restoring config space at offset 0x10 (was 0x0, writing 0x73f00000) > > ... > > thunderbolt 0000:03:00.0: restoring config space at offset 0x14 (was 0x0, writing 0x8a040000) > > > > This is even worse. None of the mandatory delays are performed. If this > > would be S3 instead of s2idle then according to PCI FW spec 3.2 section > > 4.6.8. there is a specific _DSM that allows the OS to skip the delays > > but this platform does not provide the _DSM and does not go to S3 anyway > > so no firmware is involved that could already handle these delays. > > > > In this particular Intel Coffee Lake platform these delays are not > > actually needed because there is an additional delay as part of the ACPI > > power resource that is used to turn on power to the hierarchy but since > > that additional delay is not required by any of standards (PCIe, ACPI) > > it is not present in the Intel Ice Lake, for example where missing the > > mandatory delays causes pciehp to start tearing down the stack too early > > (links are not yet trained). > > > > For this reason, change the PCIe portdrv PM resume hooks so that they > > perform the mandatory delays before the downstream component gets > > resumed. We perform the delays before port services are resumed because > > otherwise pciehp might find that the link is not up (even if it is just > > training) and tears-down the hierarchy. > > > > Signed-off-by: Mika Westerberg > > Generally > > Reviewed-by: Rafael J. Wysocki Thanks! > with a couple of nits below. > > > --- > > drivers/pci/pci.c | 29 +++++++++------ > > drivers/pci/pci.h | 1 + > > drivers/pci/pcie/portdrv_core.c | 62 +++++++++++++++++++++++++++++++++ > > 3 files changed, 82 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 8abc843b1615..87a1f902fa8e 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1004,15 +1004,10 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state) > > if (state == PCI_D0) { > > pci_platform_power_transition(dev, PCI_D0); > > /* > > - * Mandatory power management transition delays, see > > - * PCI Express Base Specification Revision 2.0 Section > > - * 6.6.1: Conventional Reset. Do not delay for > > - * devices powered on/off by corresponding bridge, > > - * because have already delayed for the bridge. > > + * Mandatory power management transition delays are > > + * handled in the PCIe portdrv resume hooks. > > */ > > if (dev->runtime_d3cold) { > > - if (dev->d3cold_delay && !dev->imm_ready) > > - msleep(dev->d3cold_delay); > > /* > > * When powering on a bridge from D3cold, the > > * whole hierarchy may be powered on into > > @@ -4568,14 +4563,16 @@ static int pci_pm_reset(struct pci_dev *dev, int probe) > > > > return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS); > > } > > + > > /** > > - * pcie_wait_for_link - Wait until link is active or inactive > > + * pcie_wait_for_link_delay - Wait until link is active or inactive > > * @pdev: Bridge device > > * @active: waiting for active or inactive? > > + * @delay: Delay to wait after link has become active (in ms) > > * > > * Use this to wait till link becomes active or inactive. > > */ > > -bool pcie_wait_for_link(struct pci_dev *pdev, bool active) > > +bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active, int delay) > > { > > int timeout = 1000; > > bool ret; > > @@ -4612,13 +4609,25 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active) > > timeout -= 10; > > } > > if (active && ret) > > - msleep(100); > > + msleep(delay); > > else if (ret != active) > > pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n", > > active ? "set" : "cleared"); > > return ret == active; > > } > > > > +/** > > + * pcie_wait_for_link - Wait until link is active or inactive > > + * @pdev: Bridge device > > + * @active: waiting for active or inactive? > > + * > > + * Use this to wait till link becomes active or inactive. > > + */ > > +bool pcie_wait_for_link(struct pci_dev *pdev, bool active) > > +{ > > + return pcie_wait_for_link_delay(pdev, active, 100); > > +} > > + > > void pci_reset_secondary_bus(struct pci_dev *dev) > > { > > u16 ctrl; > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index 9cb99380c61e..59802b3def4b 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -493,6 +493,7 @@ static inline int pci_dev_specific_disable_acs_redir(struct pci_dev *dev) > > void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, > > u32 service); > > > > +bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active, int delay); > > bool pcie_wait_for_link(struct pci_dev *pdev, bool active); > > #ifdef CONFIG_PCIEASPM > > void pcie_aspm_init_link_state(struct pci_dev *pdev); > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > index 1b330129089f..88d151a54be6 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -9,6 +9,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -378,6 +379,58 @@ static int pm_iter(struct device *dev, void *data) > > return 0; > > } > > > > +static int get_downstream_delay(struct pci_bus *bus) > > +{ > > + struct pci_dev *pdev; > > + int min_delay = 100; > > + int max_delay = 0; > > + > > + list_for_each_entry(pdev, &bus->devices, bus_list) { > > + if (!pdev->imm_ready) > > + min_delay = 0; > > + else if (pdev->d3cold_delay < min_delay) > > + min_delay = pdev->d3cold_delay; > > + if (pdev->d3cold_delay > max_delay) > > + max_delay = pdev->d3cold_delay; > > + } > > + > > + return max(min_delay, max_delay); > > +} > > + > > +static void wait_for_downstream_link(struct pci_dev *pdev) > > +{ > > + /* > > + * Handle delays according to PCIe 4.0 section 6.6.1 before > > + * configuration access to the downstream component is permitted. > > + * > > + * This blocks PCI core resume of the hierarchy below this port > > + * until the link is trained. > > + */ > > Shouldn't the above go to a kerneldoc comment? Yup I'll move it there. > > + if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || > > + pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) && > > + pdev->subordinate && !list_empty(&pdev->subordinate->devices) && > > + pdev->bridge_d3 && !pci_dev_is_disconnected(pdev)) { > > There's nothing except for this if () in the function, so maybe just > return here if the condition is not satisfied? OK > Also maybe split it into a port type check (you can return right away > if it fails), disconnected check and the rest? OK > > + int delay; > > + > > + delay = get_downstream_delay(pdev->subordinate); > > + if (!delay) > > + return; > > + > > + dev_dbg(&pdev->dev, "waiting downstream link for %d ms\n", delay); > > + > > + /* > > + * If downstream port does not support speeds greater than > > + * 5 GT/s need to wait 100ms. For higher speeds (gen3) we > > + * need to wait first for the data link layer to become > > + * active. > > + */ > > + if (pcie_get_speed_cap(pdev) <= PCIE_SPEED_5_0GT) > > + msleep(delay); > > + else > > + pcie_wait_for_link_delay(pdev, true, delay); > > + } > > +} > > + > > /** > > * pcie_port_device_suspend - suspend port services associated with a PCIe port > > * @dev: PCI Express port to handle > > @@ -391,6 +444,13 @@ int pcie_port_device_suspend(struct device *dev) > > int pcie_port_device_resume_noirq(struct device *dev) > > { > > size_t off = offsetof(struct pcie_port_service_driver, resume_noirq); > > + > > + /* > > + * Wait for the link to be fully up before resuming port services. > > + * This prevents pciehp from starting to tear-down the hierarchy > > + * too soon. > > + */ > > + wait_for_downstream_link(to_pci_dev(dev)); > > return device_for_each_child(dev, &off, pm_iter); > > } > > > > @@ -421,6 +481,8 @@ int pcie_port_device_runtime_suspend(struct device *dev) > > int pcie_port_device_runtime_resume(struct device *dev) > > { > > size_t off = offsetof(struct pcie_port_service_driver, runtime_resume); > > + > > The comment from pcie_port_device_resume_noirq() is applicable here > too, so maybe move it to a common place (kerneldoc?). Sure. > > > + wait_for_downstream_link(to_pci_dev(dev)); > > return device_for_each_child(dev, &off, pm_iter); > > } > > #endif /* PM */ > > -- > > 2.20.1 > >