Re: [PATCHv2] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id doesn't match with spec.

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Dec 10, 2024 at 08:56:14PM +0100, Rafael J. Wysocki wrote:
> On Tue, Dec 10, 2024 at 8:50 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Nov 14, 2024 at 03:04:24AM +0000, Zhou Shengqing wrote:
> > > Per PCI Firmware Specification Revision 3.3 Table 4-7 _DSM Definitions
> > > for PCI. Preserve PCI Boot Configuration Initial Revision ID is 2. But
> > > the code is 1.
> >
> > This _DSM function 5 was added in PCI Firmware r3.1, released Dec 13,
> > 2010.  It's listed in sec 4.6 with Revision 2 (as *all* the defined
> > functions are, even functions 1-4, which were included in r3.0 with
> > Revision 1).
> >
> > But the actual definition that was added in r3.1 is in sec 4.6.5,
> > which specifies Revision ID 1.
> >
> > PCI Firmware r3.2, released Jan 26, 2015, was the newest available at
> > the time Ben implemented a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot
> > Configuration _DSM"), and sec 4.6.5 still specified Revision ID 1.
> >
> > So I think Ben's addition used the correct Revision ID (1).
> >
> > PCI Firmware r3.3, released Jan 20, 2021, changed sec 4.6.5 to say
> > "lowest valid Revision ID value: 2"
> >
> > I think it's a mistake to make the kernel change below because
> > platforms in the field implemented function 5 with revision 1 (per the
> > r3.1 and r3.2 specs), and we have no idea whether they implement
> > function 5 revision 2.
> >
> > It's quite likely that newer platforms following r3.3 will implement
> > function 5 revision 2, but NOT revision 1, and the existing code won't
> > work for them.
> >
> > I think the fix is to try revision 1 and, if that isn't implemented,
> > we should try revision 2.  The semantics stayed the same, so they
> > should both work the same.
> 
> Or call Function 0 with the new revision and check the result?

IIUC we should always be using acpi_check_dsm() to call function 0 and
check for the desired function and revision, so we should do that for
both revision 1 and revision 2.  It looks like we're missing that
check here, which is a separate problem.

Of the current pci_acpi_dsm_guid uses, these functions lack that check:

  pci_acpi_preserve_config
    acpi_evaluate_dsm_typed(DSM_PCI_PRESERVE_BOOT_CONFIG)

  acpi_pci_add_bus
    acpi_evaluate_dsm_typed(DSM_PCI_POWER_ON_RESET_DELAY)

  pci_acpi_optimize_delay
    acpi_evaluate_dsm_typed(DSM_PCI_DEVICE_READINESS_DURATIONS)

The only other PCI _DSM functions we use do include the check:

  EDR_PORT_DPC_ENABLE_DSM  acpi_enable_dpc()
  EDR_PORT_LOCATE_DSM      acpi_dpc_port_get()
  TPH_ST_DSM_FUNC_INDEX    tph_invoke_dsm()
  DSM_PCI_DEVICE_NAME      dsm_get_label() (check in device_has_acpi_name())

> > > Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()")
> > > Origin fixes: a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM")
> > >
> > > Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
> > > ---
> > >  drivers/pci/pci-acpi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index af370628e583..7a4cad0c1f00 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -132,7 +132,7 @@ bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge)
> > >                */
> > >               obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev),
> > >                                             &pci_acpi_dsm_guid,
> > > -                                           1, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > > +                                           2, DSM_PCI_PRESERVE_BOOT_CONFIG,
> > >                                             NULL, ACPI_TYPE_INTEGER);
> > >               if (obj && obj->integer.value == 0)
> > >                       return true;
> > > --
> > > 2.39.2
> > >