| Message ID | 1499439193-16628-1-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Wim, On 7/7/2017 10:53 AM, Sinan Kaya wrote: > According to extended tags ECN document, all PCIe receivers are expected > to support extended tags support. It should be safe to enable extended > tags on endpoints without checking compatibility. > > This assumption seems to be working fine except for the legacy systems. > The ECN has been written against PCIE spec version 2.0. Therefore, we need > to exclude all version 1.0 devices from this change as there is HW out > there that can't handle extended tags. > > Note that the default value of Extended Tags Enable bit is implementation > specific. Therefore, we are clearing the bit by default when incompatible > HW is found without assuming that value is zero. > > Reported-by: Wim ten Have <wim.ten.have@oracle.com> > Link: https://pcisig.com/sites/default/files/specification_documents/ECN_Extended_Tag_Enable_Default_05Sept2008_final.pdf > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1467674 > Fixes: 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported") > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- Can you also give this a spin? I don't have a system with v1 PCIe bridges. I only tested v2 and later code path. I tried to address Jike Song concerns on this version and removed your tested-by since the code changed. Sinan
On 7/7/2017 10:53 AM, Sinan Kaya wrote: > + ret = pcie_capability_read_word(dev, PCI_EXP_FLAGS, &flags); > + if (ret || ((flags & PCI_EXP_FLAGS_VERS) < 2)) > + return 0; > Never mind, there is a problem here. I shouldn't have added it here. I'll remove these and post again.
On 7/7/2017 11:07 AM, Sinan Kaya wrote: > On 7/7/2017 10:53 AM, Sinan Kaya wrote: >> + ret = pcie_capability_read_word(dev, PCI_EXP_FLAGS, &flags); >> + if (ret || ((flags & PCI_EXP_FLAGS_VERS) < 2)) >> + return 0; >> > > Never mind, there is a problem here. I shouldn't have added it here. > I'll remove these and post again. > I guess I'll wait until Bjorn gets a chance to review it. There is a decision that needs to be made here. Under normal circumstances, extended tags capability is a reserved field on v1 that's expected to be 0. Code is checking for extended tags capability being non-zero next before setting/clearing the bit. It should be safe to rely on capability being 0 on v1. However, we can go paranoid and add the check above to not even look at the capability like I did it. That's why, I thought this is redundant. I'll wait until Bjorn chimes in. It is OK to keep the code as it is. It is just doing too much validation in my opinion. Somebody can always say play safe.
On Fri, 7 Jul 2017 11:01:16 -0400 Sinan Kaya <okaya@codeaurora.org> wrote: > Hi Wim, > > On 7/7/2017 10:53 AM, Sinan Kaya wrote: > > According to extended tags ECN document, all PCIe receivers are expected > > to support extended tags support. It should be safe to enable extended > > tags on endpoints without checking compatibility. > > > > This assumption seems to be working fine except for the legacy systems. > > The ECN has been written against PCIE spec version 2.0. Therefore, we need > > to exclude all version 1.0 devices from this change as there is HW out > > there that can't handle extended tags. > > > > Note that the default value of Extended Tags Enable bit is implementation > > specific. Therefore, we are clearing the bit by default when incompatible > > HW is found without assuming that value is zero. > > > > Reported-by: Wim ten Have <wim.ten.have@oracle.com> > > Link: https://pcisig.com/sites/default/files/specification_documents/ECN_Extended_Tag_Enable_Default_05Sept2008_final.pdf > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1467674 > > Fixes: 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported") > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > --- > > Can you also give this a spin? I don't have a system with v1 PCIe bridges. > I only tested v2 and later code path. > > I tried to address Jike Song concerns on this version and removed your tested-by > since the code changed. > > Sinan > Sure,
On Fri, Jul 07, 2017 at 10:53:13AM -0400, Sinan Kaya wrote: > According to extended tags ECN document, all PCIe receivers are expected > to support extended tags support. It should be safe to enable extended > tags on endpoints without checking compatibility. > > This assumption seems to be working fine except for the legacy systems. > The ECN has been written against PCIE spec version 2.0. Therefore, we need > to exclude all version 1.0 devices from this change as there is HW out > there that can't handle extended tags. > > Note that the default value of Extended Tags Enable bit is implementation > specific. Therefore, we are clearing the bit by default when incompatible > HW is found without assuming that value is zero. The PCI_EXP_DEVCTL_EXT_TAG bit controls the behavior of the function as a Requester. As far as I can see, it has nothing to do with whether the function supports 8-bit tags as a *Completer*, so the implicit assumption of the spec is that all Completers always support 8-bit tags. My guess is that's why the ECN thought it would be safe to enable extended tags by default. If that's the case, this is just a defect in the device (the Completer), and we should blacklist it. Looking at the PCIe Capability version might happen to correlate with Completer support for 8-bit tags, but that looks like just a coincidence to me. > Reported-by: Wim ten Have <wim.ten.have@oracle.com> > Link: https://pcisig.com/sites/default/files/specification_documents/ECN_Extended_Tag_Enable_Default_05Sept2008_final.pdf > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1467674 > Fixes: 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported") > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/probe.c | 45 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 19c8950..e3b3c18 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1684,21 +1684,51 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp) > */ > } > > -static void pci_configure_extended_tags(struct pci_dev *dev) > +static int pcie_bus_discover_legacy(struct pci_dev *dev, void *data) > { > + bool *found = data; > + int rc; > + u16 flags; > + > + if (!pci_is_pcie(dev)) > + return 0; > + > + rc = pcie_capability_read_word(dev, PCI_EXP_FLAGS, &flags); > + if (!rc && ((flags & PCI_EXP_FLAGS_VERS) < 2)) > + *found = true; > + > + return 0; > +} > + > +static int pcie_bus_configure_exttags(struct pci_dev *dev, void *legacy) > +{ > + bool supported = !*(bool *)legacy; > u32 dev_cap; > + u16 flags; > int ret; > > if (!pci_is_pcie(dev)) > - return; > + return 0; > + > + ret = pcie_capability_read_word(dev, PCI_EXP_FLAGS, &flags); > + if (ret || ((flags & PCI_EXP_FLAGS_VERS) < 2)) > + return 0; > > ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &dev_cap); > - if (ret) > - return; > + if (ret || (!(dev_cap & PCI_EXP_DEVCAP_EXT_TAG))) > + return 0; > > - if (dev_cap & PCI_EXP_DEVCAP_EXT_TAG) > + if (supported) { > + dev_dbg(&dev->dev, "setting extended tags capability\n"); > pcie_capability_set_word(dev, PCI_EXP_DEVCTL, > PCI_EXP_DEVCTL_EXT_TAG); > + } else { > + dev_dbg(&dev->dev, "clearing extended tags capability\n"); > + pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, > + PCI_EXP_DEVCTL_EXT_TAG); > + } > + > + return 0; > } > > static void pci_configure_device(struct pci_dev *dev) > @@ -1707,7 +1737,6 @@ static void pci_configure_device(struct pci_dev *dev) > int ret; > > pci_configure_mps(dev); > - pci_configure_extended_tags(dev); > > memset(&hpp, 0, sizeof(hpp)); > ret = pci_get_hp_params(dev, &hpp); > @@ -2201,6 +2230,7 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data) > void pcie_bus_configure_settings(struct pci_bus *bus) > { > u8 smpss = 0; > + bool legacy_found = false; > > if (!bus->self) > return; > @@ -2224,6 +2254,9 @@ void pcie_bus_configure_settings(struct pci_bus *bus) > > pcie_bus_configure_set(bus->self, &smpss); > pci_walk_bus(bus, pcie_bus_configure_set, &smpss); > + > + pci_walk_bus(bus, pcie_bus_discover_legacy, &legacy_found); > + pci_walk_bus(bus, pcie_bus_configure_exttags, &legacy_found); > } > EXPORT_SYMBOL_GPL(pcie_bus_configure_settings); > > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Bjorn, On 7/10/2017 7:09 PM, Bjorn Helgaas wrote: > The PCI_EXP_DEVCTL_EXT_TAG bit controls the behavior of the function as a > Requester. As far as I can see, it has nothing to do with whether the > function supports 8-bit tags as a *Completer*, so the implicit assumption > of the spec is that all Completers always support 8-bit tags. My guess is > that's why the ECN thought it would be safe to enable extended tags by > default. > > If that's the case, this is just a defect in the device (the Completer), > and we should blacklist it. Looking at the PCIe Capability version might > happen to correlate with Completer support for 8-bit tags, but that looks > like just a coincidence to me. Sure, I can change to blacklist. I'll not enable it unless the device is blacklisted via quirks. Sinan
One more question: On 7/10/2017 8:20 PM, Sinan Kaya wrote: > Sure, I can change to blacklist. I'll not enable it unless the device is blacklisted > via quirks. Since the quirk is at the completer, are you OK with walking the list and clearing the extended tags across the tree when at least one device with a quirk is found? Would you look for another solution? I was told that Linux assumes peer-to-peer is possible by default on another thread.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 19c8950..e3b3c18 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1684,21 +1684,51 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp) */ } -static void pci_configure_extended_tags(struct pci_dev *dev) +static int pcie_bus_discover_legacy(struct pci_dev *dev, void *data) { + bool *found = data; + int rc; + u16 flags; + + if (!pci_is_pcie(dev)) + return 0; + + rc = pcie_capability_read_word(dev, PCI_EXP_FLAGS, &flags); + if (!rc && ((flags & PCI_EXP_FLAGS_VERS) < 2)) + *found = true; + + return 0; +} + +static int pcie_bus_configure_exttags(struct pci_dev *dev, void *legacy) +{ + bool supported = !*(bool *)legacy; u32 dev_cap; + u16 flags; int ret; if (!pci_is_pcie(dev)) - return; + return 0; + + ret = pcie_capability_read_word(dev, PCI_EXP_FLAGS, &flags); + if (ret || ((flags & PCI_EXP_FLAGS_VERS) < 2)) + return 0; ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &dev_cap); - if (ret) - return; + if (ret || (!(dev_cap & PCI_EXP_DEVCAP_EXT_TAG))) + return 0; - if (dev_cap & PCI_EXP_DEVCAP_EXT_TAG) + if (supported) { + dev_dbg(&dev->dev, "setting extended tags capability\n"); pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_EXT_TAG); + } else { + dev_dbg(&dev->dev, "clearing extended tags capability\n"); + pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, + PCI_EXP_DEVCTL_EXT_TAG); + } + + return 0; } static void pci_configure_device(struct pci_dev *dev) @@ -1707,7 +1737,6 @@ static void pci_configure_device(struct pci_dev *dev) int ret; pci_configure_mps(dev); - pci_configure_extended_tags(dev); memset(&hpp, 0, sizeof(hpp)); ret = pci_get_hp_params(dev, &hpp); @@ -2201,6 +2230,7 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data) void pcie_bus_configure_settings(struct pci_bus *bus) { u8 smpss = 0; + bool legacy_found = false; if (!bus->self) return; @@ -2224,6 +2254,9 @@ void pcie_bus_configure_settings(struct pci_bus *bus) pcie_bus_configure_set(bus->self, &smpss); pci_walk_bus(bus, pcie_bus_configure_set, &smpss); + + pci_walk_bus(bus, pcie_bus_discover_legacy, &legacy_found); + pci_walk_bus(bus, pcie_bus_configure_exttags, &legacy_found); } EXPORT_SYMBOL_GPL(pcie_bus_configure_settings);
According to extended tags ECN document, all PCIe receivers are expected to support extended tags support. It should be safe to enable extended tags on endpoints without checking compatibility. This assumption seems to be working fine except for the legacy systems. The ECN has been written against PCIE spec version 2.0. Therefore, we need to exclude all version 1.0 devices from this change as there is HW out there that can't handle extended tags. Note that the default value of Extended Tags Enable bit is implementation specific. Therefore, we are clearing the bit by default when incompatible HW is found without assuming that value is zero. Reported-by: Wim ten Have <wim.ten.have@oracle.com> Link: https://pcisig.com/sites/default/files/specification_documents/ECN_Extended_Tag_Enable_Default_05Sept2008_final.pdf Link: https://bugzilla.redhat.com/show_bug.cgi?id=1467674 Fixes: 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported") Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/probe.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-)