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

[Bjorn Helgaas <helgaas@kernel.org>]

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