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
