| Message ID | 20241114030424.45074-1-zhoushengqing@ttyinfo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id doesn't match with spec. | expand |
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.
[+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 >
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 > >
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 > > >
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;
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(-)