| Message ID | 1495763586-27238-3-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 2017-05-26 at 11:53 +1000, Gavin Shan wrote: > The PCIe transaction timeout is disabled if the capability is claimed > in register PCIECAP_EXP_DCAP2, meaning we read it unconditionally. > However, the reigster can be invalid if PCIe capability version isn't > bigger than 1. It potentially causes pending (hanged) PCIe transaction. > > This fixes the issue by check the cached PCIe capability version. We > won't touch register PCIECAP_EXP_DCAP2 and PCICAP_EXP_DCTL2 if PCIe > capability version isn't bigger than 1. The limitation isn't applied > to P7/P8/P9's root port as we clearly know those registers are valid > there. > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > core/pci.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/core/pci.c b/core/pci.c > index 29c3df6..f173cc2 100644 > --- a/core/pci.c > +++ b/core/pci.c > @@ -162,8 +162,6 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) > return; > } > > - pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false); > - > /* > * XXX We observe a problem on some PLX switches where one > * of the downstream ports appears as an upstream port, we > @@ -174,9 +172,15 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) > if (pd->parent && pd->parent->dev_type == PCIE_TYPE_SWITCH_UPPORT && > pd->vdid == 0x874810b5 && pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) { > PCIDBG(phb, pd->bdfn, "Fixing up bad PLX downstream port !\n"); > + > + reg &= ~PCICAP_EXP_CAP_TYPE; > + reg |= PCIE_TYPE_SWITCH_DNPORT; > pd->dev_type = PCIE_TYPE_SWITCH_DNPORT; > } > > + /* Set PCIe capability */ > + pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, reg, false); Why would you or "PCIE_TYPE_SWITCH_DNPORT" ? The cap cache should just contain the offsets. The versions can be read explicitely. If you want to cache the versions do a separate array. The above is very confusing. That cap "Data" need to die. A generic untyped place holder is a bad thing. > /* XXX Handle ARI */ > if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT || > pd->dev_type == PCIE_TYPE_ROOT_PORT) > @@ -833,11 +837,13 @@ static int pci_configure_mps(struct phb *phb, > > static void pci_disable_completion_timeout(struct phb *phb, struct pci_device *pd) > { > - uint32_t ecap; > - uint32_t val; > + uint32_t ecap, val; > + uint16_t ecap_data; > > /* PCIE capability required */ > - if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) > + ecap_data = (uint16_t)pci_cap_data(pd, PCI_CFG_CAP_ID_EXP, false); > + if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) || > + (ecap_data & PCICAP_EXP_CAP_VERSION) <= 1) > return; Just read the capability version off the HW here. > > /* Check if it has capability to disable completion timeout */
On Thu, Jun 15, 2017 at 03:46:41PM +1000, Benjamin Herrenschmidt wrote: >On Fri, 2017-05-26 at 11:53 +1000, Gavin Shan wrote: >> The PCIe transaction timeout is disabled if the capability is claimed >> in register PCIECAP_EXP_DCAP2, meaning we read it unconditionally. >> However, the reigster can be invalid if PCIe capability version isn't >> bigger than 1. It potentially causes pending (hanged) PCIe transaction. >> >> This fixes the issue by check the cached PCIe capability version. We >> won't touch register PCIECAP_EXP_DCAP2 and PCICAP_EXP_DCTL2 if PCIe >> capability version isn't bigger than 1. The limitation isn't applied >> to P7/P8/P9's root port as we clearly know those registers are valid >> there. > >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> core/pci.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/core/pci.c b/core/pci.c >> index 29c3df6..f173cc2 100644 >> --- a/core/pci.c >> +++ b/core/pci.c >> @@ -162,8 +162,6 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) >> return; >> } >> >> - pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false); >> - >> /* >> * XXX We observe a problem on some PLX switches where one >> * of the downstream ports appears as an upstream port, we >> @@ -174,9 +172,15 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) >> if (pd->parent && pd->parent->dev_type == PCIE_TYPE_SWITCH_UPPORT && >> pd->vdid == 0x874810b5 && pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) { >> PCIDBG(phb, pd->bdfn, "Fixing up bad PLX downstream port !\n"); >> + >> + reg &= ~PCICAP_EXP_CAP_TYPE; >> + reg |= PCIE_TYPE_SWITCH_DNPORT; >> pd->dev_type = PCIE_TYPE_SWITCH_DNPORT; >> } >> >> + /* Set PCIe capability */ >> + pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, reg, false); > >Why would you or "PCIE_TYPE_SWITCH_DNPORT" ? > >The cap cache should just contain the offsets. The versions can be >read explicitely. If you want to cache the versions do a separate >array. > >The above is very confusing. That cap "Data" need to die. A generic >untyped place holder is a bad thing. > Ok, It's going to die in v2. >> /* XXX Handle ARI */ >> if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT || >> pd->dev_type == PCIE_TYPE_ROOT_PORT) >> @@ -833,11 +837,13 @@ static int pci_configure_mps(struct phb *phb, >> >> static void pci_disable_completion_timeout(struct phb *phb, struct pci_device *pd) >> { >> - uint32_t ecap; >> - uint32_t val; >> + uint32_t ecap, val; >> + uint16_t ecap_data; >> >> /* PCIE capability required */ >> - if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) >> + ecap_data = (uint16_t)pci_cap_data(pd, PCI_CFG_CAP_ID_EXP, false); >> + if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) || >> + (ecap_data & PCICAP_EXP_CAP_VERSION) <= 1) >> return; > >Just read the capability version off the HW here. Yes, Will do in v2. >> >> /* Check if it has capability to disable completion timeout */ Cheers, Gavin
diff --git a/core/pci.c b/core/pci.c index 29c3df6..f173cc2 100644 --- a/core/pci.c +++ b/core/pci.c @@ -162,8 +162,6 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) return; } - pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false); - /* * XXX We observe a problem on some PLX switches where one * of the downstream ports appears as an upstream port, we @@ -174,9 +172,15 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd) if (pd->parent && pd->parent->dev_type == PCIE_TYPE_SWITCH_UPPORT && pd->vdid == 0x874810b5 && pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) { PCIDBG(phb, pd->bdfn, "Fixing up bad PLX downstream port !\n"); + + reg &= ~PCICAP_EXP_CAP_TYPE; + reg |= PCIE_TYPE_SWITCH_DNPORT; pd->dev_type = PCIE_TYPE_SWITCH_DNPORT; } + /* Set PCIe capability */ + pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, reg, false); + /* XXX Handle ARI */ if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT || pd->dev_type == PCIE_TYPE_ROOT_PORT) @@ -833,11 +837,13 @@ static int pci_configure_mps(struct phb *phb, static void pci_disable_completion_timeout(struct phb *phb, struct pci_device *pd) { - uint32_t ecap; - uint32_t val; + uint32_t ecap, val; + uint16_t ecap_data; /* PCIE capability required */ - if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) + ecap_data = (uint16_t)pci_cap_data(pd, PCI_CFG_CAP_ID_EXP, false); + if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) || + (ecap_data & PCICAP_EXP_CAP_VERSION) <= 1) return; /* Check if it has capability to disable completion timeout */
The PCIe transaction timeout is disabled if the capability is claimed in register PCIECAP_EXP_DCAP2, meaning we read it unconditionally. However, the reigster can be invalid if PCIe capability version isn't bigger than 1. It potentially causes pending (hanged) PCIe transaction. This fixes the issue by check the cached PCIe capability version. We won't touch register PCIECAP_EXP_DCAP2 and PCICAP_EXP_DCTL2 if PCIe capability version isn't bigger than 1. The limitation isn't applied to P7/P8/P9's root port as we clearly know those registers are valid there. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- core/pci.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)