Re: [PATCH for-linus v2 3/3] PCI/bwctrl: Enable only if more than one speed is supported

Mario Limonciello
2024-12-16T14:20:06Z

On 12/16/2024 05:32, Jonathan Cameron wrote:
> On Sun, 15 Dec 2024 11:20:53 +0100
> Lukas Wunner <lukas@wunner.de> wrote:
>
>> If a PCIe port only supports a single speed, enabling bandwidth control
>> is pointless: There's no need to monitor autonomous speed changes, nor
>> can the speed be changed.
>>
>> Not enabling it saves a small amount of memory and compute resources,
>> but also fixes a boot hang reported by Niklas: It occurs when enabling
>> bandwidth control on Downstream Ports of Intel JHL7540 "Titan Ridge 2018"
>> Thunderbolt controllers. The ports only support 2.5 GT/s in accordance
>> with USB4 v2 sec 11.2.1, so the present commit works around the issue.
>>
>> PCIe r6.2 sec 8.2.1 prescribes that:
>>
>> "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."
>>
>> Consequently, bandwidth control is currently only disabled if a port
>> doesn't support higher speeds than 2.5 GT/s. However the Implementation
>> Note in 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."
>>
>> In other words, future revisions of the PCIe Base Spec may allow gaps
>> in the Supported Link Speeds Vector. To be future-proof, don't just
>> check whether speeds above 2.5 GT/s are supported, but rather check
>> whether *more than one* speed is supported.
>
> It has long felt like the need for the very low speeds will become optional
> eventually, though the challenges of getting a backwards compatibility
> change into the specification may make that take a while. Hence
> I agree this approach makes sense.
>
> Reviewed-by: Jonathan Cameron <Jonthan.Cameron@huawei.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

>
>>
>> Fixes: 665745f27487 ("PCI/bwctrl: Re-add BW notification portdrv as PCIe BW controller")
>> Reported-by: Niklas Schnelle <niks@kernel.org>
>> Closes: https://lore.kernel.org/r/db8e457fcd155436449b035e8791a8241b0df400.camel@kernel.org/
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> drivers/pci/pcie/portdrv.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
>> index 5e10306b6308..02e73099bad0 100644
>> --- a/drivers/pci/pcie/portdrv.c
>> +++ b/drivers/pci/pcie/portdrv.c
>> @@ -265,12 +265,14 @@ static int get_port_device_capability(struct pci_dev *dev)
>> (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>> services |= PCIE_PORT_SERVICE_DPC;
>>
>> + /* Enable bandwidth control if more than one speed is supported. */
>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
>> u32 linkcap;
>>
>> pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap);
>> - if (linkcap & PCI_EXP_LNKCAP_LBNC)
>> + if (linkcap & PCI_EXP_LNKCAP_LBNC &&
>> + hweight8(dev->supported_speeds) > 1)
>> services |= PCIE_PORT_SERVICE_BWCTRL;
>> }
>>
>