On Sun, 15 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.
>
> pcie_get_supported_speeds() neglects to honor the Max Link Speed field
> and will thus incorrectly deem higher speeds as supported. Fix it.
>
> One user-visible issue addressed here is an incorrect value in the sysfs
> attribute "max_link_speed".
>
> But the main motivation is a boot hang reported by Niklas: Intel JHL7540
> "Titan Ridge 2018" Thunderbolt controllers supports 2.5-8 GT/s speeds,
> but indicate 2.5 GT/s as maximum. Ilpo recalls seeing this on more
> devices. It can be explained by the controller's Downstream Ports
> supporting 8 GT/s if an Endpoint is attached, but limiting to 2.5 GT/s
> if the port interfaces to a PCIe Adapter, in accordance with USB4 v2
> sec 11.2.1:
>
> "This section defines the functionality of an Internal PCIe Port that
> interfaces to a PCIe Adapter. [...]
> The Logical sub-block shall update the PCIe configuration registers
> with the following characteristics: [...]
> Max Link Speed field in the Link Capabilities Register set to 0001b
> (data rate of 2.5 GT/s only).
> Note: These settings do not represent actual throughput. Throughput
> is implementation specific and based on the USB4 Fabric performance."
>
> The present commit is not sufficient on its own to fix Niklas' boot hang,
> but it is a prerequisite.
>
> 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 | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ab0ef7b6c798..9f672399e688 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6247,6 +6247,10 @@ 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 */
> + speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS,
> + PCI_EXP_LNKCAP2_SLS_2_5GB);
You pass a value instead of bit position to GENMASK() which is not
correct way to use GENMASK(). You need to do either:
ilog2(PCI_EXP_LNKCAP2_SLS_2_5GB)
or
__ffs(PCI_EXP_LNKCAP2_SLS)
(Technically, also __ffs(PCI_EXP_LNKCAP2_SLS_2_5GB) would work).
+ Please check the correct header is included depending on which you pick.
> +
> /* PCIe r3.0-compliant */
> if (speeds)
> return speeds;
>
--
i.
