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

[Niklas Schnelle <niks@kernel.org>]

On Fri, 2024-12-13 at 18:41 +0100, Niklas Schnelle wrote:
> 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);
> > > > 
> > 
> 
> ---8<---
> 
> > 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.
> 
> On second look I don't think it will. If lnkcap2 & PCI_EXP_LNKCAP2_SLS
> is 0 it will proceed to the synthesize part and rely on
> PCI_EXP_LNKCAP_SLS alone. The potentially broken part I see is when
> lnkcap2 has bits set but lnkcap doesn't which is also when the
> GENMASK(…, 1) would become weird. Not sure what the right handling for
> that is though.

Never mind, I missed the >5 GT/s bit. Though I think that returning 0
speeds is probably the safest bet for a broken device, no?