diff mbox series

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

Message ID 2292e75dcf26f1c6d7c1f715edfe0e49f079149d.1734257330.git.lukas@wunner.de (mailing list archive)
State Superseded
Commit 953f189b700da9a97b1d9fb8a0ac9175be5d57d5
Delegated to: Krzysztof Wilczyński
Headers show
Series Fix bwctrl boot hang | expand

Commit Message

Lukas Wunner Dec. 15, 2024, 10:20 a.m. UTC 
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.

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(-)

Comments

Niklas Schnelle Dec. 15, 2024, 9:03 p.m. UTC | #1 
On Sun, 2024-12-15 at 11:20 +0100, Lukas Wunner 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.
> 
> 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;
>  	}
>  

I can confirm that this in combination with the two other patches fixes
my problem. I'm still a little unsure if we want to go with a more
minimal patch for v6.13-rc to take more time to figure out the correct
handling in patch 1, but I think medium term this is the right overall
approach.

Either way, feel free to add:

Tested-by: Niklas Schnelle <niks@kernel.org>
Jonathan Cameron Dec. 16, 2024, 11:32 a.m. UTC | #2 
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>

> 
> 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;
>  	}
>
Mario Limonciello Dec. 16, 2024, 2:20 p.m. UTC | #3 
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;
>>   	}
>>   
>
diff mbox series

Patch

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;
 	}