On Fri, 2024-12-13 at 12:12 +0200, Ilpo Järvinen wrote: > 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. > So that's essentially GENMASK(…, 1). The reason I'm not sure if it really is more self documenting is that anyone trying to grok this is going to look in the PCIe spec and that already shows the bit 0 as reserved (if it still is reserved then) and the bit numbering explaining the offset and the __ffs() is just going to add confusion. Instead I prefer your other idea of GENMASK(…, PCI_EXP_LNKCAP2_SLS_2_5GB) because together with that constant's comment it clearly communicates "starting at LNKCAP2 SLS Vector bit 0". For the issue of GENMASK() not being well defined for l == 0, I think the code should handle lnkcap2 == 0 explizitly which is basically your point above about other broken devices. Thanks, Niklas
