[PATCH] IB/qib: detour pcie_caps for certain chip sets

[PATCH] IB/qib: detour pcie_caps for certain chip sets

[Mike Marciniszyn]

Commit 8d4548f2b ("IB/qib: Default some module parameters optimally")
introduced an issue with older root complexes.  They cannot handle
the pcie_caps of 0x51.

A typical diagnostic in this situation reported by syslog contains
the text:

  [PCIe Poisoned TLP][Send DMA memory read]

Insure the 0x51 is not used on the suspect chip sets.

Reviewed-by: Mark Debbage <mark.debbage-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/qib/qib_pcie.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_pcie.c b/drivers/infiniband/hw/qib/qib_pcie.c
index 0de55c0..bb23ba0 100644
--- a/drivers/infiniband/hw/qib/qib_pcie.c
+++ b/drivers/infiniband/hw/qib/qib_pcie.c
@@ -577,7 +577,7 @@ static int qib_tune_pcie_coalesce(struct qib_devdata *dd)
  * BIOS may not set PCIe bus-utilization parameters for best performance.
  * Check and optionally adjust them to maximize our throughput.
  */
-static int qib_pcie_caps = 0x51;
+static int qib_pcie_caps;
 module_param_named(pcie_caps, qib_pcie_caps, int, S_IRUGO);
 MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: Payload (0..3), ReadReq (4..7)");
 
@@ -589,6 +589,7 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 	u16 pcaps, pctl, ecaps, ectl;
 	int rc_sup, ep_sup;
 	int rc_cur, ep_cur;
+	int caps = 0x51; /* may be overridden below */
 
 	/* Find out supported and configured values for parent (root) */
 	parent = dd->pcidev->bus->self;
@@ -609,6 +610,14 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 		pci_read_config_word(dd->pcidev, epos + PCI_EXP_DEVCTL, &ectl);
 	} else
 		goto bail;
+	if (!qib_pcie_caps) {
+		u16 devid = parent->device;
+		if ((devid >= 0x25e2 && devid <= 0x25fa) ||
+		    (devid >= 0x65e2 && devid <= 0x65fa) ||
+		    (devid >= 0x4021 && devid <= 0x402e))
+			caps = 0;
+	} else
+		caps = qib_pcie_caps;
 	ret = 0;
 	/* Find max payload supported by root, endpoint */
 	rc_sup = fld2val(pcaps, PCI_EXP_DEVCAP_PAYLOAD);
@@ -620,8 +629,8 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 	ep_cur = fld2val(ectl, PCI_EXP_DEVCTL_PAYLOAD);
 
 	/* If Supported greater than limit in module param, limit it */
-	if (rc_sup > (qib_pcie_caps & 7))
-		rc_sup = qib_pcie_caps & 7;
+	if (rc_sup > (caps & 7))
+		rc_sup = caps & 7;
 	/* If less than (allowed, supported), bump root payload */
 	if (rc_sup > rc_cur) {
 		rc_cur = rc_sup;
@@ -643,8 +652,8 @@ static int qib_tune_pcie_caps(struct qib_devdata *dd)
 	 * which is code '5' (log2(4096) - 7)
 	 */
 	rc_sup = 5;
-	if (rc_sup > ((qib_pcie_caps >> 4) & 7))
-		rc_sup = (qib_pcie_caps >> 4) & 7;
+	if (rc_sup > ((caps >> 4) & 7))
+		rc_sup = (caps >> 4) & 7;
 	rc_cur = fld2val(pctl, PCI_EXP_DEVCTL_READRQ);
 	ep_cur = fld2val(ectl, PCI_EXP_DEVCTL_READRQ);
 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Jason Gunthorpe]

On Wed, Jan 11, 2012 at 10:00:49PM -0500, Mike Marciniszyn wrote:
> Commit 8d4548f2b ("IB/qib: Default some module parameters optimally")
> introduced an issue with older root complexes.  They cannot handle
> the pcie_caps of 0x51.

Should whatever this issue is be a general PCI fixup? Like broken MSI,
etc.

Might be nice to include what 0x51 tunes in the commit to aide other
peoole with the broken chipset :)

Isn't it necesary to check the PCI vendor as well as the devid?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Roland Dreier]

On Wed, Jan 11, 2012 at 7:00 PM, Mike Marciniszyn
<mike.marciniszyn-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org> wrote:
> +       if (!qib_pcie_caps) {
> +               u16 devid = parent->device;
> +               if ((devid >= 0x25e2 && devid <= 0x25fa) ||
> +                   (devid >= 0x65e2 && devid <= 0x65fa) ||
> +                   (devid >= 0x4021 && devid <= 0x402e))
> +                       caps = 0;
> +       } else

Does this work on systems where the broken chipset might
not be the immediate parent of the qib device (ie there are
some PCIe switches in between)?

Pushing knowledge of completely unrelated devices out into
every device driver looks it will be impossible to maintain.
If you need to know about the whole PCIe fabric to do this
type of tuning, the only way I can see to make this work
is to add some helper functions to set these values for
a given device, and then add all the workaround handling
to the core PCI driver.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Roland Dreier]

On Wed, Jan 11, 2012 at 9:48 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> Isn't it necesary to check the PCI vendor as well as the devid?

That definitely seems like a good idea.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Mike Marciniszyn]

> Should whatever this issue is be a general PCI fixup? Like broken MSI,
> etc.
>
Can you point me to some details on this?

> Might be nice to include what 0x51 tunes in the commit to aide other
> peoole with the broken chipset :)
>
> Isn't it necesary to check the PCI vendor as well as the devid?
>
Will do both of these in a V2.

Mike

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Mike Marciniszyn]

> Does this work on systems where the broken chipset might
> not be the immediate parent of the qib device (ie there are
> some PCIe switches in between)?
>

The code figures this out at the top of routine and returns, changing nothing.



This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Jason Gunthorpe]

On Thu, Jan 12, 2012 at 08:02:52AM -0800, Mike Marciniszyn wrote:
> > Should whatever this issue is be a general PCI fixup? Like broken MSI,
> > etc.
> >
> Can you point me to some details on this?

I can explain the broken MSI stuff, as an example. As I noted I'm not
sure what you are working around here, but if there are limits imposed
on otherwise correct values in the PCI capabilities block then I think
it is broadly applicable to handle this in core code...

There are flags in pci.h like:

        PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,

Which are quirk things.. Look in drivers/pci/quirks.c to see how it is
set.

So broadly you'd make a new appropriate bus flag to control
whatever you are working around and then test and set it in quirks,
and provide core code to traverse the bus path from a device to ensure
nothing in the path sets that quirk.

Really depends what the problem actually is.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Roland Dreier]

On Thu, Jan 12, 2012 at 9:17 AM, Mike Marciniszyn
<mike.marciniszyn-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org> wrote:
>> Does this work on systems where the broken chipset might
>> not be the immediate parent of the qib device (ie there are
>> some PCIe switches in between)?

> The code figures this out at the top of routine and returns, changing nothing.

OIC.

Also I see

       if (parent->vendor != 0x8086)
                return 1;

so I guess you don't need another vendor check.

Although this might be better written as PCI_VENDOR_ID_INTEL instead of 0x8086.

I guess this is OK, although as Jason said it would be much better
if the PCI core knew about these chipset errata.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Mike Marciniszyn]

>
>        if (parent->vendor != 0x8086)
>                 return 1;
>
> so I guess you don't need another vendor check.

Actually, Jason is right.  The vendor check you reference here is in qib_tune_pcie_coalesce() and not the routine being patched.

A bit of background here is that the issue was noted with the indicated Harpertown root complex chip sets as follows:
- The BIOS set the root complex MaxPayLoad to 128, but rc capabilities indicate 256 is possible
- To get the best performance we tried going to 256 on the rc and our card and noted the Poisoned TLP
- The patch is an effort to avoid having to use set pcie_caps at all as well as avoiding issues with the problematic chip sets
- The module parameter can still be used to experiment

We have never the issue with AMD or other Intel chipsets.  The problematic device ids are not in fixup.c in lib.

I can reissue a v2 with:
- the vendor check
- define use when available

We probably need to do something, since the current 3.2 rc has the above risk.

Mike


This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Jason Gunthorpe]

On Thu, Jan 12, 2012 at 02:14:12PM -0800, Mike Marciniszyn wrote:

> Actually, Jason is right.  The vendor check you reference here is in
> qib_tune_pcie_coalesce() and not the routine being patched.
> 
> A bit of background here is that the issue was noted with the
> indicated Harpertown root complex chip sets as follows:
> - The BIOS set the root complex MaxPayLoad to 128, but rc capabilities indicate 256 is possible
> - To get the best performance we tried going to 256 on the rc and
> our card and noted the Poisoned TLP

I don't think it is appropriate for a driver to modify the pci
configuration of the root complex.. What if other drivers also try and
modify this configuration? Chaos.

It doesn't seem to me like this has any place in the quirks thing
either. Things seem to be working properly, the MaxPayLoad of 128 is
clearly the highest the system will support correctly.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/qib: detour pcie_caps for certain chip sets

[Mike Marciniszyn]

> It doesn't seem to me like this has any place in the quirks thing
> either. Things seem to be working properly, the MaxPayLoad of 128 is
> clearly the highest the system will support correctly.
>
> Jason

Probably the best thing to do unwind the module parameter default in 8d4548f2b which would change the initial value back to 0.

That's the way the file has always been and that won't change the rc.

Mike

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html