[PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Mikulas Patocka]

The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
the register and compare it against them.

This patch fixes errors "amdgpu: [powerplay] failed to send message 261
ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
the slot is being incorrectly reported as PCIe-v3 capable.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
Cc: stable@vger.kernel.org	# v4.17+

---
 drivers/pci/pci.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-4.19/drivers/pci/pci.c
===================================================================
--- linux-4.19.orig/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
+++ linux-4.19/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
@@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
 
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
 	if (lnkcap) {
-		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
+		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
 			return PCIE_SPEED_16_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
+		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
 			return PCIE_SPEED_8_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
+		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
 			return PCIE_SPEED_5_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
+		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
 			return PCIE_SPEED_2_5GT;
 	}
 

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Alex Deucher]

On Tue, Oct 30, 2018 at 12:36 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> the register and compare it against them.
>
> This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> the slot is being incorrectly reported as PCIe-v3 capable.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> Cc: stable@vger.kernel.org      # v4.17+

Acked-by: Alex Deucher <alexander.deucher@amd.com>

>
> ---
>  drivers/pci/pci.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-4.19/drivers/pci/pci.c
> ===================================================================
> --- linux-4.19.orig/drivers/pci/pci.c   2018-10-30 16:58:58.000000000 +0100
> +++ linux-4.19/drivers/pci/pci.c        2018-10-30 16:58:58.000000000 +0100
> @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
>
>         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
>         if (lnkcap) {
> -               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> +               if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
>                         return PCIE_SPEED_16_0GT;
> -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
>                         return PCIE_SPEED_8_0GT;
> -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
>                         return PCIE_SPEED_5_0GT;
> -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
>                         return PCIE_SPEED_2_5GT;
>         }
>

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Alex Deucher]

On Tue, Oct 30, 2018 at 2:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Oct 30, 2018 at 12:36 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > the register and compare it against them.
> >
> > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > the slot is being incorrectly reported as PCIe-v3 capable.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > Cc: stable@vger.kernel.org      # v4.17+
>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>


Has this patch been picked up yet?  It fixes a ton of bugs.  Please apply!

Alex

>
> >
> > ---
> >  drivers/pci/pci.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > Index: linux-4.19/drivers/pci/pci.c
> > ===================================================================
> > --- linux-4.19.orig/drivers/pci/pci.c   2018-10-30 16:58:58.000000000 +0100
> > +++ linux-4.19/drivers/pci/pci.c        2018-10-30 16:58:58.000000000 +0100
> > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> >
> >         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> >         if (lnkcap) {
> > -               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > +               if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> >                         return PCIE_SPEED_16_0GT;
> > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> >                         return PCIE_SPEED_8_0GT;
> > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> >                         return PCIE_SPEED_5_0GT;
> > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> >                         return PCIE_SPEED_2_5GT;
> >         }
> >

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Alex Deucher]

On Wed, Nov 14, 2018 at 10:44 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Oct 30, 2018 at 2:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Tue, Oct 30, 2018 at 12:36 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> > >
> > > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > > the register and compare it against them.
> > >
> > > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > > the slot is being incorrectly reported as PCIe-v3 capable.
> > >
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > > Cc: stable@vger.kernel.org      # v4.17+
> >
> > Acked-by: Alex Deucher <alexander.deucher@amd.com>
>
>
> Has this patch been picked up yet?  It fixes a ton of bugs.  Please apply!

Any word on this?  Fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=108704
https://bugs.freedesktop.org/show_bug.cgi?id=108778
and several others.

Alex

>
> Alex
>
> >
> > >
> > > ---
> > >  drivers/pci/pci.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > Index: linux-4.19/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-4.19.orig/drivers/pci/pci.c   2018-10-30 16:58:58.000000000 +0100
> > > +++ linux-4.19/drivers/pci/pci.c        2018-10-30 16:58:58.000000000 +0100
> > > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> > >
> > >         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > >         if (lnkcap) {
> > > -               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > > +               if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> > >                         return PCIE_SPEED_16_0GT;
> > > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> > >                         return PCIE_SPEED_8_0GT;
> > > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> > >                         return PCIE_SPEED_5_0GT;
> > > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> > >                         return PCIE_SPEED_2_5GT;
> > >         }
> > >

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Bjorn Helgaas]

On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> the register and compare it against them.
> 
> This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> the slot is being incorrectly reported as PCIe-v3 capable.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> Cc: stable@vger.kernel.org	# v4.17+
> 
> ---
>  drivers/pci/pci.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Index: linux-4.19/drivers/pci/pci.c
> ===================================================================
> --- linux-4.19.orig/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> +++ linux-4.19/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
>  
>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
>  	if (lnkcap) {
> -		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> +		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
>  			return PCIE_SPEED_16_0GT;
> -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
>  			return PCIE_SPEED_8_0GT;
> -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
>  			return PCIE_SPEED_5_0GT;
> -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
>  			return PCIE_SPEED_2_5GT;
>  	}
>  

I'd like to apply this as below, where I removed the 8_0GB and 16_0GB
cases as recommended by the spec.  I can't test it myself, and the
bugzillas don't contain enough information for me to confirm that the
patch below is enough (the "lspci -vv" output of the root port and GPU
is what I would need).

I'm confused about the fact that 6cf57be0f78e appeared in v4.17, but
v4.18 works fine according to both bugzillas.

I also don't have a good feel for whether this is urgent enough to be
a v4.20 fix or whether it can wait for v4.21.  Evidence either way
would help.

We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
(hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
qla24xx_pci_info_str(), and maybe a couple other places.

Bjorn


commit 871f73abf4b8e6aee8a206775f944ede7c7d7250
Author: Mikulas Patocka <mpatocka@redhat.com>
Date:   Tue Oct 30 12:36:08 2018 -0400

    PCI: Fix incorrect value returned from pcie_get_speed_cap()
    
    The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks.  We must mask
    the register and compare it against them.
    
    This patch fixes errors "amdgpu: [powerplay] failed to send message 261 ret
    is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because the
    slot is being incorrectly reported as PCIe-v3 capable.
    
    Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
    Link: https://bugs.freedesktop.org/show_bug.cgi?id=108704
    Link: https://bugs.freedesktop.org/show_bug.cgi?id=108778
    Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
    [bhelgaas: update comment, remove use of PCI_EXP_LNKCAP_SLS_8_0GB and
    PCI_EXP_LNKCAP_SLS_16_0GB since those should be covered by PCI_EXP_LNKCAP2]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Alex Deucher <alexander.deucher@amd.com>
    Cc: stable@vger.kernel.org      # v4.17+

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d068f11d08a7..8563d1d9b102 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5556,9 +5556,13 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
 	u32 lnkcap2, lnkcap;
 
 	/*
-	 * PCIe r4.0 sec 7.5.3.18 recommends using the Supported Link
-	 * Speeds Vector in Link Capabilities 2 when supported, falling
-	 * back to Max Link Speed in Link Capabilities otherwise.
+	 * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18.  The
+	 * implementation note there recommends using the Supported Link
+	 * Speeds Vector in Link Capabilities 2 when supported.
+	 *
+	 * Without Link Capabilities 2, i.e., prior to PCIe r3.0, software
+	 * should use the Supported Link Speeds field in Link Capabilities,
+	 * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
 	 */
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
 	if (lnkcap2) { /* PCIe r3.0-compliant */
@@ -5575,13 +5579,9 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
 
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
 	if (lnkcap) {
-		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
-			return PCIE_SPEED_16_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
-			return PCIE_SPEED_8_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
+		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
 			return PCIE_SPEED_5_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
+		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
 			return PCIE_SPEED_2_5GT;
 	}
 

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Alex Deucher]

On Mon, Nov 19, 2018 at 7:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > the register and compare it against them.
> >
> > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > the slot is being incorrectly reported as PCIe-v3 capable.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > Cc: stable@vger.kernel.org    # v4.17+
> >
> > ---
> >  drivers/pci/pci.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > Index: linux-4.19/drivers/pci/pci.c
> > ===================================================================
> > --- linux-4.19.orig/drivers/pci/pci.c 2018-10-30 16:58:58.000000000 +0100
> > +++ linux-4.19/drivers/pci/pci.c      2018-10-30 16:58:58.000000000 +0100
> > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> >
> >       pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> >       if (lnkcap) {
> > -             if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > +             if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> >                       return PCIE_SPEED_16_0GT;
> > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> >                       return PCIE_SPEED_8_0GT;
> > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> >                       return PCIE_SPEED_5_0GT;
> > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> >                       return PCIE_SPEED_2_5GT;
> >       }
> >
>
> I'd like to apply this as below, where I removed the 8_0GB and 16_0GB
> cases as recommended by the spec.  I can't test it myself, and the
> bugzillas don't contain enough information for me to confirm that the
> patch below is enough (the "lspci -vv" output of the root port and GPU
> is what I would need).
>
> I'm confused about the fact that 6cf57be0f78e appeared in v4.17, but
> v4.18 works fine according to both bugzillas.

This issue affects AMD GPUs because we switched from using an open
coded check for pcie link speeds in the driver to using the common
pcie variants in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.19&id=5d9a6330403271fbb1244f14380a7cc44662796f

The patch below would regress performance, at least on AMD GPUs, since
we'd end up reporting a max speed of gen 2 (5 GT/s) which would cause
the driver to limit the speed to gen2 even if gen3 or 4 are available.

>
> I also don't have a good feel for whether this is urgent enough to be
> a v4.20 fix or whether it can wait for v4.21.  Evidence either way
> would help.

I'd like it to land for 4.19 and 4.20 at least.  Alternatively, we
could revert all of the drm patches to and bring back all the open
coded implementations, but it's a fairly large number of patches to
revert.

Alex

>
> We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
> (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
> qla24xx_pci_info_str(), and maybe a couple other places.
>
> Bjorn
>
>
> commit 871f73abf4b8e6aee8a206775f944ede7c7d7250
> Author: Mikulas Patocka <mpatocka@redhat.com>
> Date:   Tue Oct 30 12:36:08 2018 -0400
>
>     PCI: Fix incorrect value returned from pcie_get_speed_cap()
>
>     The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks.  We must mask
>     the register and compare it against them.
>
>     This patch fixes errors "amdgpu: [powerplay] failed to send message 261 ret
>     is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because the
>     slot is being incorrectly reported as PCIe-v3 capable.
>
>     Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
>     Link: https://bugs.freedesktop.org/show_bug.cgi?id=108704
>     Link: https://bugs.freedesktop.org/show_bug.cgi?id=108778
>     Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>     [bhelgaas: update comment, remove use of PCI_EXP_LNKCAP_SLS_8_0GB and
>     PCI_EXP_LNKCAP_SLS_16_0GB since those should be covered by PCI_EXP_LNKCAP2]
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Acked-by: Alex Deucher <alexander.deucher@amd.com>
>     Cc: stable@vger.kernel.org      # v4.17+
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d068f11d08a7..8563d1d9b102 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5556,9 +5556,13 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
>         u32 lnkcap2, lnkcap;
>
>         /*
> -        * PCIe r4.0 sec 7.5.3.18 recommends using the Supported Link
> -        * Speeds Vector in Link Capabilities 2 when supported, falling
> -        * back to Max Link Speed in Link Capabilities otherwise.
> +        * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18.  The
> +        * implementation note there recommends using the Supported Link
> +        * Speeds Vector in Link Capabilities 2 when supported.
> +        *
> +        * Without Link Capabilities 2, i.e., prior to PCIe r3.0, software
> +        * should use the Supported Link Speeds field in Link Capabilities,
> +        * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
>          */
>         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
>         if (lnkcap2) { /* PCIe r3.0-compliant */
> @@ -5575,13 +5579,9 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
>
>         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
>         if (lnkcap) {
> -               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> -                       return PCIE_SPEED_16_0GT;
> -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> -                       return PCIE_SPEED_8_0GT;
> -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> +               if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
>                         return PCIE_SPEED_5_0GT;
> -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
>                         return PCIE_SPEED_2_5GT;
>         }
>

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Bjorn Helgaas]

On Tue, Nov 20, 2018 at 09:17:52AM -0500, Alex Deucher wrote:
> On Mon, Nov 19, 2018 at 7:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> > > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > > the register and compare it against them.
> > >
> > > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > > the slot is being incorrectly reported as PCIe-v3 capable.
> > >
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > > Cc: stable@vger.kernel.org    # v4.17+
> > >
> > > ---
> > >  drivers/pci/pci.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > Index: linux-4.19/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-4.19.orig/drivers/pci/pci.c 2018-10-30 16:58:58.000000000 +0100
> > > +++ linux-4.19/drivers/pci/pci.c      2018-10-30 16:58:58.000000000 +0100
> > > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> > >
> > >       pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > >       if (lnkcap) {
> > > -             if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > > +             if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> > >                       return PCIE_SPEED_16_0GT;
> > > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> > >                       return PCIE_SPEED_8_0GT;
> > > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> > >                       return PCIE_SPEED_5_0GT;
> > > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> > >                       return PCIE_SPEED_2_5GT;
> > >       }
> > >
> >
> > I'd like to apply this as below, where I removed the 8_0GB and 16_0GB
> > cases as recommended by the spec.  I can't test it myself, and the
> > bugzillas don't contain enough information for me to confirm that the
> > patch below is enough (the "lspci -vv" output of the root port and GPU
> > is what I would need).
> >
> > I'm confused about the fact that 6cf57be0f78e appeared in v4.17, but
> > v4.18 works fine according to both bugzillas.
> 
> This issue affects AMD GPUs because we switched from using an open
> coded check for pcie link speeds in the driver to using the common
> pcie variants in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.19&id=5d9a6330403271fbb1244f14380a7cc44662796f

OK, thanks.  I added that SHA1 and a note to explain the connection.

> The patch below would regress performance, at least on AMD GPUs, since
> we'd end up reporting a max speed of gen 2 (5 GT/s) which would cause
> the driver to limit the speed to gen2 even if gen3 or 4 are available.

I guess this means these parts are broken with respect to the spec,
since they support gen3 speeds but don't implement LnkCap2?

Gen2, i.e., PCIe r2.1, only defined 2.5 GT/s and 5 GT/s.  Devices
capable of the higher speeds added by PCIe r3.0 are supposed to
implement LnkCap2, but if we're even getting to this code, it means
LnkCap2 was zero.

If we confirm that this is a device defect, the question is what the
best way to work around it is.  Probably the original patch is easier
than some sort of quirk, but we'd need to expand the comment a little
bit to explain why we're not following the spec recommendation.

It looks like lspci probably also needs some updating here -- it
currently doesn't do anything at all with LnkCap2.

> > I also don't have a good feel for whether this is urgent enough to be
> > a v4.20 fix or whether it can wait for v4.21.  Evidence either way
> > would help.
> 
> I'd like it to land for 4.19 and 4.20 at least.  Alternatively, we
> could revert all of the drm patches to and bring back all the open
> coded implementations, but it's a fairly large number of patches to
> revert.

OK, sounds like it makes sense to do this for v4.20 and backport it at
least to v4.19 stable.  I do want to get the places below fixed also.
They may not be as urgent, but we might as well try and make
everything consistent while we're looking at it.

> > We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
> > (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
> > qla24xx_pci_info_str(), and maybe a couple other places.
> >
> > Bjorn
> >
> >
> > commit 871f73abf4b8e6aee8a206775f944ede7c7d7250
> > Author: Mikulas Patocka <mpatocka@redhat.com>
> > Date:   Tue Oct 30 12:36:08 2018 -0400
> >
> >     PCI: Fix incorrect value returned from pcie_get_speed_cap()
> >
> >     The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks.  We must mask
> >     the register and compare it against them.
> >
> >     This patch fixes errors "amdgpu: [powerplay] failed to send message 261 ret
> >     is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because the
> >     slot is being incorrectly reported as PCIe-v3 capable.
> >
> >     Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> >     Link: https://bugs.freedesktop.org/show_bug.cgi?id=108704
> >     Link: https://bugs.freedesktop.org/show_bug.cgi?id=108778
> >     Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> >     [bhelgaas: update comment, remove use of PCI_EXP_LNKCAP_SLS_8_0GB and
> >     PCI_EXP_LNKCAP_SLS_16_0GB since those should be covered by PCI_EXP_LNKCAP2]
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >     Acked-by: Alex Deucher <alexander.deucher@amd.com>
> >     Cc: stable@vger.kernel.org      # v4.17+
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index d068f11d08a7..8563d1d9b102 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5556,9 +5556,13 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
> >         u32 lnkcap2, lnkcap;
> >
> >         /*
> > -        * PCIe r4.0 sec 7.5.3.18 recommends using the Supported Link
> > -        * Speeds Vector in Link Capabilities 2 when supported, falling
> > -        * back to Max Link Speed in Link Capabilities otherwise.
> > +        * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18.  The
> > +        * implementation note there recommends using the Supported Link
> > +        * Speeds Vector in Link Capabilities 2 when supported.
> > +        *
> > +        * Without Link Capabilities 2, i.e., prior to PCIe r3.0, software
> > +        * should use the Supported Link Speeds field in Link Capabilities,
> > +        * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
> >          */
> >         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
> >         if (lnkcap2) { /* PCIe r3.0-compliant */
> > @@ -5575,13 +5579,9 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
> >
> >         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> >         if (lnkcap) {
> > -               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > -                       return PCIE_SPEED_16_0GT;
> > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > -                       return PCIE_SPEED_8_0GT;
> > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > +               if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
> >                         return PCIE_SPEED_5_0GT;
> > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> >                         return PCIE_SPEED_2_5GT;
> >         }
> >

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Alex Deucher]

On Tue, Nov 20, 2018 at 10:13 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Nov 20, 2018 at 09:17:52AM -0500, Alex Deucher wrote:
> > On Mon, Nov 19, 2018 at 7:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> > > > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > > > the register and compare it against them.
> > > >
> > > > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > > > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > > > the slot is being incorrectly reported as PCIe-v3 capable.
> > > >
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > > > Cc: stable@vger.kernel.org    # v4.17+
> > > >
> > > > ---
> > > >  drivers/pci/pci.c |    8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > Index: linux-4.19/drivers/pci/pci.c
> > > > ===================================================================
> > > > --- linux-4.19.orig/drivers/pci/pci.c 2018-10-30 16:58:58.000000000 +0100
> > > > +++ linux-4.19/drivers/pci/pci.c      2018-10-30 16:58:58.000000000 +0100
> > > > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> > > >
> > > >       pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > > >       if (lnkcap) {
> > > > -             if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > > > +             if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> > > >                       return PCIE_SPEED_16_0GT;
> > > > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > > > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> > > >                       return PCIE_SPEED_8_0GT;
> > > > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > > > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> > > >                       return PCIE_SPEED_5_0GT;
> > > > -             else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > > > +             else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> > > >                       return PCIE_SPEED_2_5GT;
> > > >       }
> > > >
> > >
> > > I'd like to apply this as below, where I removed the 8_0GB and 16_0GB
> > > cases as recommended by the spec.  I can't test it myself, and the
> > > bugzillas don't contain enough information for me to confirm that the
> > > patch below is enough (the "lspci -vv" output of the root port and GPU
> > > is what I would need).
> > >
> > > I'm confused about the fact that 6cf57be0f78e appeared in v4.17, but
> > > v4.18 works fine according to both bugzillas.
> >
> > This issue affects AMD GPUs because we switched from using an open
> > coded check for pcie link speeds in the driver to using the common
> > pcie variants in
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v4.19&id=5d9a6330403271fbb1244f14380a7cc44662796f
>
> OK, thanks.  I added that SHA1 and a note to explain the connection.
>
> > The patch below would regress performance, at least on AMD GPUs, since
> > we'd end up reporting a max speed of gen 2 (5 GT/s) which would cause
> > the driver to limit the speed to gen2 even if gen3 or 4 are available.
>
> I guess this means these parts are broken with respect to the spec,
> since they support gen3 speeds but don't implement LnkCap2?

Sorry, I mis-read the patch.  The patch is correct and our parts are
compliant.  We are only hitting this issue on non-gen3 platforms which
would end up in the second case.  Sorry for the confusion.  You new
patch is:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

Alex

>
> Gen2, i.e., PCIe r2.1, only defined 2.5 GT/s and 5 GT/s.  Devices
> capable of the higher speeds added by PCIe r3.0 are supposed to
> implement LnkCap2, but if we're even getting to this code, it means
> LnkCap2 was zero.
>
> If we confirm that this is a device defect, the question is what the
> best way to work around it is.  Probably the original patch is easier
> than some sort of quirk, but we'd need to expand the comment a little
> bit to explain why we're not following the spec recommendation.
>
> It looks like lspci probably also needs some updating here -- it
> currently doesn't do anything at all with LnkCap2.
>
> > > I also don't have a good feel for whether this is urgent enough to be
> > > a v4.20 fix or whether it can wait for v4.21.  Evidence either way
> > > would help.
> >
> > I'd like it to land for 4.19 and 4.20 at least.  Alternatively, we
> > could revert all of the drm patches to and bring back all the open
> > coded implementations, but it's a fairly large number of patches to
> > revert.
>
> OK, sounds like it makes sense to do this for v4.20 and backport it at
> least to v4.19 stable.  I do want to get the places below fixed also.
> They may not be as urgent, but we might as well try and make
> everything consistent while we're looking at it.
>
> > > We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
> > > (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
> > > qla24xx_pci_info_str(), and maybe a couple other places.
> > >
> > > Bjorn
> > >
> > >
> > > commit 871f73abf4b8e6aee8a206775f944ede7c7d7250
> > > Author: Mikulas Patocka <mpatocka@redhat.com>
> > > Date:   Tue Oct 30 12:36:08 2018 -0400
> > >
> > >     PCI: Fix incorrect value returned from pcie_get_speed_cap()
> > >
> > >     The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks.  We must mask
> > >     the register and compare it against them.
> > >
> > >     This patch fixes errors "amdgpu: [powerplay] failed to send message 261 ret
> > >     is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because the
> > >     slot is being incorrectly reported as PCIe-v3 capable.
> > >
> > >     Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > >     Link: https://bugs.freedesktop.org/show_bug.cgi?id=108704
> > >     Link: https://bugs.freedesktop.org/show_bug.cgi?id=108778
> > >     Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > >     [bhelgaas: update comment, remove use of PCI_EXP_LNKCAP_SLS_8_0GB and
> > >     PCI_EXP_LNKCAP_SLS_16_0GB since those should be covered by PCI_EXP_LNKCAP2]
> > >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > >     Acked-by: Alex Deucher <alexander.deucher@amd.com>
> > >     Cc: stable@vger.kernel.org      # v4.17+
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index d068f11d08a7..8563d1d9b102 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5556,9 +5556,13 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
> > >         u32 lnkcap2, lnkcap;
> > >
> > >         /*
> > > -        * PCIe r4.0 sec 7.5.3.18 recommends using the Supported Link
> > > -        * Speeds Vector in Link Capabilities 2 when supported, falling
> > > -        * back to Max Link Speed in Link Capabilities otherwise.
> > > +        * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18.  The
> > > +        * implementation note there recommends using the Supported Link
> > > +        * Speeds Vector in Link Capabilities 2 when supported.
> > > +        *
> > > +        * Without Link Capabilities 2, i.e., prior to PCIe r3.0, software
> > > +        * should use the Supported Link Speeds field in Link Capabilities,
> > > +        * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
> > >          */
> > >         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
> > >         if (lnkcap2) { /* PCIe r3.0-compliant */
> > > @@ -5575,13 +5579,9 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
> > >
> > >         pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > >         if (lnkcap) {
> > > -               if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > > -                       return PCIE_SPEED_16_0GT;
> > > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > > -                       return PCIE_SPEED_8_0GT;
> > > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > > +               if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
> > >                         return PCIE_SPEED_5_0GT;
> > > -               else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > > +               else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> > >                         return PCIE_SPEED_2_5GT;
> > >         }
> > >

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Bjorn Helgaas]

On Mon, Nov 19, 2018 at 06:47:04PM -0600, Bjorn Helgaas wrote:
> On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > the register and compare it against them.
> > 
> > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > the slot is being incorrectly reported as PCIe-v3 capable.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > Cc: stable@vger.kernel.org	# v4.17+
> > 
> > ---
> >  drivers/pci/pci.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > Index: linux-4.19/drivers/pci/pci.c
> > ===================================================================
> > --- linux-4.19.orig/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> > +++ linux-4.19/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> >  
> >  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> >  	if (lnkcap) {
> > -		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > +		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> >  			return PCIE_SPEED_16_0GT;
> > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> >  			return PCIE_SPEED_8_0GT;
> > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> >  			return PCIE_SPEED_5_0GT;
> > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> >  			return PCIE_SPEED_2_5GT;
> >  	}

> We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
> (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
> qla24xx_pci_info_str(), and maybe a couple other places.

Does anybody want to volunteer to fix the places above as well?  I
found them by grepping for PCI_EXP_LNKCAP, and they're all broken in
ways similar to pcie_get_speed_cap().  Possibly some of these places
could use pcie_get_speed_cap() directly.

Bjorn

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Mikulas Patocka]

On Mon, 26 Nov 2018, Bjorn Helgaas wrote:

> On Mon, Nov 19, 2018 at 06:47:04PM -0600, Bjorn Helgaas wrote:
> > On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> > > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > > the register and compare it against them.
> > > 
> > > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > > the slot is being incorrectly reported as PCIe-v3 capable.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > > Cc: stable@vger.kernel.org	# v4.17+
> > > 
> > > ---
> > >  drivers/pci/pci.c |    8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > Index: linux-4.19/drivers/pci/pci.c
> > > ===================================================================
> > > --- linux-4.19.orig/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> > > +++ linux-4.19/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> > > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> > >  
> > >  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > >  	if (lnkcap) {
> > > -		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > > +		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> > >  			return PCIE_SPEED_16_0GT;
> > > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> > >  			return PCIE_SPEED_8_0GT;
> > > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> > >  			return PCIE_SPEED_5_0GT;
> > > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> > >  			return PCIE_SPEED_2_5GT;
> > >  	}
> 
> > We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
> > (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
> > qla24xx_pci_info_str(), and maybe a couple other places.
> 
> Does anybody want to volunteer to fix the places above as well?  I
> found them by grepping for PCI_EXP_LNKCAP, and they're all broken in
> ways similar to pcie_get_speed_cap().  Possibly some of these places
> could use pcie_get_speed_cap() directly.
> 
> Bjorn
> 

They are not broken, they are masking the value with PCI_EXP_LNKCAP_SLS - 
that is correct.

pci_set_bus_speed:
                pcie_capability_read_dword(bridge, PCI_EXP_LNKCAP, &linkcap);
                bus->max_bus_speed = pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS];

pcie_speeds:
	if ((linkcap & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_8_0GB)

cobalt_pcie_status_show:
	just prints the values without doing anything with them

hba_ioctl_callback:
	gai->pci.link_speed_max = (u8)(caps & PCI_EXP_LNKCAP_SLS);

	gai->pci.link_width_max = (u8)((caps & PCI_EXP_LNKCAP_MLW) >> 4);

qla24xx_pci_info_str:
	lspeed = lstat & PCI_EXP_LNKCAP_SLS;
	lwidth = (lstat & PCI_EXP_LNKCAP_MLW) >> 4;

Mikulas

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Bjorn Helgaas]

On Tue, Nov 27, 2018 at 11:49:43AM -0500, Mikulas Patocka wrote:
> On Mon, 26 Nov 2018, Bjorn Helgaas wrote:
> > On Mon, Nov 19, 2018 at 06:47:04PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> > > > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> > > > the register and compare it against them.
> > > > 
> > > > This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> > > > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> > > > the slot is being incorrectly reported as PCIe-v3 capable.
> > > > 
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> > > > Cc: stable@vger.kernel.org	# v4.17+
> > > > 
> > > > ---
> > > >  drivers/pci/pci.c |    8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > Index: linux-4.19/drivers/pci/pci.c
> > > > ===================================================================
> > > > --- linux-4.19.orig/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> > > > +++ linux-4.19/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> > > > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
> > > >  
> > > >  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > > >  	if (lnkcap) {
> > > > -		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> > > > +		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
> > > >  			return PCIE_SPEED_16_0GT;
> > > > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> > > > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
> > > >  			return PCIE_SPEED_8_0GT;
> > > > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> > > > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
> > > >  			return PCIE_SPEED_5_0GT;
> > > > -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> > > > +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
> > > >  			return PCIE_SPEED_2_5GT;
> > > >  	}
> > 
> > > We also need similar fixes in pci_set_bus_speed(), pcie_speeds()
> > > (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(),
> > > qla24xx_pci_info_str(), and maybe a couple other places.
> > 
> > Does anybody want to volunteer to fix the places above as well?  I
> > found them by grepping for PCI_EXP_LNKCAP, and they're all broken in
> > ways similar to pcie_get_speed_cap().  Possibly some of these places
> > could use pcie_get_speed_cap() directly.
> 
> They are not broken, they are masking the value with PCI_EXP_LNKCAP_SLS - 
> that is correct.

You're right, "broken" is an overstatement except in terms of
maintainance: they do happen to work correctly, but there's no reason
to reimplement the same functionality several places in slightly
different ways.

pcie_get_speed_cap() looks at LNKCAP2, then LNKCAP.  The other places
look only at LNKCAP.  Since these are all finding only the max link
speed (which can be determined using only LKNCAP), not the set of
supported speeds (which requires both LNKCAP2 and LNKCAP), they should
all do it the same way.

> pci_set_bus_speed:
>                 pcie_capability_read_dword(bridge, PCI_EXP_LNKCAP, &linkcap);
>                 bus->max_bus_speed = pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS];
> 
> pcie_speeds:
> 	if ((linkcap & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_8_0GB)
> 
> cobalt_pcie_status_show:
> 	just prints the values without doing anything with them
> 
> hba_ioctl_callback:
> 	gai->pci.link_speed_max = (u8)(caps & PCI_EXP_LNKCAP_SLS);
> 
> 	gai->pci.link_width_max = (u8)((caps & PCI_EXP_LNKCAP_MLW) >> 4);
> 
> qla24xx_pci_info_str:
> 	lspeed = lstat & PCI_EXP_LNKCAP_SLS;
> 	lwidth = (lstat & PCI_EXP_LNKCAP_MLW) >> 4;
> 
> Mikulas

Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap

[Bjorn Helgaas]

On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote:
> The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask
> the register and compare it against them.
> 
> This patch fixes errors "amdgpu: [powerplay] failed to send message 261
> ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because
> the slot is being incorrectly reported as PCIe-v3 capable.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Applied the patch below to for-linus for v4.20 and backport to v4.17+
stable kernels, thanks!

I removed the "if (lnkcap)" test because the LNKCAP case is not
parallel to the LNKCAP2 case.  LNKCAP2 is optional so we have to test
for lnkcap2 being non-zero.  But LNKCAP is required for all devices.
If the config read of it fails, we should get either 0 or ~0, and
neither one will satisfy the 5.0 or 2.5 checks.

> Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
> Cc: stable@vger.kernel.org	# v4.17+
> 
> ---
>  drivers/pci/pci.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Index: linux-4.19/drivers/pci/pci.c
> ===================================================================
> --- linux-4.19.orig/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> +++ linux-4.19/drivers/pci/pci.c	2018-10-30 16:58:58.000000000 +0100
> @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st
>  
>  	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
>  	if (lnkcap) {
> -		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
> +		if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB)
>  			return PCIE_SPEED_16_0GT;
> -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
> +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB)
>  			return PCIE_SPEED_8_0GT;
> -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
> +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB)
>  			return PCIE_SPEED_5_0GT;
> -		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
> +		else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
>  			return PCIE_SPEED_2_5GT;
>  	}
>  

commit 94ea01a6d9a6
Author: Mikulas Patocka <mpatocka@redhat.com>
Date:   Mon Nov 26 10:37:13 2018 -0600

    PCI: Fix incorrect value returned from pcie_get_speed_cap()
    
    The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks.  We must mask
    the register and compare it against them.
    
    This fixes errors like this:
    
      amdgpu: [powerplay] failed to send message 261 ret is 0
    
    when PCIe-v3 card is plugged into a PCIe-v1 slot, because the slot is being
    incorrectly reported as PCIe-v3 capable.
    
    6cf57be0f78e, which appeared in v4.17, added pcie_get_speed_cap() with the
    incorrect test of PCI_EXP_LNKCAP_SLS as a bitmask.  5d9a63304032, which
    appeared in v4.19, changed amdgpu to use pcie_get_speed_cap(), so the
    amdgpu bug reports below are regressions in v4.19.
    
    Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed")
    Fixes: 5d9a63304032 ("drm/amdgpu: use pcie functions for link width and speed")
    Link: https://bugs.freedesktop.org/show_bug.cgi?id=108704
    Link: https://bugs.freedesktop.org/show_bug.cgi?id=108778
    Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
    [bhelgaas: update comment, remove use of PCI_EXP_LNKCAP_SLS_8_0GB and
    PCI_EXP_LNKCAP_SLS_16_0GB since those should be covered by PCI_EXP_LNKCAP2,
    remove test of PCI_EXP_LNKCAP for zero, since that register is required]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Alex Deucher <alexander.deucher@amd.com>
    Cc: stable@vger.kernel.org      # v4.17+

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d068f11d08a7..c9d8e3c837de 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5556,9 +5556,13 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
 	u32 lnkcap2, lnkcap;
 
 	/*
-	 * PCIe r4.0 sec 7.5.3.18 recommends using the Supported Link
-	 * Speeds Vector in Link Capabilities 2 when supported, falling
-	 * back to Max Link Speed in Link Capabilities otherwise.
+	 * Link Capabilities 2 was added in PCIe r3.0, sec 7.8.18.  The
+	 * implementation note there recommends using the Supported Link
+	 * Speeds Vector in Link Capabilities 2 when supported.
+	 *
+	 * Without Link Capabilities 2, i.e., prior to PCIe r3.0, software
+	 * should use the Supported Link Speeds field in Link Capabilities,
+	 * where only 2.5 GT/s and 5.0 GT/s speeds were defined.
 	 */
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
 	if (lnkcap2) { /* PCIe r3.0-compliant */
@@ -5574,16 +5578,10 @@ enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev)
 	}
 
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
-	if (lnkcap) {
-		if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB)
-			return PCIE_SPEED_16_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB)
-			return PCIE_SPEED_8_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB)
-			return PCIE_SPEED_5_0GT;
-		else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB)
-			return PCIE_SPEED_2_5GT;
-	}
+	if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
+		return PCIE_SPEED_5_0GT;
+	else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB)
+		return PCIE_SPEED_2_5GT;
 
 	return PCI_SPEED_UNKNOWN;
 }