On 2015/4/23 22:24, Bjorn Helgaas wrote: > [+cc Alex] > > Hi Yijing, > > Thanks for picking this up and working on it! > > On Thu, Apr 23, 2015 at 03:41:56PM +0800, Yijing Wang wrote: >> https://bugzilla.kernel.org/show_bug.cgi?id=94361 reported >> NULL Pointer in pcie_aspm_init_link_state(), Some platform >> like ATCA may has strange PCIe topology: >> E.g. >> ---root port---downstream port---upstream port >> |--downstream port >> |--downstream port > > I agree that this is "strange" in the sense that it's not the typical PC > topology. However, I can't find anything in the spec that prohibits it. > As far as I can tell, it is legal, so the PCI core shouldn't treat it as > an exception. > > PCI bridges (including PCIe switch ports) are symmetric: anything in the > bridge windows will be transferred from the primary interface to the > secondary, and anything outside the windows will be transferred from > secondary to primary. That applies to both address-routed transactions > (using the I/O port, memory, and prefetchable memory windows) and > ID-routed transactions (using the bus number windows). And it applies > to both Upstream and Downstream Ports, so from a routing perspective, I > think Upstream and Downstream Ports are mostly interchangeable. > > There are some wrinkles that might be issues: > > - PCIe spec r3.0, sec 7.3.3 says propagation of config requests from > Downstream to Upstream is unsupported. I think that means we > wouldn't be able to discover anything on the other side of the > Upstream Port in the above topology. > > - Sec 7.5.1.7 says primary side Error Control/Status registers apply > to the link for Upstream Ports and to the internal logic for > Downstream Ports. For this ATCA topology that means the primary > side registers of a Downstream Port really refer to the *secondary* > interface. > > - Sec 7.16 (ACS) might have issues similar to this ASPM one. > > If this topology is legal, the NULL pointer is a hint that the code uses > an incorrect assumption, and I'd rather fix the incorrect assumption > than apply a point fix for the NULL pointer. > > In this case, the code incorrectly assumes that a Downstream Port is > always on the upstream end of a link. I think it's safe to assume that > a *Root Port* is always on the upstream end of a link. Maybe we can > start from there and deduce where the links are, without relying on > whether a Port is an Upstream or Downstream Port. The levels of the > hierarchy should alternate between links and internal switch logic, I agree with this assumption, there should be no adjacent links or internal buses. Thanks! Yijing. > e.g.: > > RP -link- endpoint > RP -link- UP -int-bus- DP -link- endpoint > RP -link- UP -int-bus- DP -link- UP -int-bus- DP -link- endpoint > RP -link- DP -int-bus- DP -link- endpoint > RP -link- DP -int-bus- DP -link- UP -int-bus- DP -link- endpoint > >> When we try to alloc pcie_link_state, we assumed downstream port >> always has a upstream port, in this case, NULL pointer would >> be triggered when try to find the parent pci_link_state, >> because downstream port connects to root port directly. > >> Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx> >> --- >> drivers/pci/pcie/aspm.c | 13 +++++++++++-- >> 1 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index 7d4fcdc..f295824 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -526,8 +526,17 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) >> INIT_LIST_HEAD(&link->link); >> link->pdev = pdev; >> if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) { >> - struct pcie_link_state *parent; >> - parent = pdev->bus->parent->self->link_state; >> + struct pci_bus *pbus = pdev->bus; >> + struct pcie_link_state *parent = NULL; >> + >> + while (pbus) { >> + if (pbus->self) { >> + parent = pbus->self->link_state; >> + if (parent) >> + break; >> + } >> + pbus = pbus->parent; >> + } >> if (!parent) { >> kfree(link); >> return NULL; > > . > -- Thanks! Yijing -- 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
