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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

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.