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

[Rafael J. Wysocki]

On Wed, Apr 9, 2025 at 4:47 PM Bjorn Helgaas <helgaas at kernel.org> wrote:
>
> 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 at 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 at 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).

Yes, I do.

> 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 think that it would just be confusing because both the OS and the
firmware would now have to remember which function is defined at which
revision.

Also the industry practice in this respect has been different so far, AFAICS.

And PCI FW r3.3 wants the OS to use the same rev for all functions
which doesn't leave too much room for interpretation.

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

Well, if you know that the function is supposed to work the same in
the new rev, it only is a matter of changing the rev passed to
acpi_evaluate_dsm*().

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

So say you've only changed the rev passed to acpi_evaluate_dsm*() as
per the above.  The only reason why it may not work is when there is a
bug in the firmware.

Now, suppose that you don't change anything and stick to rev X-1, but
there is a new version of the firmware that contains a bug in the
given _DSM implementation.  Same thing.

I get the "only the tested code is trustable" argument, but its
applicability here is naturally limited.

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

Unfortunately, as far as OS-firmware interfaces are concerned, you
need to trust the other end to do the right thing because you cannot
test all of the possible combinations.

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

I'm not talking about the definition of the interface, but about
dependencies at the implementation level.

Given the PCI FW r3.3 provisions, firmware may assume that the OS will
use the same rev for all functions and it may depend on that.