[+cc Dan]
On Tue, Jun 17, 2025 at 10:37:38AM +0800, Zhe Qiao wrote:
> The patch "PCI/ACPI: Fix allocated memory release on error in
> pci_acpi_scan_root()" introduces a dual release issue. When
> acpi_pci_root_creat() fails, the pci_cpi_can_root() function
> will release 'ri ->cfg' and 'root_ops' in the error handling
> path.However, acpi_pci_root_creat() will also call
> __acpi_pci_root_release_info(), which in turn will call the
> release_info hook, causing the same block of memory to be
> released again.
These are all nits, but would have to be fixed before applying:
- 'The patch "PCI/ACPI: Fix ..."' is not the usual way to identify a
commit. Use the same style as in the Fixes: tag below.
- Typo in "acpi_pci_root_creat" (twice)
- Typo in "pci_cpi_can_root"
- Add space after the period in "path.However, ..."
- Add "Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>" and
"Closes: https://lore.kernel.org/all/aEmdnuw715btq7Q5@stanley.mountain/";
and cc: Dan.
- 631b2af2f357 appeared in v6.16-rc1, so we should try to get the
fix into v6.16. A hint after the "---" would be helpful to make
sure that happens.
Wait a few days before reposting in case other folks have comments.
> Fixes: 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()")
> Signed-off-by: Zhe Qiao <qiaozhe@xxxxxxxxxxx>
> ---
> v1 -> v2:
> - Restore all changes from the first version.
> - Remove unnecessary release info hooks.
> - Add a NULL check before calling info->ops->release_info().
> - Delete the currently unused pci_api_geneic_delease_info () function.
> ---
> drivers/acpi/pci_root.c | 3 ++-
> drivers/pci/pci-acpi.c | 12 ------------
> 2 files changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 74ade4160314..83628adbc56b 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -974,7 +974,8 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
> resource_list_destroy_entry(entry);
> }
>
> - info->ops->release_info(info);
> + if (info->ops && info->ops->release_info)
> + info->ops->release_info(info);
> }
>
> static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..6e85816ee1c3 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1652,17 +1652,6 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
> return cfg;
> }
>
> -/* release_info: free resources allocated by init_info */
> -static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> -{
> - struct acpi_pci_generic_root_info *ri;
> -
> - ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> - pci_ecam_free(ri->cfg);
> - kfree(ci->ops);
> - kfree(ri);
> -}
> -
> /* Interface called from ACPI code to setup PCI host controller */
> struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> {
> @@ -1683,7 +1672,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> if (!ri->cfg)
> goto free_root_ops;
>
> - root_ops->release_info = pci_acpi_generic_release_info;
> root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
> bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> --
> 2.43.0
>
