On Thu, 12 Dec 2024, Lukas Wunner wrote:
> The Supported Link Speeds Vector in the Link Capabilities 2 Register
> indicates the *supported* link speeds. The Max Link Speed field in
> the Link Capabilities Register indicates the *maximum* of those speeds.
>
> Niklas reports that the Intel JHL7540 "Titan Ridge 2018" Thunderbolt
> controller supports 2.5-8 GT/s speeds, but indicates 2.5 GT/s as maximum.
> Ilpo recalls seeing this inconsistency on more devices.
>
> pcie_get_supported_speeds() neglects to honor the Max Link Speed field
> and will thus incorrectly deem higher speeds as supported. Fix it.
>
> Fixes: d2bd39c0456b ("PCI: Store all PCIe Supported Link Speeds")
> Reported-by: Niklas Schnelle <niks@xxxxxxxxxx>
> Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@xxxxxxxxxx/
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> drivers/pci/pci.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 35dc9f2..b730560 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6240,12 +6240,14 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
> pcie_capability_read_dword(dev, PCI_EXP_LNKCAP2, &lnkcap2);
> speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS;
>
> + /* Ignore speeds higher than Max Link Speed */
> + pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> + speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, 0);
Hi Lukas,
Why do you start GENMASK() from 0th position? That's the reserved bit.
(I doesn't exactly cause a misbehavior to & the never set 0th bit but
it is slightly confusing).
I suggest to get that either from PCI_EXP_LNKCAP2_SLS_2_5GB or
PCI_EXP_LNKCAP2_SLS (e.g. with __ffs()) and do not use literal at all
to make it explicit where it originates from.
--
i.
> +
> /* PCIe r3.0-compliant */
> if (speeds)
> return speeds;
>
> - pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> -
> /* Synthesize from the Max Link Speed field */
> if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_5_0GB)
> speeds = PCI_EXP_LNKCAP2_SLS_5_0GB | PCI_EXP_LNKCAP2_SLS_2_5GB;
>
