From c8fc9339409df88693742d323819ab8415cd2e9d Mon Sep 17 00:00:00 2001 From: Yijing Wang Date: Thu, 21 May 2015 15:05:03 +0800 Subject: [PATCH] PCI/ASPM: Use dev->has_secondary_link to find downstream links We allocate pcie_link_state for the component at the upstream end of a Link. Previously we did this by allocating pcie_link_state for Root Ports and Downstream Ports. This works fine for the typical topology: 00:1c.0 Root Port [bridge to bus 02] 02:00.0 Upstream Port [bridge to bus 03] 03:00.0 Downstream Port [bridge to bus 04] 04:00.0 Endpoint or Switch Port However, it is possible to have a Root Port connected to a Downstream Port instead of an Upstream Port, as in Robert White's ATCA system: 00:1c.0 Root Port [bridge to bus 02] 02:00.0 Downstream Port [bridge to bus 03] 03:01.0 Downstream Port [bridge to bus 04] 04:00.0 Endpoint or Switch Port In this topology, we wrongly allocated pcie_link_state for the 02:00.0 Downstream Port, which is actually the *downstream* end of a link. This led to the following NULL pointer dereference when we tried to connect this link into the tree of links starting at the 00:1c.0 Root Port: BUG: unable to handle kernel NULL pointer dereference at 0000000000000088 IP: [] pcie_aspm_init_link_state+0x744/0x850 Hardware name: Kontron B3001/B3001, BIOS 4.6.3 08/07/2012 Call Trace: [] pci_scan_slot+0xd5/0x120 [] pci_scan_child_bus+0x2d/0xd0 ... Instead of relying on the component type to identify the upstream end of a link, use the "dev->has_secondary_link" field. This means it's now possible for an Upstream Port to have a link on its secondary side, so alloc_pcie_link_state() needs to connect links originating from both Upstream and Downstream Ports into the tree. [bhelgaas: changelog, add comment] Link: https://bugzilla.kernel.org/show_bug.cgi?id=94361 Link: http://lkml.kernel.org/r/54EB81B2.4050904@pobox.com Reported-by: Robert White Signed-off-by: Yijing Wang Signed-off-by: Bjorn Helgaas --- drivers/pci/pcie/aspm.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 4e1af7637501..a84f87247d2f 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -525,7 +525,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) INIT_LIST_HEAD(&link->children); INIT_LIST_HEAD(&link->link); link->pdev = pdev; - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) { + if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT) { struct pcie_link_state *parent; parent = pdev->bus->parent->self->link_state; if (!parent) { @@ -559,10 +559,15 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) if (!aspm_support_enabled) return; - if (!pci_is_pcie(pdev) || pdev->link_state) + if (pdev->link_state) return; - if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT && - pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM) + + /* + * We allocate pcie_link_state for the component on the upstream + * end of a Link, so there's nothing to do unless this device has a + * Link on its secondary side. + */ + if (!pdev->has_secondary_link) return; /* VIA has a strange chipset, root port is under a bridge */ @@ -715,8 +720,7 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) if (!pci_is_pcie(pdev)) return; - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT || - pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) + if (pdev->has_secondary_link) parent = pdev; if (!parent || !parent->link_state) return; -- 2.20.1