The commit 407d1a51921e ("PCI: Create device tree node for bridge")
creates of_node for PCI devices.
During the insertion handling of these new DT nodes done by of_platform,
new devices (struct device) are created. For each PCI devices a struct
device is already present (created and handled by the PCI core).
Having a second struct device to represent the exact same PCI device is
not correct.
On the of_node creation:
- tell the of_platform that there is no need to create a device for this
node (OF_POPULATED flag),
- link this newly created of_node to the already present device,
- tell fwnode that the device attached to this of_node is ready using
fwnode_dev_initialized().
With this fix, the of_node are available in the sysfs device tree:
/sys/devices/platform/soc/d0070000.pcie/
+ of_node -> .../devicetree/base/soc/pcie@d0070000
+ pci0000:00
+ 0000:00:00.0
+ of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0
+ 0000:01:00.0
+ of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0/dev@0,0
On the of_node removal, revert the operations.
Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
Cc: [email protected]
Signed-off-by: Herve Codina <[email protected]>
---
drivers/pci/of.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5ab..5afd2731e876 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -615,7 +615,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);
@@ -668,12 +669,22 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
if (ret)
goto out_free_node;
+ /*
+ * This of_node will be added to an existing device.
+ * Avoid any device creation and use the existing device
+ */
+ of_node_set_flag(np, OF_POPULATED);
+ np->fwnode.dev = &pdev->dev;
+ fwnode_dev_initialized(&np->fwnode, true);
+
ret = of_changeset_apply(cset);
if (ret)
goto out_free_node;
np->data = cset;
- pdev->dev.of_node = np;
+
+ /* Add the of_node to the existing device */
+ device_add_of_node(&pdev->dev, np);
kfree(name);
return;
--
2.44.0
On Mon, Mar 25, 2024 at 04:39:15PM +0100, Herve Codina wrote:
> The commit 407d1a51921e ("PCI: Create device tree node for bridge")
> creates of_node for PCI devices.
>
> During the insertion handling of these new DT nodes done by of_platform,
> new devices (struct device) are created. For each PCI devices a struct
> device is already present (created and handled by the PCI core).
> Having a second struct device to represent the exact same PCI device is
> not correct.
>
> On the of_node creation:
> - tell the of_platform that there is no need to create a device for this
> node (OF_POPULATED flag),
> - link this newly created of_node to the already present device,
> - tell fwnode that the device attached to this of_node is ready using
> fwnode_dev_initialized().
>
> With this fix, the of_node are available in the sysfs device tree:
> /sys/devices/platform/soc/d0070000.pcie/
> + of_node -> .../devicetree/base/soc/pcie@d0070000
> + pci0000:00
> + 0000:00:00.0
> + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0
> + 0000:01:00.0
> + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0/dev@0,0
>
> On the of_node removal, revert the operations.
>
> Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> Cc: [email protected]
> Signed-off-by: Herve Codina <[email protected]>
I need an ack from the maintainer here before I can take this.
thanks,
greg k-h
On Thu, Apr 11, 2024 at 03:23:55PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Mar 25, 2024 at 04:39:15PM +0100, Herve Codina wrote:
> > The commit 407d1a51921e ("PCI: Create device tree node for bridge")
> > creates of_node for PCI devices.
> >
> > During the insertion handling of these new DT nodes done by of_platform,
> > new devices (struct device) are created. For each PCI devices a struct
> > device is already present (created and handled by the PCI core).
> > Having a second struct device to represent the exact same PCI device is
> > not correct.
> >
> > On the of_node creation:
> > - tell the of_platform that there is no need to create a device for this
> > node (OF_POPULATED flag),
> > - link this newly created of_node to the already present device,
> > - tell fwnode that the device attached to this of_node is ready using
> > fwnode_dev_initialized().
> >
> > With this fix, the of_node are available in the sysfs device tree:
> > /sys/devices/platform/soc/d0070000.pcie/
> > + of_node -> .../devicetree/base/soc/pcie@d0070000
> > + pci0000:00
> > + 0000:00:00.0
> > + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0
> > + 0000:01:00.0
> > + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0/dev@0,0
> >
> > On the of_node removal, revert the operations.
> >
> > Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> > Cc: [email protected]
> > Signed-off-by: Herve Codina <[email protected]>
>
> I need an ack from the maintainer here before I can take this.
Correct me if I'm wrong, but having the of_node sysfs link populated or
changed after device_add is a race we lost. Userspace is notified about
the new device and then some time later the symlink shows up.
However, it so far is not appearing that there's an easy way to
reshuffle order of things to fix this.
Maybe the short term (and stable) answer just don't create any of_node
symlinks on these dynamically created nodes.
Rob
On Thu, Apr 11, 2024 at 03:34:49PM -0500, Rob Herring wrote:
> On Thu, Apr 11, 2024 at 03:23:55PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Mar 25, 2024 at 04:39:15PM +0100, Herve Codina wrote:
> > > The commit 407d1a51921e ("PCI: Create device tree node for bridge")
> > > creates of_node for PCI devices.
> > >
> > > During the insertion handling of these new DT nodes done by of_platform,
> > > new devices (struct device) are created. For each PCI devices a struct
> > > device is already present (created and handled by the PCI core).
> > > Having a second struct device to represent the exact same PCI device is
> > > not correct.
> > >
> > > On the of_node creation:
> > > - tell the of_platform that there is no need to create a device for this
> > > node (OF_POPULATED flag),
> > > - link this newly created of_node to the already present device,
> > > - tell fwnode that the device attached to this of_node is ready using
> > > fwnode_dev_initialized().
> > >
> > > With this fix, the of_node are available in the sysfs device tree:
> > > /sys/devices/platform/soc/d0070000.pcie/
> > > + of_node -> .../devicetree/base/soc/pcie@d0070000
> > > + pci0000:00
> > > + 0000:00:00.0
> > > + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0
> > > + 0000:01:00.0
> > > + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0/dev@0,0
> > >
> > > On the of_node removal, revert the operations.
> > >
> > > Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> > > Cc: [email protected]
> > > Signed-off-by: Herve Codina <[email protected]>
> >
> > I need an ack from the maintainer here before I can take this.
>
> Correct me if I'm wrong, but having the of_node sysfs link populated or
> changed after device_add is a race we lost. Userspace is notified about
> the new device and then some time later the symlink shows up.
Ah, yes, I missed that, good catch, this will not work.
> However, it so far is not appearing that there's an easy way to
> reshuffle order of things to fix this.
>
> Maybe the short term (and stable) answer just don't create any of_node
> symlinks on these dynamically created nodes.
That would work, but does userspace really need to know this
information?
thanks,
greg k-h
Hi Greg, Rob,
On Fri, 12 Apr 2024 09:41:19 +0200
Greg Kroah-Hartman <[email protected]> wrote:
> On Thu, Apr 11, 2024 at 03:34:49PM -0500, Rob Herring wrote:
> > On Thu, Apr 11, 2024 at 03:23:55PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Mar 25, 2024 at 04:39:15PM +0100, Herve Codina wrote:
> > > > The commit 407d1a51921e ("PCI: Create device tree node for bridge")
> > > > creates of_node for PCI devices.
> > > >
> > > > During the insertion handling of these new DT nodes done by of_platform,
> > > > new devices (struct device) are created. For each PCI devices a struct
> > > > device is already present (created and handled by the PCI core).
> > > > Having a second struct device to represent the exact same PCI device is
> > > > not correct.
> > > >
> > > > On the of_node creation:
> > > > - tell the of_platform that there is no need to create a device for this
> > > > node (OF_POPULATED flag),
> > > > - link this newly created of_node to the already present device,
> > > > - tell fwnode that the device attached to this of_node is ready using
> > > > fwnode_dev_initialized().
> > > >
> > > > With this fix, the of_node are available in the sysfs device tree:
> > > > /sys/devices/platform/soc/d0070000.pcie/
> > > > + of_node -> .../devicetree/base/soc/pcie@d0070000
> > > > + pci0000:00
> > > > + 0000:00:00.0
> > > > + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0
> > > > + 0000:01:00.0
> > > > + of_node -> .../devicetree/base/soc/pcie@d0070000/pci@0,0/dev@0,0
> > > >
> > > > On the of_node removal, revert the operations.
> > > >
> > > > Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> > > > Cc: [email protected]
> > > > Signed-off-by: Herve Codina <[email protected]>
> > >
> > > I need an ack from the maintainer here before I can take this.
> >
> > Correct me if I'm wrong, but having the of_node sysfs link populated or
> > changed after device_add is a race we lost. Userspace is notified about
> > the new device and then some time later the symlink shows up.
>
> Ah, yes, I missed that, good catch, this will not work.
>
> > However, it so far is not appearing that there's an easy way to
> > reshuffle order of things to fix this.
> >
> > Maybe the short term (and stable) answer just don't create any of_node
> > symlinks on these dynamically created nodes.
>
> That would work, but does userspace really need to know this
> information?
>
I don't think that the user space really need this information.
I agree, it should work.
Let me rework my series in that sense and perform some tests before
sending a new iteration removing the of_node sysfs link creation.
Best regards,
Hervé