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 <mpato...@redhat.com> > > > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max > > > > supported link speed") > > > > Cc: sta...@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 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx