Re: [PATCH 02/12] PCI/ACPI: Add PERST# Assertion Delay _DSM method

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Apr 09, 2025 at 02:30:31PM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 8, 2025 at 10:48 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Apr 02, 2025 at 09:36:01PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Apr 2, 2025 at 8:48 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > > > I don't *expect* rev 5 to be different.  But as a user of it,
> > > > why would I change working, tested code that is not broken?
> > > >
> > > > The PCI DPC function 0xc is an example where rev 5 (per ECN)
> > > > and rev 6 (per r3.3) are not compatible.
> > > >
> > > > If the OS implemented rev 5, then learns via function 0 that
> > > > function 0xc is also supported at rev 6, and starts using the
> > > > same OS code with rev 6, the OS is broken.
> > >
> > > Yes, in this case the backward compatibility language in the
> > > _DSM definition is obviously not followed.
> >
> > Rev 5 in the ECN isn't compatible with rev 6 in the PCI FW r3.3
> > spec, so it doesn't follow the ACPI compatibility requirement.
> > And this is documented in PCI FW, which says "Fn 0xC was added
> > with rev 5 (see ECN for rev 5 details); here is how rev 6 works."
> >
> > An OS implemented to the ECN doesn't know that rev 6 is different
> > from rev 5; it assumes they're the same because ACPI says we can
> > assume that, and PCI FW r3.3 even says the OS should use the same
> > rev for all functions.
> 
> OK (and this is important because PCI FW r3.3 is the spec defining
> the interface)
> 
> > If OS adds support for rev 6 of a some other function, it is
> > supposed to use rev 6 of Fn 0xC, which doesn't work as the OS
> > expects.
> 
> IMV with respect to _DSM, the spec that has defined the interface
> (PCI FW r3.3) takes precedence over the ACPI spec regardless of what
> the latter is saying.  In this case ACPI provides a framework the
> interface can be based on, but the actual interface is not defined
> by it.
> 
> Also, I think that the OS should use rev 6 if it is supported by the
> firmware (for all functions) and it should literally follow the
> definition of rev 6.

I think you interpret rev 6 as a global revision of the platform,
i.e., the platform supports rev 6 for every function it implements in
this UUID (which is clearly the intent of the ACPI ASL example).

I suggest that it would be better to interpret the revision
individually for each function because it removes the backwards
compatibility assumption and reduces the testing burden.

Most functions would be specified and implemented with rev 0, and
would never have a rev 1.  There would only be a rev 1 of a function
if we made a non backwards compatible change to it.

Any other functions would be untouched, and they would still only
support rev 0, not rev 1.

> > I guess one could argue that "OS didn't add rev 6 support for
> > anything until PCI FW r3.3 added a function at rev 6, r3.3 did
> > mention the difference between Fn 0xC rev 5 and 6, and OS should
> > have looked at all the already-implemented unrelated functions for
> > possible changes between rev 5 and rev 6."
> 
> Yes, it should.

I don't think it's reasonable to require the person adding support for
Fn 0xE rev 6 (TLP Processing Hints) to go back and add Fn 0xC rev 6
(Downstream Port Containment) at the same time.

Assuming that "rev X works the same as rev X-1 and therefore rev X
doesn't need to be tested" seems unwise to me.  But even if we
normally rely on that assumption, in this case Fn 0xC rev 5 and rev 6
are different, so we'd be adding new code that would require testing
on every platform that supports rev 6 of any function.

> What if the functions on the firmware side depend on each other
> interfally and the firmware gets confused if revisions are mixed up
> on the OS side?

In such a case, the backwards compatibility assumption doesn't apply
to those functions, so the spec would have to document multiple
revisions of them, and IMO that's the natural place to document
a requirement to use the same revision for the set.

Bjorn