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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Apr 02, 2025 at 04:52:55PM +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 2, 2025 at 4:21 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Apr 02, 2025 at 01:06:42PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Apr 1, 2025 at 5:36 PM Anshuman Gupta <anshuman.gupta@intel.com> wrote:
> > > >
> > > > Implement _DSM Method 11 as per PCI firmware specs
> > > > section 4.6.11 Rev 3.3.
> >
> > > > +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
> > > > +{
> > > > +       union acpi_object in_obj = {
> > > > +               .integer.type = ACPI_TYPE_INTEGER,
> > > > +               .integer.value = delay_us,
> > > > +       };
> > > > +
> > > > +       union acpi_object *out_obj;
> > > > +       acpi_handle handle;
> > > > +       int result, ret = -EINVAL;
> > > > +
> > > > +       if (!dev || !ACPI_HANDLE(&dev->dev))
> > > > +               return -EINVAL;
> > > > +
> > > > +       handle = ACPI_HANDLE(&dev->dev);
> > > > +
> > > > +       out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
> > >
> > > This is something I haven't noticed in the previous patch, but also
> > > applies to it.
> > >
> > > Why is rev 4 of the interface hard-coded here?
> >
> > Thanks for asking this because it's related to the whole _DSM revision
> > question that I don't understand.
> >
> > If we didn't use rev 4 here, what should we use?  The PCI Firmware
> > spec, r3.3, sec 4.6.11, documents this interface and says "lowest
> > valid Revision ID value is 4", so that's the source of the 4.
> 
> Well, the "lowest valid Revision ID" does not generally mean the "only
> valid Revision ID".

True, but why should the kernel change from using the tested revision
ID to some other revision ID?  What would be the benefit of using rev
5?

PCI Firmware r3.3 does contain a statement that "OSPM must invoke all
Functions other than Function 0 with the same Revision ID value," but
IMO that was a mistake, and we just approved an ECR to remove it.

> > My argument is that the spec documents rev 4, the kernel code was
> > tested with rev 4, so what would be the benefit of using a different
> > revision here?
> 
> I'm talking about using a symbol to represent the number 4, not about
> possibly using a different number, along the lines of using, say,
> ACPI_FADT_LOW_POWER_S0 instead of putting BIT(21) directly into the
> code.
>
> The value is not likely to change, but using a symbol for representing
> it has merit (it can be meaningfully used in searches, it can be
> documented etc.).

DSM_PCI_PERST_ASSERTION_DELAY (the function number) is a great thing
to search for.  I doubt a symbol for "4" would really add anything.
At least in PCI, changes to a particular _DSM function are relatively
rare.

> Now, I'm not sure how likely it is for the PCI Firmware spec to bump
> up the revision of this interface (I suppose that it will do so if a
> new function is defined), but even if it does so, the kernel will have
> to check both the new revision and rev 4 anyway, in case the firmware
> doesn't know about the new revision.

I guess the reason the OS would check both rev 5 and rev 4 would be
that a new platform might implement DSM_PCI_PERST_ASSERTION_DELAY only
at rev 5?  I think this would be a real mistake in terms of
implementing something to the spec.  

The spec documents DSM_PCI_PERST_ASSERTION_DELAY rev 4.  Some
platforms implemented and tested DSM_PCI_PERST_ASSERTION_DELAY rev 4.
Linux added and tested support for DSM_PCI_PERST_ASSERTION_DELAY rev
4.  I propose new platforms should also implement and test
DSM_PCI_PERST_ASSERTION_DELAY rev 4.

If a new platform implements only DSM_PCI_PERST_ASSERTION_DELAY rev 5,
there is no actual documentation for that rev other than the spec
assertion that "rev 5 must support all functions defined by previous
revs of the UUID."  IMO this is nonsense.  The platform might have no
need to implement all the functions defined for the UUID.

Even if the platform makes all its functions valid for "the lowest
valid Revision ID" through the "current Revision ID", there's nothing
other than the spec assertion to guarantee that they all behave the
same.  And dealing with multiple revisions that are "supposed" to be
the same just makes work and risk for the OS.

Bjorn