Re: [PATCH v3 05/10] PCI: Store all PCIe Supported Link Speeds

[Ilpo Järvinen]

On Mon, 1 Jan 2024, Lukas Wunner wrote:

> On Mon, Jan 01, 2024 at 06:26:40PM +0200, Ilpo Järvinen wrote:
> > On Sat, 30 Dec 2023, Lukas Wunner wrote:
> > 
> > > On Sat, Dec 30, 2023 at 12:45:49PM +0100, Lukas Wunner wrote:
> > > > On Fri, Sep 29, 2023 at 02:57:18PM +0300, Ilpo Järvinen wrote:
> > > > > struct pci_bus stores max_bus_speed. Implementation Note in PCIe r6.0.1
> > > > > sec 7.5.3.18, however, recommends determining supported Link Speeds
> > > > > using the Supported Link Speeds Vector in the Link Capabilities 2
> > > > > Register (when available).
> > > > > 
> > > > > Add pcie_bus_speeds into struct pci_bus which caches the Supported Link
> > > > > Speeds. The value is taken directly from the Supported Link Speeds
> > > > > Vector or synthetized from the Max Link Speed in the Link Capabilities
> > > > > Register when the Link Capabilities 2 Register is not available.
> > > > 
> > > > Remind me, what's the reason again to cache this and why is
> > > > max_bus_speed not sufficient?  Is the point that there may be
> > > > "gaps" in the supported link speeds, i.e. not every bit below
> > > > the maximum supported speed may be set?  And you need to skip
> > > > over those gaps when throttling to a lower speed?
> > > 
> > > FWIW I went and re-read the internal review I provided on May 18.
> > > Turns out I already mentioned back then that gaps aren't permitted:
> > > 
> > >  "Per PCIe r6.0.1 sec 8.2.1, the bitfield in the Link Capabilities 2
> > >   register is not permitted to contain gaps between maximum supported
> > >   speed and lowest possible speed (2.5 GT/s Gen1)."
> > > 
> > > 
> > > > Also, I note that pci_set_bus_speed() doesn't use LNKCAP2.
> > > 
> > > About that, I wrote in May:
> > > 
> > >  "Actually, scratch that.  pci_set_bus_speed() is fine.  Since it's only
> > >   interested in the *maximum* link speed, reading just LnkCap is correct.
> > >   LnkCap2 only needs to be read to determine if a certain speed is
> > >   *supported*.  E.g., even though 32 GT/s are supported, perhaps 16 GT/s
> > >   are not.
> > > 
> > >   It's rather pcie_get_speed_cap() which should be changed.  There's
> > >   no need for it to read LnkCap2.  The commit which introduced this,
> > >   6cf57be0f78e, was misguided and had to be fixed up with f1f90e254e46.
> > >   It could be simplified to just read LnkCap and return
> > >   pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS].  If the device is a
> > >   Root Port or Downstream Port, it doesn't even have to do that but
> > >   could return the cached value in subordinate->max_bus_speed.
> > >   If you add another attribute to struct pci_bus for the downstream
> > >   device's maximum speed, the maximum speed for Endpoints and Upstream
> > >   Ports could be returned directly as well from that attribute."
> > 
> > I know it's quite far back so it's understandable to forget :-), 
> > but already by May 23rd your position had changed and you wrote this:
> > 
> > 'Per the Implementation Note at the end of PCIe r6.0.1 sec 7.5.3.18,
> > 
> >    "It is strongly encouraged that software primarily utilize the
> >     Supported Link Speeds Vector instead of the Max Link Speed field,
> >     so that software can determine the exact set of supported speeds on
> >     current and future hardware. This can avoid software being confused
> >     if a future specification defines Links that do not require support
> >     for all slower speeds."
> > 
> > This means that it's not sufficient if you just check that the desired 
> > speed is lower than the maximum.  Instead, you should check if the bit 
> > corresponding to the desired speed is set in the LnkCap2 register's 
> > Supported Link Speeds Vector.
> > 
> > PCIe r6.0.1 sec 8.2.1 stipulates that the bitfield is not permitted to 
> > contain gaps between maximum supported speed and lowest possible speed
> > (2.5 GT/s Gen1).  However the Implementation Note suggests that rule may 
> > no longer apply in future revisions of the PCIe Base Spec.'
> > 
> > So I'd assume I should still follow the way spec recommends, not the "old 
> > method" that may not function correctly after some future version of the 
> > spec, or have you really changed position once again on this?
> 
> I haven't, you're right, I forgot about all those details.
> Thanks for that blast from the past. ;)
>
> But it would be good to extend the commit message because without all
> that context, it's difficult to understand why the max_bus_speed isn't
> sufficient.

Thanks. I'll extend the commit message.

-- 
 i.