Hi Manivannan,
> Subject: Re: [PATCH] PCI: check bridge->bus in
> pci_host_common_remove
>
> On Mon, Oct 28, 2024 at 04:46:43PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > When PCI node was created using an overlay and the overlay is
> > reverted/destroyed, the "linux,pci-domain" property no longer exists,
> > so of_get_pci_domain_nr will return failure. Then
> > of_pci_bus_release_domain_nr will actually use the dynamic IDA,
> even
> > if the IDA was allocated in static IDA. So the flow is as below:
> > A: of_changeset_revert
> > pci_host_common_remove
> > pci_bus_release_domain_nr
> > of_pci_bus_release_domain_nr
> > of_get_pci_domain_nr # fails because overlay is gone
> > ida_free(&pci_domain_nr_dynamic_ida)
> >
> > With driver calls pci_host_common_remove explicity, the flow
> becomes:
> > B pci_host_common_remove
> > pci_bus_release_domain_nr
> > of_pci_bus_release_domain_nr
> > of_get_pci_domain_nr # succeeds in this order
> > ida_free(&pci_domain_nr_static_ida)
> > A of_changeset_revert
> > pci_host_common_remove
> >
> > With updated flow, the pci_host_common_remove will be called
> twice, so
> > need to check 'bridge->bus' to avoid accessing invalid pointer.
> >
> > Fixes: c14f7ccc9f5d ("PCI: Assign PCI domain IDs by ida_alloc()")
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
>
> I went through the previous discussion [1] and I couldn't see an
> agreement on the point raised by Bjorn on 'removing the host bridge
> before the overlay'.
This patch is an agreement to Bjorn's idea.
I have added pci_host_common_remove to remove host bridge
before removing overlay as I wrote in commit log.
But of_changeset_revert will still runs into pci_host_
common_remove to remove the host bridge again. Per
my view, the design of of_changeset_revert to remove
the device tree node will trigger device remove, so even
pci_host_common_remove was explicitly used before
of_changeset_revert. The following call to of_changeset_revert
will still call pci_host_common_remove.
So I did this patch to add a check of 'bus' to avoid remove again.
>
> I do think this is a valid point and if you do not think so, please state
> the reason.
I agree Bjorn's view, this patch is output of agreement as I write above.
Thanks,
Peng.
>
> - Mani
>
> [1]
> https://lore.kernel.org/lkml/20230913115737.GA426735@bhelgaas/
>
> > ---
> >
> > V1:
> > Not sure to keep the fixes here. I could drop the Fixes tag if it is
> > improper.
> > This is to revisit the patch [1] which was rejected last year. This
> > new flow is using the suggest flow following Bjorn's suggestion.
> > But of_changeset_revert will still invoke plaform_remove and then
> > pci_host_common_remove. So worked out this patch together with a
> patch
> > to jailhouse driver as below:
> > static void destroy_vpci_of_overlay(void) {
> > + struct device_node *vpci_node = NULL;
> > +
> > if (overlay_applied) {
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(6,6,0)
> > + vpci_node = of_find_node_by_path("/pci@0");
> > + if (vpci_node) {
> > + struct platform_device *pdev =
> of_find_device_by_node(vpci_node);
> > + if (!pdev)
> > + printk("Not found device for /pci@0\n");
> > + else {
> > + pci_host_common_remove(pdev);
> > + platform_device_put(pdev);
> > + }
> > + }
> > + of_node_put(vpci_node); #endif
> > +
> > of_changeset_revert(&overlay_changeset);
> >
> > [1]
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> lore
> > .kernel.org%2Flkml%2F20230908224858.GA306960%40bhelgaas%2
> FT%2F%23md12e
> >
> 6097d91a012ede78c997fc5abf460029a569&data=05%7C02%7Cpeng.
> fan%40nxp.com
> > %7C025e209cf9264c4240fa08dd053d9211%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635
> > %7C0%7C0%7C638672484189046564%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGki
> >
> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIl
> dUIjoyfQ
> > %3D%3D%7C0%7C%7C%7C&sdata=uCo5MO5fEqCjBzwZ8hdnsf3ORh
> SedhrJWvNOCCMNvG0%
> > 3D&reserved=0
> >
> > drivers/pci/controller/pci-host-common.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-host-common.c
> > b/drivers/pci/controller/pci-host-common.c
> > index cf5f59a745b3..5a9c29fc57cd 100644
> > --- a/drivers/pci/controller/pci-host-common.c
> > +++ b/drivers/pci/controller/pci-host-common.c
> > @@ -86,8 +86,10 @@ void pci_host_common_remove(struct
> platform_device *pdev)
> > struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
> >
> > pci_lock_rescan_remove();
> > - pci_stop_root_bus(bridge->bus);
> > - pci_remove_root_bus(bridge->bus);
> > + if (bridge->bus) {
> > + pci_stop_root_bus(bridge->bus);
> > + pci_remove_root_bus(bridge->bus);
> > + }
> > pci_unlock_rescan_remove();
> > }
> > EXPORT_SYMBOL_GPL(pci_host_common_remove);
> > --
> > 2.37.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
