On Fri, Apr 24, 2015 at 02:59:51PM +0800, Yijing Wang wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=94361 reported > in ATCA platform, system had unusual pcie topology: > > (root port) (downstream port) (upstream port) > +-1c.0-[02-0a]----00.0-[03-0a]--+-00.0-[04]-- > | +-01.0-[05]-- (downstream port) > | +-02.0-[06]-- > | +-03.0-[07]-- > | +-08.0-[08]-- > | +-09.0-[09]-- > | \-0a.0-[0a]-- > We assumed root port and downstream port always > have external link, and downstream port always has a > upstream port. So in this case, when we allocated > pcie_link_state for downstream port 02:00.0, it try > to get parent bus pcie_link_state, > parent = pdev->bus->parent->self->link_state; > because root bus self is NULL, system will crash here. > > This patch fix this issue based on the following > assumption suggested by Bjorn. > 1. Root port is always on the upstream end of a link. > 2. The pcie hierarchy should alternate between links > and internal switch logic, there should be no adjacent > links or internal buses in pcie tree. > > Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx> > --- > drivers/pci/pcie/aspm.c | 23 +++++++++++++++++++++-- > 1 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 7d4fcdc..61b9d3f 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -546,6 +546,25 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > return link; > } > > +/* > + * We assume root port is always on the upstream end of > + * a link, and the pcie hierarchy should alternate > + * between links and internal switch logic. > + */ > +static bool pcie_has_external_link(struct pci_dev *pdev) > +{ > + if (!pci_is_pcie(pdev)) > + return false; > + > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) > + return true; > + > + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) > + if (!pdev->bus->self->link_state) > + return true; > + > + return false; I like the idea of this, but I'd rather not bury it inside ASPM and I'd rather not have it depend on ASPM data (the link_state pointer). I think it would be better if we had a real PCI interface, maybe using a bit in struct pci_dev that we could set in set_pcie_port_type(). We should also look for other cases that might not work on this topology. The following functions test for PCI_EXP_TYPE_DOWNSTREAM and I think most of them assume that the Downstream Port has a link on its secondary side, which is not the case in this topology: reset_link() alloc_pcie_link_state() pcie_aspm_pm_state_change() pcie_aspm_powersave_config_link() __pci_disable_link_state() pcie_aspm_create_sysfs_dev_files() pcie_aspm_remove_sysfs_dev_files() only_one_child() quirk_apple_wait_for_thunderbolt() pci_vc_enable() "has_external_link" doesn't seem very descriptive to me -- I'm not sure how to interpret "external." "has_downstream_link" suggests a link on the downstream (secondary) side, which is more what we're getting at, although it might be confusing that an Upstream Port can have a downstream link. Or maybe "has_secondary_link" or "secondary_is_link". Bjorn > +} > /* > * pcie_aspm_init_link_state: Initiate PCI express link state. > * It is called after the pcie and its children devices are scanned. > @@ -561,8 +580,8 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev) > > if (!pci_is_pcie(pdev) || pdev->link_state) > return; > - if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ROOT_PORT && > - pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM) > + > + if (!pcie_has_external_link(pdev)) > return; > > /* VIA has a strange chipset, root port is under a bridge */ > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
