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

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

[Zhou Shengqing]

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.

v2:add Fixes tag.

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

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

[Rafael J. Wysocki]

The period at the end of the subject doesn't match the common practice.

On Thu, Nov 14, 2024 at 4:05 AM Zhou Shengqing
<zhoushengqing@ttyinfo.com> 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.
>
> v2:add Fixes tag.
>
> Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()")
> Origin fixes: a78cf9657ba5 ("PCI/ACPI: Evaluate PCI Boot Configuration _DSM")

What does this mean?

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

This looks like a genuine fix, but I think it is PCI material.

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

[Bjorn Helgaas]

[+cc Ben, original author of a78cf9657ba5]

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.

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

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

[Rafael J. Wysocki]

On Tue, Dec 10, 2024 at 8:50 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Ben, original author of a78cf9657ba5]
>
> 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?

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

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

[Bjorn Helgaas]

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