Hi Bjorn On Tue, Jun 17, 2025 at 10:40 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > [+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. > Thank you for your detailed review and pointing out these issues. I will wait a few days before resending. Thanks again! > > 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 > > regards, Zhe
