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

[Niklas Schnelle <niks@kernel.org>]

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)