| Message ID | 1429774916-24920-1-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
[+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, 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@huawei.com> > --- > 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; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@huawei.com> >> --- >> 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; > > . >
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;
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 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@huawei.com> --- drivers/pci/pcie/aspm.c | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-)