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

[Niklas Schnelle <niks@kernel.org>]

On Thu, 2024-12-12 at 16:33 +0200, Ilpo Järvinen wrote:
> On Thu, 12 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.
> > 
> > Niklas reports that the Intel JHL7540 "Titan Ridge 2018" Thunderbolt
> > controller supports 2.5-8 GT/s speeds, but indicates 2.5 GT/s as maximum.
> > Ilpo recalls seeing this inconsistency on more devices.
> > 
> > pcie_get_supported_speeds() neglects to honor the Max Link Speed field
> > and will thus incorrectly deem higher speeds as supported.  Fix it.
> > 
> > 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 | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 35dc9f2..b730560 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6240,12 +6240,14 @@ 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 */
> > +	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
> > +	speeds &= GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, 0);
> 
> Hi Lukas,
> 
> Why do you start GENMASK() from 0th position? That's the reserved bit.
> (I doesn't exactly cause a misbehavior to & the never set 0th bit but
> it is slightly confusing).
> 
> I suggest to get that either from PCI_EXP_LNKCAP2_SLS_2_5GB or 
> PCI_EXP_LNKCAP2_SLS (e.g. with __ffs()) and do not use literal at all
> to make it explicit where it originates from.
> 

Hi Ilpo,

I agree this is quite confusing even in the PCIe spec. According to
PCIe r6.2 the value of the Max Link Speed field references a bit in the
Supported Link Speeds Vector with a value of 0b0001 in Max Link Speeds
referring to bit 0 in Supported Link Speeds, a value of 0xb0010 to bit
1 and so on. So the value is actually shiftet left by 1 versus the
typical (i.e. non IBM ;-)) bit counting.

Then looking at the Supported Link Speeds description they refer to bit
0 as 2.5 GT/s, bit 1 as 5 GT/s up to bit 6 (RsvdP) while in the figure
the RsvdP is the right most bit. So unless I have completely confused
myself playing around with this genmask calculator[0] we actually want
GENMASK(lnkcap & PCI_EXP_LNKCAP_SLS, 1) because just like the Supported
Link Speeds field the dev->supported_speeds also reserves the LSB. And
I feel this actually matches the spec wording pretty well. What do you
think?

Thanks,
Niklas

[0] https://mazdermind.de/genmask/