[PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Sinan Kaya]

All PCIe devices are expected to be able to handle 8-bit tags.
'commit 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported")'
enabled extended tags for all devices based on the spec direction.

The Broadcom HT2100 seems to be having issues with handling
8-bit tags. Mark it as broken.

Reported-by: Wim ten Have <wim.ten.have@oracle.com>
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/quirks.c | 12 ++++++++++++
 include/linux/pci.h  |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 085fb78..073d5dd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4664,3 +4664,15 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
+
+static void quirk_exttags_completer(struct pci_dev *pdev)
+{
+	/* This device cannot handle 256 tags as a completer */
+	pdev->broken_exttags_completer = 1;
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0140,
+			quirk_exttags_completer);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0142,
+			quirk_exttags_completer);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0144,
+			quirk_exttags_completer);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f..0b9f42d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,6 +376,7 @@ struct pci_dev {
 	unsigned int	irq_managed:1;
 	unsigned int	has_secondary_link:1;
 	unsigned int	non_compliant_bars:1;	/* broken BARs; ignore them */
+	unsigned int	broken_exttags_completer:1;
 	pci_dev_flags_t dev_flags;
 	atomic_t	enable_cnt;	/* pci_enable_device has been called */
 
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V3 2/2] PCI: Do not enable extended tags if a quirky device is found

[Sinan Kaya]

According to extended tags ECN document, all PCIe receivers are expected
to support extended tags. However, devices with exceptions/quirks were
found. If a device with extended tags quirk is found, disable extended tags
for all devices in the tree assuming peer-to-peer is possible.

Also 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.

Reported-by: Wim ten Have <wim.ten.have@oracle.com>
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 | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..396dd80 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1684,21 +1684,43 @@ 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_find_broken_exttags(struct pci_dev *dev, void *data)
 {
+	bool *found = data;
+
+	if (!pci_is_pcie(dev))
+		return 0;
+
+	if (dev->broken_exttags_completer)
+		*found = dev->broken_exttags_completer;
+
+	return 0;
+}
+
+static int pcie_bus_configure_exttags(struct pci_dev *dev, void *broken)
+{
+	bool supported = !*(bool *)broken;
 	u32 dev_cap;
 	int ret;
 
 	if (!pci_is_pcie(dev))
-		return;
+		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 +1729,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 +2222,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 broken_exttags = false;
 
 	if (!bus->self)
 		return;
@@ -2224,6 +2246,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_find_broken_exttags, &broken_exttags);
+	pci_walk_bus(bus, pcie_bus_configure_exttags, &broken_exttags);
 }
 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

Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Bjorn Helgaas]

On Tue, Jul 11, 2017 at 11:25:33AM -0400, Sinan Kaya wrote:
> All PCIe devices are expected to be able to handle 8-bit tags.
> 'commit 60db3a4d8cc9 ("PCI: Enable PCIe Extended Tags if supported")'
> enabled extended tags for all devices based on the spec direction.
> 
> The Broadcom HT2100 seems to be having issues with handling
> 8-bit tags. Mark it as broken.
> 
> Reported-by: Wim ten Have <wim.ten.have@oracle.com>
> 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/quirks.c | 12 ++++++++++++
>  include/linux/pci.h  |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 085fb78..073d5dd 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4664,3 +4664,15 @@ static void quirk_intel_no_flr(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1502, quirk_intel_no_flr);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1503, quirk_intel_no_flr);
> +
> +static void quirk_exttags_completer(struct pci_dev *pdev)
> +{
> +	/* This device cannot handle 256 tags as a completer */
> +	pdev->broken_exttags_completer = 1;

I think we should print something here as a clue to the user.

Wim, how did you find this problem originally?  Seems like it could
have been a hassle to track down.  I wonder if we should log a message
in pci_configure_extended_tags() when we change the setting (either to
enable or disable).  Maybe something in dmesg would have made it
easier to find the problem.

> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0140,
> +			quirk_exttags_completer);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0142,
> +			quirk_exttags_completer);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, 0x0144,
> +			quirk_exttags_completer);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8039f9f..0b9f42d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,6 +376,7 @@ struct pci_dev {
>  	unsigned int	irq_managed:1;
>  	unsigned int	has_secondary_link:1;
>  	unsigned int	non_compliant_bars:1;	/* broken BARs; ignore them */
> +	unsigned int	broken_exttags_completer:1;

I think maybe we should put this bit in struct pci_host_bridge.  Then
the quirk could be something like this:

  static void quirk_ext_tags_broken(struct pci_dev *pdev)
  {
    struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);

    bridge->broken_ext_tags = 1;
    dev_info(&pdev->dev, "Extended Tag handling is broken\n");

    pci_walk_bus(bridge->bus, pci_configure_extended_tags, NULL);
  }

and we could keep the current strategy of calling
pci_configure_extended_tags() from pci_configure_device(), slightly
modified like this:

  void pci_configure_extended_tags(struct pci_dev *dev, void *ign)
  {
    u32 cap;
    u16 ctl;
    int ret;
    struct pci_host_bridge *host;

    if (!pci_is_pcie(dev))
      return;

    ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
    if (ret)
      return;

    if (!(cap & PCI_EXP_DEVCAP_EXT_TAG))
      return;

    ret = pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
    if (ret)
      return;

    host = pci_find_host_bridge(dev->bus);
    if (host->broken_ext_tags && (ctl & PCI_EXP_DEVCTL_EXT_TAG)) {
      dev_info(&dev->dev, "disabling Extended Tags\n");
      pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
                                 PCI_EXP_DEVCTL_EXT_TAG);
      return;
    }

    if (!(ctl & PCI_EXP_DEVCTL_EXT_TAG)) {
      dev_info(&dev->dev, "enabling Extended Tags\n");
      pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
                               PCI_EXP_DEVCTL_EXT_TAG);
    }
  }

I'm trying to avoid pci_walk_bus() because it's such a minefield as
far as hot-added devices.  I don't think we can avoid one in the
quirk, to turn off extended tags for any devices we've already found,
but I think we *can* avoid adding more in
pcie_bus_configure_settings().

>  	pci_dev_flags_t dev_flags;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Sinan Kaya]

On 7/11/2017 3:58 PM, Bjorn Helgaas wrote:
> I'm trying to avoid pci_walk_bus() because it's such a minefield as
> far as hot-added devices.  I don't think we can avoid one in the
> quirk, to turn off extended tags for any devices we've already found,
> but I think we *can* avoid adding more i

Jike Song asked what if you attach a PCIe endpoint that has an extended
tags quirk to think about the reverse direction. That's why, I was walking
the entire bus. 

We can maybe hold onto putting such a generic code until such a device
with quirk actually shows up.

I'll pull your suggestion into a patch, give it a try and then post the
next version.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Bjorn Helgaas]

On Tue, Jul 11, 2017 at 04:16:23PM -0400, Sinan Kaya wrote:
> On 7/11/2017 3:58 PM, Bjorn Helgaas wrote:
> > I'm trying to avoid pci_walk_bus() because it's such a minefield as
> > far as hot-added devices.  I don't think we can avoid one in the
> > quirk, to turn off extended tags for any devices we've already found,
> > but I think we *can* avoid adding more i
> 
> Jike Song asked what if you attach a PCIe endpoint that has an extended
> tags quirk to think about the reverse direction. That's why, I was walking
> the entire bus. 

My proposal handles endpoints, too.  The pci_walk_bus() in the quirk
handles all devices we've already enumerated, and all devices we'll
enumerate in the future are handled in pci_configure_device().

> We can maybe hold onto putting such a generic code until such a device
> with quirk actually shows up.
> 
> I'll pull your suggestion into a patch, give it a try and then post the
> next version.
> 
> -- 
> Sinan Kaya
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Sinan Kaya]

Hi Bjorn,

On 7/11/2017 4:39 PM, Bjorn Helgaas wrote:
> My proposal handles endpoints, too.  The pci_walk_bus() in the quirk
> handles all devices we've already enumerated, and all devices we'll
> enumerate in the future are handled in pci_configure_device().

Code clears the endpoint's extended tag capability only if a quirky host
bridge is found. 

The question here was 

"what if you have an endpoint, it may declare extended tags capability
and has a bug even though the host bridge is just fine"

Code will enable extended tags on both the host bridge and endpoint
if it is supported.

The host bridge will start generating 256 tags towards the endpoint
but endpoint is unable to catch up with it. 

Same thing is possible with two endpoints that try to do peer-to-peer
communication. The first endpoint may generate 256 requests, second
endpoint may not handle it.

Again, this is a hypothetical condition with no known endpoints. I
suggest we deal with this when time comes.

Sinan 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Bjorn Helgaas]

On Wed, Jul 12, 2017 at 09:07:14AM -0400, Sinan Kaya wrote:
> Hi Bjorn,
> 
> On 7/11/2017 4:39 PM, Bjorn Helgaas wrote:
> > My proposal handles endpoints, too.  The pci_walk_bus() in the quirk
> > handles all devices we've already enumerated, and all devices we'll
> > enumerate in the future are handled in pci_configure_device().
> 
> Code clears the endpoint's extended tag capability only if a quirky host
> bridge is found. 
> 
> The question here was 
> 
> "what if you have an endpoint, it may declare extended tags capability
> and has a bug even though the host bridge is just fine"
> 
> Code will enable extended tags on both the host bridge and endpoint
> if it is supported.
> 
> The host bridge will start generating 256 tags towards the endpoint
> but endpoint is unable to catch up with it. 
> 
> Same thing is possible with two endpoints that try to do peer-to-peer
> communication. The first endpoint may generate 256 requests, second
> endpoint may not handle it.
> 
> Again, this is a hypothetical condition with no known endpoints. I
> suggest we deal with this when time comes.

Jike's question (at least, the one I saw via email) was this:

Jike> Maybe checking the version of this endpoint at first? Do you expect a
Jike> v1 endpoint
Jike> to be working under v2+ ports?

This has nothing to do with whether a device is v1 or v2.  All PCIe devices
are expected to handle 8-bit tags as completers.  If we find defective
endpoints, we'll have to add quirks for them just like you did for the
HT2100 root port.  There's nothing we can do until we find them.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port

[Sinan Kaya]

On 7/12/2017 3:45 PM, Bjorn Helgaas wrote:
> There's nothing we can do until we find them.

sounds good.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.