| Message ID | 20241216052751.5460-1-zhoushengqing@ttyinfo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv4] PCI/ACPI: _DSM PRESERVE_BOOT_CONFIG function rev id doesn't match with spec | expand |
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. - Throughout, wrap code and comments to fit in 80 columns because that's the convention for the rest of the file. - You can use "BIT(DSM_PCI_PRESERVE_BOOT_CONFIG)" and similar instead of "1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG" when calling acpi_check_dsm(). - Add a [0/n] cover letter when posting the series. Each patch should be a response to the cover letter. "git format-patch" and "git send-email" will do that for you automatically. > Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()") > > Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com> > --- > drivers/pci/pci-acpi.c | 42 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 10 deletions(-) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index af370628e583..f805cd134019 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -123,19 +123,41 @@ phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle) > bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) > { > if (ACPI_HANDLE(&host_bridge->dev)) { > - union acpi_object *obj; > + union acpi_object *obj = NULL; > > /* > - * Evaluate the "PCI Boot Configuration" _DSM Function. If it > - * exists and returns 0, we must preserve any PCI resource > - * assignments made by firmware for this host bridge. > + * Per PCI Firmware r3.2, released Jan 26, 2015, > + * DSM_PCI_PRESERVE_BOOT_CONFIG Revision ID is 1. > + * But PCI Firmware r3.3, released Jan 20, 2021, > + * changed sec 4.6.5 to say > + * "lowest valid Revision ID value: 2". So try revision 1 > + * first for old platform, then try revision 2. > */ > - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > - &pci_acpi_dsm_guid, > - 1, DSM_PCI_PRESERVE_BOOT_CONFIG, > - NULL, ACPI_TYPE_INTEGER); > - if (obj && obj->integer.value == 0) > - return true; > + if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > + 1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) { > + /* > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > + * exists and returns 0, we must preserve any PCI resource > + * assignments made by firmware for this host bridge. > + */ > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > + &pci_acpi_dsm_guid, > + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, > + NULL, ACPI_TYPE_INTEGER); > + if (obj && obj->integer.value == 0) > + return true; > + } > + > + if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 2, > + 1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) { > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > + &pci_acpi_dsm_guid, > + 2, DSM_PCI_PRESERVE_BOOT_CONFIG, > + NULL, ACPI_TYPE_INTEGER); > + if (obj && obj->integer.value == 0) > + return true; > + } > + > ACPI_FREE(obj); > } > > -- > 2.39.2 >
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? > > - Throughout, wrap code and comments to fit in 80 columns because > that's the convention for the rest of the file. > > - You can use "BIT(DSM_PCI_PRESERVE_BOOT_CONFIG)" and similar > instead of "1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG" when calling > acpi_check_dsm(). > > - Add a [0/n] cover letter when posting the series. Each patch > should be a response to the cover letter. "git format-patch" and > "git send-email" will do that for you automatically. > > > Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()") > > > > Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com> > > --- > > drivers/pci/pci-acpi.c | 42 ++++++++++++++++++++++++++++++++---------- > > 1 file changed, 32 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > > index af370628e583..f805cd134019 100644 > > --- a/drivers/pci/pci-acpi.c > > +++ b/drivers/pci/pci-acpi.c > > @@ -123,19 +123,41 @@ phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle) > > bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) > > { > > if (ACPI_HANDLE(&host_bridge->dev)) { > > - union acpi_object *obj; > > + union acpi_object *obj = NULL; > > > > /* > > - * Evaluate the "PCI Boot Configuration" _DSM Function. If it > > - * exists and returns 0, we must preserve any PCI resource > > - * assignments made by firmware for this host bridge. > > + * Per PCI Firmware r3.2, released Jan 26, 2015, > > + * DSM_PCI_PRESERVE_BOOT_CONFIG Revision ID is 1. > > + * But PCI Firmware r3.3, released Jan 20, 2021, > > + * changed sec 4.6.5 to say > > + * "lowest valid Revision ID value: 2". So try revision 1 > > + * first for old platform, then try revision 2. > > */ > > - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > > - &pci_acpi_dsm_guid, > > - 1, DSM_PCI_PRESERVE_BOOT_CONFIG, > > - NULL, ACPI_TYPE_INTEGER); > > - if (obj && obj->integer.value == 0) > > - return true; > > + if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, > > + 1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) { > > + /* > > + * Evaluate the "PCI Boot Configuration" _DSM Function. If it > > + * exists and returns 0, we must preserve any PCI resource > > + * assignments made by firmware for this host bridge. > > + */ > > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > > + &pci_acpi_dsm_guid, > > + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, > > + NULL, ACPI_TYPE_INTEGER); > > + if (obj && obj->integer.value == 0) > > + return true; > > + } > > + > > + if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 2, > > + 1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) { > > + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), > > + &pci_acpi_dsm_guid, > > + 2, DSM_PCI_PRESERVE_BOOT_CONFIG, > > + NULL, ACPI_TYPE_INTEGER); > > + if (obj && obj->integer.value == 0) > > + return true; > > + } > > + > > ACPI_FREE(obj); > > } > > > > -- > > 2.39.2 > >
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; }
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index af370628e583..f805cd134019 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -123,19 +123,41 @@ phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle) bool pci_acpi_preserve_config(struct pci_host_bridge *host_bridge) { if (ACPI_HANDLE(&host_bridge->dev)) { - union acpi_object *obj; + union acpi_object *obj = NULL; /* - * Evaluate the "PCI Boot Configuration" _DSM Function. If it - * exists and returns 0, we must preserve any PCI resource - * assignments made by firmware for this host bridge. + * Per PCI Firmware r3.2, released Jan 26, 2015, + * DSM_PCI_PRESERVE_BOOT_CONFIG Revision ID is 1. + * But PCI Firmware r3.3, released Jan 20, 2021, + * changed sec 4.6.5 to say + * "lowest valid Revision ID value: 2". So try revision 1 + * first for old platform, then try revision 2. */ - obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), - &pci_acpi_dsm_guid, - 1, DSM_PCI_PRESERVE_BOOT_CONFIG, - NULL, ACPI_TYPE_INTEGER); - if (obj && obj->integer.value == 0) - return true; + if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1, + 1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) { + /* + * Evaluate the "PCI Boot Configuration" _DSM Function. If it + * exists and returns 0, we must preserve any PCI resource + * assignments made by firmware for this host bridge. + */ + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), + &pci_acpi_dsm_guid, + 1, DSM_PCI_PRESERVE_BOOT_CONFIG, + NULL, ACPI_TYPE_INTEGER); + if (obj && obj->integer.value == 0) + return true; + } + + if (acpi_check_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 2, + 1ULL << DSM_PCI_PRESERVE_BOOT_CONFIG)) { + obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), + &pci_acpi_dsm_guid, + 2, DSM_PCI_PRESERVE_BOOT_CONFIG, + NULL, ACPI_TYPE_INTEGER); + if (obj && obj->integer.value == 0) + return true; + } + ACPI_FREE(obj); }
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. Fixes: 9d7d5db8e78e ("PCI: Move PRESERVE_BOOT_CONFIG _DSM evaluation to pci_register_host_bridge()") Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com> --- drivers/pci/pci-acpi.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)