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

[Lukas Wunner <lukas@wunner.de>]

On Mon, Jan 01, 2024 at 08:31:39PM +0200, Ilpo Järvinen wrote:
> On Sat, 30 Dec 2023, Lukas Wunner wrote:
> > On Fri, Sep 29, 2023 at 02:57:19PM +0300, Ilpo Järvinen wrote:
> > > 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.

Yes, that would be a reasonable argument to keep both values instead
of storing just the intersection.


> > > 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. :-)

Bjorn has used phrases such as "We're about to ..." a couple of times
in commit messages to convey that a particular change in the present
patch will be taken advantage of by a subsequent patch.

I've used the same phrase but got criticized (in other subsystems)
for using "we".

So I use phrases such as:

 "An upcoming commit will create DOE mailboxes upon device enumeration by
  the PCI core.  Their lifetime shall not be limited by a driver.
  Therefore rework..." (see 022b66f38195)

Can also reference the past:

 "The PCI core has just been amended to create a pci_doe_mb struct for
  every DOE instance on device enumeration.  [...]  That leaves [...]
  without any callers, so drop them." (see 74e491e5d1bc)

If someone finds your commit e.g. through git blame, it may help them
enormously if you provide context in the commit message.  If maintainers
in other subsystem tell you otherwise, they're wrong. ;)

Thanks,

Lukas