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

[Lukas Wunner <lukas@wunner.de>]

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