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
