Re: [PATCH] PCI/portdrv: Disable bwctrl service if port is fixed at 2.5 GT/s

[Lukas Wunner <lukas@wunner.de>]

On Sat, Dec 07, 2024 at 07:44:09PM +0100, Niklas Schnelle wrote:
> Trying to enable bwctrl on a Thunderbolt port causes a boot hang on some
> systems though the exact reason is not yet understood.

Probably worth highlighting the discrete Thunderbolt chip which exhibits
this issue, i.e. Intel JHL7540 (Titan Ridge).

> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -270,7 +270,8 @@ static int get_port_device_capability(struct pci_dev *dev)
>  		u32 linkcap;
>  
>  		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap);
> -		if (linkcap & PCI_EXP_LNKCAP_LBNC)
> +		if (linkcap & PCI_EXP_LNKCAP_LBNC &&
> +		    (linkcap & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB)
>  			services |= PCIE_PORT_SERVICE_BWCTRL;
>  	}

This is fine in principle because PCIe r6.2 sec 8.2.1 states:

   "A device must support 2.5 GT/s and is not permitted to skip support
    for any data rates between 2.5 GT/s and the highest supported rate."

However the Implementation Note at the end of PCIe r6.2 sec 7.5.3.18
cautions:

   "It is strongly encouraged that software primarily utilize the
    Supported Link Speeds Vector instead of the Max Link Speed field,
    so that software can determine the exact set of supported speeds
    on current and future hardware. This can avoid software being
    confused if a future specification defines Links that do not
    require support for all slower speeds."

First of all, the Supported Link Speeds field in the Link Capabilities
register (which you're querying here) was renamed to Max Link Speed in
PCIe r3.1 and a new Link Capabilities 2 register was added which contains
a new Supported Link Speeds field.  Software is supposed to query the
latter if the device implements the Link Capabilities 2 register
(see the other Implementation Note at the end of PCIe r6.2 sec 7.5.3.18).

Second, the above-quoted Implementation Note says that software should
not rely on future spec versions to mandate that *all* link speeds
(2.5 GT/s and all intermediate speeds up to the maximum supported speed)
are supported.

Since v6.13-rc1, we cache the supported speeds in the "supported_speeds"
field in struct pci_dev, taking care of the PCIe 3.0 versus later versions
issue.

So to make this future-proof what you could do is check whether only a
*single* speed is supported (which could be something else than 2.5 GT/s
if future spec versions allow that), i.e.:

-		if (linkcap & PCI_EXP_LNKCAP_LBNC)
+		if (linkcap & PCI_EXP_LNKCAP_LBNC &&
+		    hweight8(dev->supported_speeds) > 1)

...and optionally add a code comment, e.g.:

	/* Enable bandwidth control if more than one speed is supported. */

Thanks,

Lukas