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