Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1685144 > unrolled thread

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

Started bySinan Kaya <okaya@codeaurora.org>
First post2017-07-11 17:30 +0200
Last post2017-07-12 21:50 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port Sinan Kaya <okaya@codeaurora.org> - 2017-07-11 17:30 +0200
    Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100  Root Port Bjorn Helgaas <helgaas@kernel.org> - 2017-07-11 22:00 +0200
      Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100  Root Port Sinan Kaya <okaya@codeaurora.org> - 2017-07-11 22:20 +0200
        Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100  Root Port Bjorn Helgaas <helgaas@kernel.org> - 2017-07-11 22:50 +0200
          Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100  Root Port Sinan Kaya <okaya@codeaurora.org> - 2017-07-12 15:10 +0200
            Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100  Root Port Bjorn Helgaas <helgaas@kernel.org> - 2017-07-12 21:50 +0200
              Re: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100  Root Port Sinan Kaya <okaya@codeaurora.org> - 2017-07-12 21:50 +0200

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

FromSinan Kaya <okaya@codeaurora.org>
Date2017-07-11 17:30 +0200
Subject[PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u25gS-41E-5@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromBjorn Helgaas <helgaas@kernel.org>
Date2017-07-11 22:00 +0200
SubjectRe: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u29ua-6y9-21@gated-at.bofh.it>
In reply to#1685144
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

[toc] | [prev] | [next] | [standalone]

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

FromSinan Kaya <okaya@codeaurora.org>
Date2017-07-11 22:20 +0200
SubjectRe: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u29Nv-6TI-7@gated-at.bofh.it>
In reply to#1685322
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.

[toc] | [prev] | [next] | [standalone]

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

FromBjorn Helgaas <helgaas@kernel.org>
Date2017-07-11 22:50 +0200
SubjectRe: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u2agx-73x-3@gated-at.bofh.it>
In reply to#1685326
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.

[toc] | [prev] | [next] | [standalone]

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

FromSinan Kaya <okaya@codeaurora.org>
Date2017-07-12 15:10 +0200
SubjectRe: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u2pyW-cE-39@gated-at.bofh.it>
In reply to#1685334
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.

[toc] | [prev] | [next] | [standalone]

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

FromBjorn Helgaas <helgaas@kernel.org>
Date2017-07-12 21:50 +0200
SubjectRe: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u2vO1-41S-1@gated-at.bofh.it>
In reply to#1685766
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

[toc] | [prev] | [next] | [standalone]

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

FromSinan Kaya <okaya@codeaurora.org>
Date2017-07-12 21:50 +0200
SubjectRe: [PATCH V3 1/2] PCI: Add Extended Tags quirk for Broadcom HT2100 Root Port
Message-ID<u2vO2-41S-15@gated-at.bofh.it>
In reply to#1686004
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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web