| Message ID | 20250224141356.36325-3-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Add support for the PCI host bridge device-tree node creation. | expand |
On Mon, Feb 24, 2025 at 03:13:52PM +0100, Herve Codina wrote: > The commit 407d1a51921e ("PCI: Create device tree node for bridge") > creates of_node for PCI devices. The newly created of_node is attached > to an existing device. This is done setting directly pdev->dev.of_node > in the code. > > Even if pdev->dev.of_node cannot be previously set, this doesn't handle > the fwnode field of the struct device. Indeed, this field needs to be > set if it hasn't already been set. > > device_{add,remove}_of_node() have been introduced to handle this case. I guess another way to say this is: - If dev->of_node has already been set, it is an error and we want to do nothing. The error is impossible in this case because of_pci_make_dev_node() returns early if dev->of_node has been set. - Otherwise, we want to set dev->of_node (just as we previously did), and - if dev->fwnode has not been set, we want to set that too. So the whole point of this is to set dev->fwnode, which we didn't do before. But has np->fwnode been set to anything? Maybe it's buried somewhere inside of_changeset_create_node(), but I didn't see it. I can't tell if this actually *does* anything. And if it does, all the above is basically C code translated into English, so it doesn't tell us *why* this is useful, which is what I try to put in the merge commit log. > Use them instead of the direct setting. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > --- > drivers/pci/of.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 7a806f5c0d20..fb5e6da1ead0 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -653,8 +653,8 @@ void of_pci_remove_node(struct pci_dev *pdev) > np = pci_device_to_OF_node(pdev); > if (!np || !of_node_check_flag(np, OF_DYNAMIC)) > return; > - pdev->dev.of_node = NULL; > > + device_remove_of_node(&pdev->dev); > of_changeset_revert(np->data); > of_changeset_destroy(np->data); > of_node_put(np); > @@ -711,11 +711,18 @@ void of_pci_make_dev_node(struct pci_dev *pdev) > goto out_free_node; > > np->data = cset; > - pdev->dev.of_node = np; > + > + ret = device_add_of_node(&pdev->dev, np); > + if (ret) > + goto out_revert_cset; > + > kfree(name); > > return; > > +out_revert_cset: > + np->data = NULL; > + of_changeset_revert(cset); > out_free_node: > of_node_put(np); > out_destroy_cset: > -- > 2.48.1 >
On Fri, 28 Feb 2025 14:58:55 -0600 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Mon, Feb 24, 2025 at 03:13:52PM +0100, Herve Codina wrote: > > The commit 407d1a51921e ("PCI: Create device tree node for bridge") > > creates of_node for PCI devices. The newly created of_node is attached > > to an existing device. This is done setting directly pdev->dev.of_node > > in the code. > > > > Even if pdev->dev.of_node cannot be previously set, this doesn't handle > > the fwnode field of the struct device. Indeed, this field needs to be > > set if it hasn't already been set. > > > > device_{add,remove}_of_node() have been introduced to handle this case. > > I guess another way to say this is: > > - If dev->of_node has already been set, it is an error and we want > to do nothing. The error is impossible in this case because > of_pci_make_dev_node() returns early if dev->of_node has been set. > > - Otherwise, we want to set dev->of_node (just as we previously > did), and > > - if dev->fwnode has not been set, we want to set that too. > > So the whole point of this is to set dev->fwnode, which we didn't do > before. But has np->fwnode been set to anything? Maybe it's buried > somewhere inside of_changeset_create_node(), but I didn't see it. np->fwnode can be set by ACPI. We are at the frontier between ACPI and device-tree. The ofnode is created and filled from an already existing device. This device can be created from information provided by the ACPI world. In that case, np->fwnode is set to and ACPI fwnode. Best regards, Hervé
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 7a806f5c0d20..fb5e6da1ead0 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -653,8 +653,8 @@ void of_pci_remove_node(struct pci_dev *pdev) np = pci_device_to_OF_node(pdev); if (!np || !of_node_check_flag(np, OF_DYNAMIC)) return; - pdev->dev.of_node = NULL; + device_remove_of_node(&pdev->dev); of_changeset_revert(np->data); of_changeset_destroy(np->data); of_node_put(np); @@ -711,11 +711,18 @@ void of_pci_make_dev_node(struct pci_dev *pdev) goto out_free_node; np->data = cset; - pdev->dev.of_node = np; + + ret = device_add_of_node(&pdev->dev, np); + if (ret) + goto out_revert_cset; + kfree(name); return; +out_revert_cset: + np->data = NULL; + of_changeset_revert(cset); out_free_node: of_node_put(np); out_destroy_cset:
The commit 407d1a51921e ("PCI: Create device tree node for bridge") creates of_node for PCI devices. The newly created of_node is attached to an existing device. This is done setting directly pdev->dev.of_node in the code. Even if pdev->dev.of_node cannot be previously set, this doesn't handle the fwnode field of the struct device. Indeed, this field needs to be set if it hasn't already been set. device_{add,remove}_of_node() have been introduced to handle this case. Use them instead of the direct setting. Signed-off-by: Herve Codina <herve.codina@bootlin.com> Reviewed-by: Rob Herring (Arm) <robh@kernel.org> --- drivers/pci/of.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)