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

[Niklas Schnelle <niks@kernel.org>]

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.