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

[Lukas Wunner <lukas@wunner.de>]

On Thu, Dec 12, 2024 at 04:33:23PM +0200, Ilpo Järvinen wrote:
> On Thu, 12 Dec 2024, Lukas Wunner wrote:
> > --- 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);
> 
> 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).

GENMASK() does a BUILD_BUG_ON(l > h) and if a broken PCIe device
does not set any bits in the PCI_EXP_LNKCAP_SLS field, I'd risk
doing a GENMASK(0, 1) here, fulfilling the l > h condition.

Granted, the BUILD_BUG_ON() only triggers if l and h can be
evaluated at compile time, which isn't the case here.

Still, I felt uneasy risking any potential future breakage.
Including the reserved bit 0 is harmless because it's forced to
zero by the earlier "speeds = lnkcap2 & PCI_EXP_LNKCAP2_SLS"
expression.


> 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.

Pardon me for being dense but I don't understand what you mean.

Thanks,

Lukas