From ee8bdfb6568d86bb93f55f8d99c4c643e77304ee Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Mon, 2 Oct 2017 15:08:40 +0100 Subject: [PATCH 1/5] PCI/ASPM: Deal with missing root ports in link state handling Even though it is unconventional, some PCIe host implementations omit the root ports entirely, and simply consist of a host bridge (which is not modeled as a device in the PCI hierarchy) and a link. When the downstream device is an endpoint, our current code does not seem to mind this unusual configuration. However, when PCIe switches are involved, the ASPM code assumes that any downstream switch port has a parent, and blindly dereferences the bus->parent->self field of the pci_dev struct to chain the downstream link state to the link state of the root port. Given that the root port is missing, the link is not modeled at all, and nor is the link state, and attempting to access it results in a NULL pointer dereference and a crash. Avoid this by allowing the link state chain to terminate at the downstream port if no root port exists. Signed-off-by: Ard Biesheuvel Signed-off-by: Bjorn Helgaas --- drivers/pci/pcie/aspm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 1dfa10cc566b..0bea8498b5a5 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -802,10 +802,14 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) /* * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe - * hierarchies. + * hierarchies. Note that some PCIe host implementations omit + * the root ports entirely, in which case a downstream port on + * a switch may become the root of the link state chain for all + * its subordinate endpoints. */ if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || - pci_pcie_type(pdev) == PCI_EXP_TYPE_PCIE_BRIDGE) { + pci_pcie_type(pdev) == PCI_EXP_TYPE_PCIE_BRIDGE || + !pdev->bus->parent->self) { link->root = link; } else { struct pcie_link_state *parent; From 94ac327e043ee40d7fc57b54541da50507ef4e99 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Mon, 13 Nov 2017 08:50:30 -0600 Subject: [PATCH 2/5] PCI/ASPM: Account for downstream device's Port Common_Mode_Restore_Time Every Port that supports the L1.2 substate advertises its Port Common_Mode_Restore_Time, i.e., the time the Port requires to re-establish common mode when exiting L1.2 (see PCIe r3.1, sec 7.33.2). Per sec 5.5.3.3.1, when exiting L1.2, the Downstream Port (the device at the upstream end of the link) must send TS1 training sequences for at least T(COMMONMODE) after it detects electrical idle exit on the Link. We want this to be long enough for both ends of the Link, so we should set it to the maximum of the Port Common_Mode_Restore_Time for the upstream and downstream components on the Link. Previously we only looked at the Port Common_Mode_Restore_Time of the upstream device, so if the downstream device required more time, we didn't program the upstream device's T(COMMONMODE) correctly. Fixes: f1f0366dd6be ("PCI/ASPM: Calculate and save the L1.2 timing parameters") Signed-off-by: Bjorn Helgaas Reviewed-by: Vidya Sagar Acked-by: Rajat Jain CC: stable@vger.kernel.org # v4.11+ --- drivers/pci/pcie/aspm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 0bea8498b5a5..46c59afb8355 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -452,7 +452,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, /* Choose the greater of the two T_cmn_mode_rstr_time */ val1 = (upreg->l1ss_cap >> 8) & 0xFF; - val2 = (upreg->l1ss_cap >> 8) & 0xFF; + val2 = (dwreg->l1ss_cap >> 8) & 0xFF; if (val1 > val2) link->l1ss.ctl1 |= val1 << 8; else From c00054f540bf81e592e1fee709b0bdbf20f478b5 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Mon, 13 Nov 2017 15:05:50 -0600 Subject: [PATCH 3/5] PCI/ASPM: Use correct capability pointer to program LTR_L1.2_THRESHOLD Previously we programmed the LTR_L1.2_THRESHOLD in the parent (upstream) device using the capability pointer of the *child* (downstream) device, which corrupted some random word of the parent's config space. Use the parent's L1 SS capability pointer to program its LTR_L1.2_THRESHOLD. Fixes: aeda9adebab8 ("PCI/ASPM: Configure L1 substate settings") Signed-off-by: Bjorn Helgaas Reviewed-by: Vidya Sagar CC: stable@vger.kernel.org # v4.11+ CC: Rajat Jain --- drivers/pci/pcie/aspm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 46c59afb8355..a378dd9d2473 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -657,7 +657,7 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state) 0xFF00, link->l1ss.ctl1); /* Program LTR L1.2 threshold in both ports */ - pci_clear_and_set_dword(parent, dw_cap_ptr + PCI_L1SS_CTL1, + pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1, 0xE3FF0000, link->l1ss.ctl1); pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1, 0xE3FF0000, link->l1ss.ctl1); From 7f88ba4a19b91d310eca836b647edeb100c61c8d Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 10 Nov 2017 15:13:10 -0600 Subject: [PATCH 4/5] PCI/ASPM: Reformat ASPM register definitions Reformat register field definitions in the style used elsewhere and align comments with names used in the spec. No functional change intended. Signed-off-by: Bjorn Helgaas Reviewed-by: Vidya Sagar --- include/uapi/linux/pci_regs.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index f8d58045926f..4150acb4cccb 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -995,19 +995,19 @@ #define PCI_PTM_CTRL_ENABLE 0x00000001 /* PTM enable */ #define PCI_PTM_CTRL_ROOT 0x00000002 /* Root select */ -/* L1 PM Substates */ -#define PCI_L1SS_CAP 4 /* capability register */ -#define PCI_L1SS_CAP_PCIPM_L1_2 1 /* PCI PM L1.2 Support */ -#define PCI_L1SS_CAP_PCIPM_L1_1 2 /* PCI PM L1.1 Support */ -#define PCI_L1SS_CAP_ASPM_L1_2 4 /* ASPM L1.2 Support */ -#define PCI_L1SS_CAP_ASPM_L1_1 8 /* ASPM L1.1 Support */ -#define PCI_L1SS_CAP_L1_PM_SS 16 /* L1 PM Substates Support */ -#define PCI_L1SS_CTL1 8 /* Control Register 1 */ -#define PCI_L1SS_CTL1_PCIPM_L1_2 1 /* PCI PM L1.2 Enable */ -#define PCI_L1SS_CTL1_PCIPM_L1_1 2 /* PCI PM L1.1 Support */ -#define PCI_L1SS_CTL1_ASPM_L1_2 4 /* ASPM L1.2 Support */ -#define PCI_L1SS_CTL1_ASPM_L1_1 8 /* ASPM L1.1 Support */ -#define PCI_L1SS_CTL1_L1SS_MASK 0x0000000F -#define PCI_L1SS_CTL2 0xC /* Control Register 2 */ +/* ASPM L1 PM Substates */ +#define PCI_L1SS_CAP 0x04 /* Capabilities Register */ +#define PCI_L1SS_CAP_PCIPM_L1_2 0x00000001 /* PCI-PM L1.2 Supported */ +#define PCI_L1SS_CAP_PCIPM_L1_1 0x00000002 /* PCI-PM L1.1 Supported */ +#define PCI_L1SS_CAP_ASPM_L1_2 0x00000004 /* ASPM L1.2 Supported */ +#define PCI_L1SS_CAP_ASPM_L1_1 0x00000008 /* ASPM L1.1 Supported */ +#define PCI_L1SS_CAP_L1_PM_SS 0x00000010 /* L1 PM Substates Supported */ +#define PCI_L1SS_CTL1 0x08 /* Control 1 Register */ +#define PCI_L1SS_CTL1_PCIPM_L1_2 0x00000001 /* PCI-PM L1.2 Enable */ +#define PCI_L1SS_CTL1_PCIPM_L1_1 0x00000002 /* PCI-PM L1.1 Enable */ +#define PCI_L1SS_CTL1_ASPM_L1_2 0x00000004 /* ASPM L1.2 Enable */ +#define PCI_L1SS_CTL1_ASPM_L1_1 0x00000008 /* ASPM L1.1 Enable */ +#define PCI_L1SS_CTL1_L1SS_MASK 0x0000000f +#define PCI_L1SS_CTL2 0x0c /* Control 2 Register */ #endif /* LINUX_PCI_REGS_H */ From a48f3d5b197494d903c97ff7bc0909dac65740f8 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Mon, 13 Nov 2017 08:36:40 -0600 Subject: [PATCH 5/5] PCI/ASPM: Add L1 Substates definitions Add and use #defines for L1 Substate register fields instead of hard-coding the masks. Also update comments to use names from the spec. No functional change intended. Signed-off-by: Bjorn Helgaas Reviewed-by: Vidya Sagar --- drivers/pci/pcie/aspm.c | 34 ++++++++++++++++++++-------------- include/uapi/linux/pci_regs.h | 6 ++++++ 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index a378dd9d2473..d240ffab24c1 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -450,24 +450,25 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, if (!(link->aspm_support & ASPM_STATE_L1_2_MASK)) return; - /* Choose the greater of the two T_cmn_mode_rstr_time */ - val1 = (upreg->l1ss_cap >> 8) & 0xFF; - val2 = (dwreg->l1ss_cap >> 8) & 0xFF; + /* Choose the greater of the two Port Common_Mode_Restore_Times */ + val1 = (upreg->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; + val2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; if (val1 > val2) link->l1ss.ctl1 |= val1 << 8; else link->l1ss.ctl1 |= val2 << 8; + /* * We currently use LTR L1.2 threshold to be fixed constant picked from * Intel's coreboot. */ link->l1ss.ctl1 |= LTR_L1_2_THRESHOLD_BITS; - /* Choose the greater of the two T_pwr_on */ - val1 = (upreg->l1ss_cap >> 19) & 0x1F; - scale1 = (upreg->l1ss_cap >> 16) & 0x03; - val2 = (dwreg->l1ss_cap >> 19) & 0x1F; - scale2 = (dwreg->l1ss_cap >> 16) & 0x03; + /* Choose the greater of the two Port T_POWER_ON times */ + val1 = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19; + scale1 = (upreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16; + val2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19; + scale2 = (dwreg->l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16; if (calc_l1ss_pwron(link->pdev, scale1, val1) > calc_l1ss_pwron(link->downstream, scale2, val2)) @@ -646,21 +647,26 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state) if (enable_req & ASPM_STATE_L1_2_MASK) { - /* Program T_pwr_on in both ports */ + /* Program T_POWER_ON times in both ports */ pci_write_config_dword(parent, up_cap_ptr + PCI_L1SS_CTL2, link->l1ss.ctl2); pci_write_config_dword(child, dw_cap_ptr + PCI_L1SS_CTL2, link->l1ss.ctl2); - /* Program T_cmn_mode in parent */ + /* Program Common_Mode_Restore_Time in upstream device */ pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1, - 0xFF00, link->l1ss.ctl1); + PCI_L1SS_CTL1_CM_RESTORE_TIME, + link->l1ss.ctl1); - /* Program LTR L1.2 threshold in both ports */ + /* Program LTR_L1.2_THRESHOLD time in both ports */ pci_clear_and_set_dword(parent, up_cap_ptr + PCI_L1SS_CTL1, - 0xE3FF0000, link->l1ss.ctl1); + PCI_L1SS_CTL1_LTR_L12_TH_VALUE | + PCI_L1SS_CTL1_LTR_L12_TH_SCALE, + link->l1ss.ctl1); pci_clear_and_set_dword(child, dw_cap_ptr + PCI_L1SS_CTL1, - 0xE3FF0000, link->l1ss.ctl1); + PCI_L1SS_CTL1_LTR_L12_TH_VALUE | + PCI_L1SS_CTL1_LTR_L12_TH_SCALE, + link->l1ss.ctl1); } val = 0; diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index 4150acb4cccb..85a4014de42e 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -1002,12 +1002,18 @@ #define PCI_L1SS_CAP_ASPM_L1_2 0x00000004 /* ASPM L1.2 Supported */ #define PCI_L1SS_CAP_ASPM_L1_1 0x00000008 /* ASPM L1.1 Supported */ #define PCI_L1SS_CAP_L1_PM_SS 0x00000010 /* L1 PM Substates Supported */ +#define PCI_L1SS_CAP_CM_RESTORE_TIME 0x0000ff00 /* Port Common_Mode_Restore_Time */ +#define PCI_L1SS_CAP_P_PWR_ON_SCALE 0x00030000 /* Port T_POWER_ON scale */ +#define PCI_L1SS_CAP_P_PWR_ON_VALUE 0x00f80000 /* Port T_POWER_ON value */ #define PCI_L1SS_CTL1 0x08 /* Control 1 Register */ #define PCI_L1SS_CTL1_PCIPM_L1_2 0x00000001 /* PCI-PM L1.2 Enable */ #define PCI_L1SS_CTL1_PCIPM_L1_1 0x00000002 /* PCI-PM L1.1 Enable */ #define PCI_L1SS_CTL1_ASPM_L1_2 0x00000004 /* ASPM L1.2 Enable */ #define PCI_L1SS_CTL1_ASPM_L1_1 0x00000008 /* ASPM L1.1 Enable */ #define PCI_L1SS_CTL1_L1SS_MASK 0x0000000f +#define PCI_L1SS_CTL1_CM_RESTORE_TIME 0x0000ff00 /* Common_Mode_Restore_Time */ +#define PCI_L1SS_CTL1_LTR_L12_TH_VALUE 0x03ff0000 /* LTR_L1.2_THRESHOLD_Value */ +#define PCI_L1SS_CTL1_LTR_L12_TH_SCALE 0xe0000000 /* LTR_L1.2_THRESHOLD_Scale */ #define PCI_L1SS_CTL2 0x0c /* Control 2 Register */ #endif /* LINUX_PCI_REGS_H */