Re: [PATCH v2] PCI: Fix crash during pci_dev hot-unplug on pseries KVM guest

[Amit Machhiwal]

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