Re: [PATCH v3 06/10] PCI: Cache PCIe device's Supported Speed Vector

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3096 bytes --]

On Sat, 30 Dec 2023, Lukas Wunner wrote:

> On Fri, Sep 29, 2023 at 02:57:19PM +0300, Ilpo Järvinen wrote:
> > The Supported Link Speeds Vector in the Link Capabilities Register 2
> > corresponds to the bus below on Root Ports and Downstream Ports,
> > whereas it corresponds to the bus above on Upstream Ports and
> > Endpoints.
> 
> It would be good to add a pointer to the spec here.  I think the
> relevant section is PCIe r6.1 sec 7.5.3.18 which says:
> 
>  "Supported Link Speeds Vector - This field indicates the supported
>   Link speed(s) of the associated Port."
>                        ^^^^^^^^^^^^^^^
> 
> Obviously the associated port is upstream on a Switch Upstream Port
> or Endpoint, whereas it is downstream on a Switch Downstream Port
> or Root Port.
> 
> Come to think of it, what about edge cases such as RCiEPs?

On real HW I've seen, RCiEPs don't seem to have these speeds at all 
(PCIe r6.1, sec 7.5.3):

"The Link Capabilities, Link Status, and Link Control registers are 
required for all Root Ports, Switch Ports, Bridges, and Endpoints that are 
not RCiEPs. For Functions that do not implement the Link Capabilities, 
Link Status, and Link Contro registers, these spaces must be hardwired to 
0. Link Capabilities 2, Link Status 2, and Link Control 2 registers are
required for all Root Ports, Switch Ports, Bridges, and Endpoints (except 
for RCiEPs) that implement capabilities requiring those registers. For 
Functions that do not implement the Link Capabilities 2, Link Status 2, 
and Link Control 2 registers, these spaces must be hardwired to 0b."

> > Only the former is currently cached in pcie_bus_speeds in
> > the struct pci_bus. The link speeds that are supported is the
> > intersection of these two.
> 
> I'm wondering if caching both is actually necessary.  Why not cache
> just the intersection?  Do we need either of the two somewhere?

Intersection is enough at least for bwctrl. The only downside that is 
barely worth mentioning is that the bus SLSV has to be re-read when
function 0 sets the intersection.

I can think of somebody wanting to expose the list of both supported speed 
to userspace though sysfs (not done by this patch series), but they could 
be read from the registers in that case so that use case doesn't really 
matter much, IMO.

> > Store the device's Supported Link Speeds Vector into the struct pci_bus
> > when the Function 0 is enumerated (the Multi-Function Devices must have
> > same speeds the same for all Functions) to be easily able to calculate
> > the intersection of Supported Link Speeds.
> 
> Might want to add an explanation what you're going to need this for,
> I assume it's accessed frequently by the bandwidth throttling driver
> in a subsequent patch?

Yes. I tend to try to avoid forward references because some maintainers 
complain about them (leading to minimal changes where true motivations 
have to be hidden because "future" cannot be used to motivate a change 
even if that's often the truest motivation within a patch series). But 
I'll add a fwd ref here to make it more obvious. :-)

-- 
 i.