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

Lukas Wunner
2024-01-01T16:40:19Z

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.

Lukas