Hi Lizhi, On 2024/07/29 09:47 AM, Lizhi Hou wrote: > Hi Amit > > On 7/29/24 04:13, Amit Machhiwal wrote: > > Hi Lizhi, > > > > On 2024/07/26 11:45 AM, Lizhi Hou wrote: > > > On 7/26/24 10:52, Rob Herring wrote: > > > > On Thu, Jul 25, 2024 at 6:06 PM Lizhi Hou <lizhi.hou@xxxxxxx> wrote: > > > > > Hi Amit, > > > > > > > > > > > > > > > I try to follow the option which add a OF flag. If Rob is ok with this, > > > > > I would suggest to use it instead of V1 patch > > > > > > > > > > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > > > > > index dda6092e6d3a..a401ed0463d9 100644 > > > > > --- a/drivers/of/dynamic.c > > > > > +++ b/drivers/of/dynamic.c > > > > > @@ -382,6 +382,11 @@ void of_node_release(struct kobject *kobj) > > > > > __func__, node); > > > > > } > > > > > > > > > > + if (of_node_check_flag(node, OF_CREATED_WITH_CSET)) { > > > > > + of_changeset_revert(node->data); > > > > > + of_changeset_destroy(node->data); > > > > > + } > > > > What happens if multiple nodes are created in the changeset? > > > Ok. multiple nodes will not work. > > > > > + > > > > > if (node->child) > > > > > pr_err("ERROR: %s() unexpected children for %pOF/%s\n", > > > > > __func__, node->parent, node->full_name); > > > > > @@ -507,6 +512,7 @@ struct device_node *of_changeset_create_node(struct > > > > > of_changeset *ocs, > > > > > np = __of_node_dup(NULL, full_name); > > > > > if (!np) > > > > > return NULL; > > > > > + of_node_set_flag(np, OF_CREATED_WITH_CSET); > > > > This should be set where the data ptr is set. > > > Ok. It sounds the fix could be simplified to 3 lines change. > > Thanks for the patch. The hot-plug and hot-unplug of PCI device seem to work > > fine as expected. I see this patch would attempt to remove only the nodes which > > were created in `of_pci_make_dev_node()` with the help of the newly introduced > > flag, which looks good to me. > > > > Also, since a call to `of_pci_make_dev_node()` from `pci_bus_add_device()`, that > > creates devices nodes only for bridge devices, is conditional on > > `pci_is_bridge()`, it only makes sense to retain the logical symmetry and call > > `of_pci_remove_node()` conditionally on `pci_is_bridge()` as well in > > `pci_stop_dev()`. Hence, I would like to propose the below change along with the > > above patch: > > > > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c > > index 910387e5bdbf..c6394bf562cd 100644 > > --- a/drivers/pci/remove.c > > +++ b/drivers/pci/remove.c > > @@ -23,7 +23,8 @@ static void pci_stop_dev(struct pci_dev *dev) > > device_release_driver(&dev->dev); > > pci_proc_detach_device(dev); > > pci_remove_sysfs_dev_files(dev); > > - of_pci_remove_node(dev); > > + if (pci_is_bridge(dev)) > > + of_pci_remove_node(dev); > > pci_dev_assign_added(dev, false); > > } > > > > Please let me know of your thoughts on this and based on that I can spin the v3 > > of this patch. > > As I mentioned, there are endpoints in pci quirks (pci/quirks.c) will also > create nodes by of_pci_make_dev_node(). So please remove above two lines. Sorry if I'm misinterpreting something here but as I mentioned, `of_pci_make_dev_node()` is called only for bridge devices with check performed via `pci_is_bridge()`, could you please elaborate more on why the same check can't be put while removing the node via `of_pci_remove_node()`? Thanks, Amit > > Thanks, > > Lizhi > > > > > In addition to this, can this patch be taken as part of 6.11 as a bug fix? > > > > Thanks, > > Amit > > > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > index 51e3dd0ea5ab..0b3ba1e1b18c 100644 > > > --- a/drivers/pci/of.c > > > +++ b/drivers/pci/of.c > > > @@ -613,7 +613,7 @@ void of_pci_remove_node(struct pci_dev *pdev) > > > struct device_node *np; > > > > > > np = pci_device_to_OF_node(pdev); > > > - if (!np || !of_node_check_flag(np, OF_DYNAMIC)) > > > + if (!np || !of_node_check_flag(np, OF_CREATED_WITH_CSET)) > > > return; > > > pdev->dev.of_node = NULL; > > > > > > @@ -672,6 +672,7 @@ void of_pci_make_dev_node(struct pci_dev *pdev) > > > if (ret) > > > goto out_free_node; > > > > > > + of_node_set_flag(np, OF_CREATED_WITH_CSET); > > > np->data = cset; > > > pdev->dev.of_node = np; > > > kfree(name); > > > diff --git a/include/linux/of.h b/include/linux/of.h > > > index a0bedd038a05..a46317f6626e 100644 > > > --- a/include/linux/of.h > > > +++ b/include/linux/of.h > > > @@ -153,6 +153,7 @@ extern struct device_node *of_stdout; > > > #define OF_POPULATED_BUS 4 /* platform bus created for children */ > > > #define OF_OVERLAY 5 /* allocated for an overlay */ > > > #define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */ > > > +#define OF_CREATED_WITH_CSET 7 /* created by of_changeset_create_node */ > > > > > > #define OF_BAD_ADDR ((u64)-1) > > > > > > > > > Lizhi > > > > > > > Rob