[PATCH for-linus v2 0/3] Fix bwctrl boot hang

[PATCH for-linus v2 0/3] Fix bwctrl boot hang

[Lukas Wunner]

Here's my renewed proposal to fix the boot hang reported by Niklas
when enabling the bandwidth controller on Intel JHL7540 "Titan Ridge 2018"
Thunderbolt controllers.

@Niklas, could you re-test this?

I believe I've addressed all the feedback on v1, please let me know
if I've missed anything.


Changes v1 -> v2:

* [PATCH 2/3] PCI: Honor Max Link Speed when determining supported speeds
  * Use PCI_EXP_LNKCAP2_SLS_2_5GB as lowest bit in GENMASK() macro
    (Ilpo, Niklas).
  * Mention user-visible issues addressed by the patch in commit message
    (Bjorn).

* [PATCH 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined
  * New patch to prevent invocation of malformed GENMASK(0, lowest) macro.

* [PATCH 3/3] PCI/bwctrl: Enable only if more than one speed is supported
  * New patch to prevent the boot hang.  This is a future-proof alternative
    to Niklas' patch.


Link to v1, prior discussion and Niklas' patch:

https://lore.kernel.org/r/e3386d62a766be6d0ef7138a001dabfe563cdff8.1733991971.git.lukas@wunner.de/
https://lore.kernel.org/r/db8e457fcd155436449b035e8791a8241b0df400.camel@kernel.org/
https://lore.kernel.org/r/20241207-fix_bwctrl_thunderbolt-v1-1-b711f572a705@kernel.org/
https://lore.kernel.org/r/20241213-fix_bwctrl_thunderbolt-v2-1-b52fef641dfc@kernel.org/


Lukas Wunner (3):
  PCI: Assume 2.5 GT/s if Max Link Speed is undefined
  PCI: Honor Max Link Speed when determining supported speeds
  PCI/bwctrl: Enable only if more than one speed is supported

 drivers/pci/pci.c          | 13 +++++++++++--
 drivers/pci/pcie/portdrv.c |  4 +++-
 2 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH for-linus v2 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined

[Lukas Wunner]

Broken PCIe devices may not set any of the bits in the Link Capabilities
Register's "Max Link Speed" field.  Assume 2.5 GT/s in such a case,
which is the lowest possible PCIe speed.  It must be supported by every
device per PCIe r6.2 sec 8.2.1.

Emit a message informing about the malformed field.  Use KERN_INFO
severity to minimize annoyance.  This will help silicon validation
engineers take note of the issue so that regular users hopefully never
see it.

There is currently no known affected product, but a subsequent commit
will honor the Max Link Speed field when determining supported speeds
and depends on the field being well-formed.  (It uses the Max Link Speed
as highest bit in a GENMASK(highest, lowest) macro and if the field is
zero, that would result in GENMASK(0, lowest).)

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 35dc9f249b86..ab0ef7b6c798 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
 	u32 lnkcap2, lnkcap;
 	u8 speeds;
 
+	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
+	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
+	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
+		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");
+		return PCI_EXP_LNKCAP2_SLS_2_5GB;
+	}
+
 	/*
 	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
 	 * Speeds Vector to allow using SLS Vector bit defines directly.
@@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
 	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;
-- 
2.43.0

Re: [PATCH for-linus v2 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined

[Niklas Schnelle]

On Sun, 2024-12-15 at 11:20 +0100, Lukas Wunner wrote:
> Broken PCIe devices may not set any of the bits in the Link Capabilities
> Register's "Max Link Speed" field.  Assume 2.5 GT/s in such a case,
> which is the lowest possible PCIe speed.  It must be supported by every
> device per PCIe r6.2 sec 8.2.1.
> 
> Emit a message informing about the malformed field.  Use KERN_INFO
> severity to minimize annoyance.  This will help silicon validation
> engineers take note of the issue so that regular users hopefully never
> see it.
> 
> There is currently no known affected product, but a subsequent commit
> will honor the Max Link Speed field when determining supported speeds
> and depends on the field being well-formed.  (It uses the Max Link Speed
> as highest bit in a GENMASK(highest, lowest) macro and if the field is
> zero, that would result in GENMASK(0, lowest).)
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 35dc9f249b86..ab0ef7b6c798 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  	u32 lnkcap2, lnkcap;
>  	u8 speeds;
>  
> +	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
> +	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> +	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
> +		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");
> +		return PCI_EXP_LNKCAP2_SLS_2_5GB;
> +	}
> +
>  	/*
>  	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
>  	 * Speeds Vector to allow using SLS Vector bit defines directly.
> @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  	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;

I feel like this patch goes a bit against the idea of this being more
future proof. Personally, I kind of expect that any future devices
which may skip support for lower speeds would start with skipping 2.5
GT/s and a future PCIe spec might allow this.

In that case with the above code we end up assuming 2.5 GT/s which
won't work while the Supported Link Speeds Vector could contain
supported speeds with the assumption that when in doubt software relies
on that (PCIe r6.2 sec 7.5.3.18) and it might even be future spec
conformant. 

So I think instead of assuming 2.5 GT/s I was thinking of something
like the diff below (on top of this series).

Thanks
Niklas

----
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ef5c48bda012..cfb34fa96f81 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6223,14 +6223,11 @@ EXPORT_SYMBOL(pcie_bandwidth_available);
 u8 pcie_get_supported_speeds(struct pci_dev *dev)
 {
 	u32 lnkcap2, lnkcap;
-	u8 speeds;
+	u8 speeds, max_bits;
 
 	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
-	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
-		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");
-		return PCI_EXP_LNKCAP2_SLS_2_5GB;
-	}
+	max_bits = lnkcap & PCI_EXP_LNKCAP_SLS;
 
 	/*
 	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
@@ -6238,10 +6235,11 @@ 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);
+	if (max_bits)
+		speeds &= GENMASK(max_bits, PCI_EXP_LNKCAP2_SLS_2_5GB);
+	else
+		pci_info(dev, "Undefined Max Link Speed; relying on LnkCap2\n");
 
 	/* PCIe r3.0-compliant */
 	if (speeds)

Re: [PATCH for-linus v2 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined

[Lukas Wunner]

On Sun, Dec 15, 2024 at 10:17:46PM +0100, Niklas Schnelle wrote:
> On Sun, 2024-12-15 at 11:20 +0100, Lukas Wunner wrote:
> > Broken PCIe devices may not set any of the bits in the Link Capabilities
> > Register's "Max Link Speed" field.  Assume 2.5 GT/s in such a case,
> > which is the lowest possible PCIe speed.  It must be supported by every
> > device per PCIe r6.2 sec 8.2.1.
[...]
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
> >  	u32 lnkcap2, lnkcap;
> >  	u8 speeds;
> >  
> > +	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
> > +	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > +	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
> > +		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");
> > +		return PCI_EXP_LNKCAP2_SLS_2_5GB;
> > +	}
> > +
> >  	/*
> >  	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
> >  	 * Speeds Vector to allow using SLS Vector bit defines directly.
> 
> I feel like this patch goes a bit against the idea of this being more
> future proof. Personally, I kind of expect that any future devices
> which may skip support for lower speeds would start with skipping 2.5
> GT/s and a future PCIe spec might allow this.
> 
> In that case with the above code we end up assuming 2.5 GT/s which
> won't work while the Supported Link Speeds Vector could contain
> supported speeds with the assumption that when in doubt software relies
> on that (PCIe r6.2 sec 7.5.3.18) and it might even be future spec
> conformant.
> 
> So I think instead of assuming 2.5 GT/s I was thinking of something
> like the diff below (on top of this series).
[...]
> @@ -6238,10 +6235,11 @@ 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);
> +	if (max_bits)
> +		speeds &= GENMASK(max_bits, PCI_EXP_LNKCAP2_SLS_2_5GB);
> +	else
> +		pci_info(dev, "Undefined Max Link Speed; relying on LnkCap2\n");

I see.  Right now assuming 2.5 GT/s is the most conservative approach.
We may have to revisit this once the PCIe spec does allow gaps in the
Supported Link Speeds.  Then again, I'm not aware of any broken devices
that actually *have* an undefined Max Link Speed, so this patch is a
safety measure to avoid the GENMASK() inversion in patch [2/3].

Thanks,

Lukas

Re: [PATCH for-linus v2 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined

[Jonathan Cameron]

On Sun, 15 Dec 2024 11:20:51 +0100
Lukas Wunner <lukas@wunner.de> wrote:

> Broken PCIe devices may not set any of the bits in the Link Capabilities
> Register's "Max Link Speed" field.  Assume 2.5 GT/s in such a case,
> which is the lowest possible PCIe speed.  It must be supported by every
> device per PCIe r6.2 sec 8.2.1.
> 
> Emit a message informing about the malformed field.  Use KERN_INFO
> severity to minimize annoyance.  This will help silicon validation
> engineers take note of the issue so that regular users hopefully never
> see it.
> 
> There is currently no known affected product, but a subsequent commit
> will honor the Max Link Speed field when determining supported speeds
> and depends on the field being well-formed.  (It uses the Max Link Speed
> as highest bit in a GENMASK(highest, lowest) macro and if the field is
> zero, that would result in GENMASK(0, lowest).)
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Seems like this is the best we can do for this (hopefully)
theoretical hardware bug.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/pci/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 35dc9f249b86..ab0ef7b6c798 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  	u32 lnkcap2, lnkcap;
>  	u8 speeds;
>  
> +	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
> +	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> +	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
> +		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");
> +		return PCI_EXP_LNKCAP2_SLS_2_5GB;
> +	}
> +
>  	/*
>  	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
>  	 * Speeds Vector to allow using SLS Vector bit defines directly.
> @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  	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;

Re: [PATCH for-linus v2 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined

[Ilpo Järvinen]

On Sun, 15 Dec 2024, Lukas Wunner wrote:

> Broken PCIe devices may not set any of the bits in the Link Capabilities
> Register's "Max Link Speed" field.  Assume 2.5 GT/s in such a case,
> which is the lowest possible PCIe speed.  It must be supported by every
> device per PCIe r6.2 sec 8.2.1.
> 
> Emit a message informing about the malformed field.  Use KERN_INFO
> severity to minimize annoyance.  This will help silicon validation
> engineers take note of the issue so that regular users hopefully never
> see it.
> 
> There is currently no known affected product, but a subsequent commit
> will honor the Max Link Speed field when determining supported speeds
> and depends on the field being well-formed.  (It uses the Max Link Speed
> as highest bit in a GENMASK(highest, lowest) macro and if the field is
> zero, that would result in GENMASK(0, lowest).)
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 35dc9f249b86..ab0ef7b6c798 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  	u32 lnkcap2, lnkcap;
>  	u8 speeds;
>  
> +	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
> +	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> +	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
> +		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");
> +		return PCI_EXP_LNKCAP2_SLS_2_5GB;
> +	}

After some more thinking, I realized this is probably not a good idea (I 
know you got it from me :-(). IIRC, this function is also called for 
RCiEPs and they do not have to implement these registers. I saw 
supported_speeds were 0 for many devices, for most to be more precise,
while developing bwctrl (none of those impacted bwctrl).

If I'm correct, that print out should trigger many times on a simple boot 
test so it should be easy to confirm.

-- 
 i.

> +
>  	/*
>  	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
>  	 * Speeds Vector to allow using SLS Vector bit defines directly.
> @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>  	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;
> 

Re: [PATCH for-linus v2 1/3] PCI: Assume 2.5 GT/s if Max Link Speed is undefined

[Mario Limonciello]

On 12/15/2024 04:20, Lukas Wunner wrote:
> Broken PCIe devices may not set any of the bits in the Link Capabilities
> Register's "Max Link Speed" field.  Assume 2.5 GT/s in such a case,
> which is the lowest possible PCIe speed.  It must be supported by every
> device per PCIe r6.2 sec 8.2.1.
> 
> Emit a message informing about the malformed field.  Use KERN_INFO
> severity to minimize annoyance.  This will help silicon validation
> engineers take note of the issue so that regular users hopefully never
> see it.
> 
> There is currently no known affected product, but a subsequent commit
> will honor the Max Link Speed field when determining supported speeds
> and depends on the field being well-formed.  (It uses the Max Link Speed
> as highest bit in a GENMASK(highest, lowest) macro and if the field is
> zero, that would result in GENMASK(0, lowest).)
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>   drivers/pci/pci.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 35dc9f249b86..ab0ef7b6c798 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6233,6 +6233,13 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>   	u32 lnkcap2, lnkcap;
>   	u8 speeds;
>   
> +	/* A device must support 2.5 GT/s (PCIe r6.2 sec 8.2.1) */
> +	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> +	if (!(lnkcap & PCI_EXP_LNKCAP_SLS)) {
> +		pci_info(dev, "Undefined Max Link Speed; assume 2.5 GT/s\n");

As it's just theoretical, shouldn't it be noisier?  I'm thinking at 
least pci_warn().  Otherwise if this goes in and stays at pci_info() 
it's going to be a lot easier to miss.

Whereas at least messages that are warn or err get a more thorough look 
at during hardware bring up.

> +		return PCI_EXP_LNKCAP2_SLS_2_5GB;
> +	}
> +
>   	/*
>   	 * Speeds retain the reserved 0 at LSB before PCIe Supported Link
>   	 * Speeds Vector to allow using SLS Vector bit defines directly.
> @@ -6244,8 +6251,6 @@ u8 pcie_get_supported_speeds(struct pci_dev *dev)
>   	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;

[PATCH for-linus v2 2/3] PCI: Honor Max Link Speed when determining supported speeds

[Lukas Wunner]

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@kernel.org>
Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@kernel.org/
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 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);
+
 	/* PCIe r3.0-compliant */
 	if (speeds)
 		return speeds;
-- 
2.43.0

Re: [PATCH for-linus v2 2/3] PCI: Honor Max Link Speed when determining supported speeds

[Niklas Schnelle]

On Sun, 2024-12-15 at 11:20 +0100, 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".

Can confirm that I saw this effect on my affected Intel JHL7540 "Titan
Ridge 2018" Thunderbolt controller.

Feel free to add:

Tested-by: Niklas Schnelle <niks@kernel.org>

Re: [PATCH for-linus v2 2/3] PCI: Honor Max Link Speed when determining supported speeds

[Jonathan Cameron]

On Sun, 15 Dec 2024 11:20:52 +0100
Lukas Wunner <lukas@wunner.de> 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@kernel.org>
> Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@kernel.org/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Makes sense as a hardening step, even without the oddity of thunderbolt.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH for-linus v2 2/3] PCI: Honor Max Link Speed when determining supported speeds

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

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@kernel.org>
> Closes: https://lore.kernel.org/r/70829798889c6d779ca0f6cd3260a765780d1369.camel@kernel.org/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  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.

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

[Lukas Wunner]

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

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

[Niklas Schnelle]

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>

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

[Jonathan Cameron]

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

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

[Mario Limonciello]

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