On Thu, 12 Dec 2024, Lukas Wunner wrote: > 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. Fair but that's quite many thoughts you didn't record into the commit message. If you intentionally do confusing tricks like that, please be mindful about the poor guy who has to figure this out years from now. :-) (One of the most annoying thing in digging into commits far in the past are those nagging "Why?" questions nobody is there to answer.) I know it doesn't misbehave because of bit 0 is cleared by the earlier statement. (I also thought of the GENMASK() inversion.) Another option would be to explicitly ensure PCI_EXP_LNKCAP_SLS is at least PCI_EXP_LNKCAP2_SLS_2_5GB which would also ensure pre-3.0 part returns always at least one speed (which the current code doesn't guarantee). As in more broader terms there are other kinds of broken devices this code doesn't handle. If PCI_EXP_LNKCAP2_SLS is empty of bits but the device has >5GT/s in PCI_EXP_LNKCAP_SLS, this function will return 0. > > 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. I meant that instead of GENMASK(..., 0) use GENMASK(..., __ffs(PCI_EXP_LNKCAP2_SLS)). But it doesn't matter if go with this bit 0 included into GENMASK() approach. -- i.
