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

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Mar 01, 2025 at 03:28:22AM +0000, Zhou Shengqing wrote:
> On Thu, Feb 27, 2025 at 6:40 PM Bjorn Helgaas wrote:
> > On Mon, Dec 16, 2024 at 05:27:51AM +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 changed to 2.
> > > But the code remains unchanged, still 1.
> > > 
> > > v4:Initialize *obj to NULL.
> > > v3:try revision id 1 first, then try revision id 2.
> > > v2:add Fixes tag.
> > 
> > Thanks for working on this issue.
> > 
> >   - Thanks for the revision history.  For future posts, put it below
> >     the "---" line so it's in the email but not part of the commit log.
> > 
> >   - I think there's a leak in pci_acpi_preserve_config() because it
> >     doesn't free "obj" before it returns true.  If you agree, add a
> >     preparatory patch to fix this.
> > 
> >   - Add a preparatory patch to return false early in
> >     pci_acpi_preserve_config() if !ACPI_HANDLE(&host_bridge->dev) so
> >     the body of the function is unindented, similar to what
> >     acpi_pci_add_bus() does.
> > 
> >   - Add another preparatory patch that adds acpi_check_dsm() of the
> >     desired function/rev ID for DSM_PCI_PRESERVE_BOOT_CONFIG,
> >     DSM_PCI_POWER_ON_RESET_DELAY, DSM_PCI_DEVICE_READINESS_DURATIONS.
> >     Move the "Evaluate PCI Boot Configuration" comment above the
> >     acpi_check_dsm() since it applies to the whole function, not just
> >     the rev 1 code in this patch.
> > 
> >   - Rework this patch so it only adds acpi_check_dsm() and
> >     acpi_evaluate_dsm_typed() for rev 2.
> 
> Could you please explain this in more detail? Do you mean we don't need to
> consider rev 1 anymore?

No, I think we still need to handle platforms that implemented PCI
Firmware spec r3.2 and used rev 1.

Platforms that implemented spec r3.3 probably use rev 2, and we don't
know whether they support rev 1.

So I think the ultimate function would look something like this:

  bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge)
  {
      bool rc = false;

      if (!ACPI_HANDLE(&host_bridge->dev))
	  return false;

      if (acpi_check_dsm(..., 1, BIT(DSM_PCI_PRESERVE_BOOT_CONFIG))) {
	  obj = acpi_evaluate_dsm_typed(..., 1, DSM_PCI_PRESERVE_BOOT_CONFIG, ...);
	  if (obj && obj->integer.value == 0)
	      rc = true;
	  ACPI_FREE(obj);
      } else if (acpi_check_dsm(..., 2, BIT(DSM_PCI_PRESERVE_BOOT_CONFIG))) {
	  obj = acpi_evaluate_dsm_typed(..., 2, DSM_PCI_PRESERVE_BOOT_CONFIG, ...);
	  if (obj && obj->integer.value == 0)
	      rc = true;
	    ACPI_FREE(obj);
      }

      return rc;
  }