From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754993AbdGJXJP (ORCPT ); Mon, 10 Jul 2017 19:09:15 -0400 Received: from mail.kernel.org ([198.145.29.99]:49370 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754696AbdGJXJO (ORCPT ); Mon, 10 Jul 2017 19:09:14 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 912A522BD0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Mon, 10 Jul 2017 18:09:11 -0500 From: Bjorn Helgaas To: Sinan Kaya Cc: linux-pci@vger.kernel.org, timur@codeaurora.org, wim.ten.have@oracle.com, linux-arm-msm@vger.kernel.org, Bjorn Helgaas , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH V2] PCI: Do not enable extended tags on pre-dated (v1.x) systems Message-ID: <20170710230911.GH5610@bhelgaas-glaptop.roam.corp.google.com> References: <1499439193-16628-1-git-send-email-okaya@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499439193-16628-1-git-send-email-okaya@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > --- > 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