[PATCH v3 0/2] Attach DT nodes to existing PCI devices

[PATCH v3 0/2] Attach DT nodes to existing PCI devices

[Herve Codina]

Hi,

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).

Creating a new device from a DT node leads to some kind of wrong struct
device duplication to represent the exact same PCI device.

This patch series first introduces device_{add,remove}_of_node() in
order to add or remove a newly created of_node to an already existing
device. Then it fixes the DT node creation for PCI devices to add or
remove the created node to the existing PCI device without any new
device creation.

Compared to the previous iteration:
  https://lore.kernel.org/all/20231130165700.685764-1-herve.codina@bootlin.com/
this v3 series rewrap commit log.

The potential issue related to the sysfs of_node symlink added after the
sysfs PCI device is visible from user-space (raised during the v2
review) is maybe not a problem according to Bjorn:
  https://lore.kernel.org/all/20240319165430.GA1233494@bhelgaas/

IMHO, the discussions started with Rob around the interrupt-controller
during the v2 review are out of the issue this specific series tries to
fix. Some modifications are needed for the interrupt-controller topic
but should be done in a specific series.

Best regards,
Hervé

Changes v2 -> v3
  - Patch 1
    No changes

  - Patch 2
    Rewrap commit log

Changes v1 -> v2
  - Patch 1
    Add 'Cc: stable@vger.kernel.org'

Herve Codina (2):
  driver core: Introduce device_{add,remove}_of_node()
  PCI: of: Attach created of_node to existing device

 drivers/base/core.c    | 74 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/of.c       | 15 +++++++--
 include/linux/device.h |  2 ++
 3 files changed, 89 insertions(+), 2 deletions(-)

-- 
2.44.0

[PATCH v3 1/2] driver core: Introduce device_{add,remove}_of_node()

[Herve Codina]

An of_node can be duplicated from an existing device using
device_set_of_node_from_dev() or initialized using device_set_node()
In both cases, these functions have to be called before the device_add()
call in order to have the of_node link created in the device sysfs
directory. Further more, these function cannot prevent any of_node
and/or fwnode overwrites.

When adding an of_node on an already present device, the following
operations need to be done:
- Attach the of_node if no of_node were already attached
- Attach the of_node as a fwnode if no fwnode were already attached
- Create the of_node sysfs link if needed

This is the purpose of device_add_of_node().
device_remove_of_node() reverts the operations done by
device_add_of_node().

Cc: stable@vger.kernel.org
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c    | 74 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  2 ++
 2 files changed, 76 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 521757408fc0..7e3af0ad770a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5127,6 +5127,80 @@ void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(set_secondary_fwnode);
 
+/**
+ * device_remove_of_node - Remove an of_node from a device
+ * @dev: device whose device-tree node is being removed
+ */
+void device_remove_of_node(struct device *dev)
+{
+	dev = get_device(dev);
+	if (!dev)
+		return;
+
+	if (!dev->of_node)
+		goto end;
+
+	sysfs_remove_link(&dev->kobj, "of_node");
+
+	if (dev->fwnode == of_fwnode_handle(dev->of_node))
+		dev->fwnode = NULL;
+
+	of_node_put(dev->of_node);
+	dev->of_node = NULL;
+
+end:
+	put_device(dev);
+}
+EXPORT_SYMBOL_GPL(device_remove_of_node);
+
+/**
+ * device_add_of_node - Add an of_node to an existing device
+ * @dev: device whose device-tree node is being added
+ * @of_node: of_node to add
+ */
+void device_add_of_node(struct device *dev, struct device_node *of_node)
+{
+	int ret;
+
+	if (!of_node)
+		return;
+
+	dev = get_device(dev);
+	if (!dev)
+		return;
+
+	if (dev->of_node) {
+		dev_warn(dev, "Replace node %pOF with %pOF\n", dev->of_node, of_node);
+		device_remove_of_node(dev);
+	}
+
+	dev->of_node = of_node_get(of_node);
+
+	if (!dev->fwnode)
+		dev->fwnode = of_fwnode_handle(of_node);
+
+	if (!dev->p) {
+		/*
+		 * device_add() was not previously called.
+		 * The of_node link will be created when device_add() is called.
+		 */
+		goto end;
+	}
+
+	/*
+	 * device_add() was previously called and so the of_node link was not
+	 * created by device_add_class_symlinks().
+	 * Create this link now.
+	 */
+	ret = sysfs_create_link(&dev->kobj, of_node_kobj(of_node), "of_node");
+	if (ret)
+		dev_warn(dev, "Error %d creating of_node link\n", ret);
+
+end:
+	put_device(dev);
+}
+EXPORT_SYMBOL_GPL(device_add_of_node);
+
 /**
  * device_set_of_node_from_dev - reuse device-tree node of another device
  * @dev: device whose device-tree node is being set
diff --git a/include/linux/device.h b/include/linux/device.h
index 97c4b046c09d..1795121dee9a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1127,6 +1127,8 @@ int device_offline(struct device *dev);
 int device_online(struct device *dev);
 void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
 void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
+void device_add_of_node(struct device *dev, struct device_node *of_node);
+void device_remove_of_node(struct device *dev);
 void device_set_of_node_from_dev(struct device *dev, const struct device *dev2);
 void device_set_node(struct device *dev, struct fwnode_handle *fwnode);
 
-- 
2.44.0

[PATCH v3 2/2] PCI: of: Attach created of_node to existing device

[Herve Codina]

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: stable@vger.kernel.org
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 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

Re: [PATCH v3 0/2] Attach DT nodes to existing PCI devices

[Herve Codina]

Hi,

On Mon, 25 Mar 2024 16:39:13 +0100
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi,
> 
> 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).
> 
> Creating a new device from a DT node leads to some kind of wrong struct
> device duplication to represent the exact same PCI device.
> 
> This patch series first introduces device_{add,remove}_of_node() in
> order to add or remove a newly created of_node to an already existing
> device. Then it fixes the DT node creation for PCI devices to add or
> remove the created node to the existing PCI device without any new
> device creation.
> 
> Compared to the previous iteration:
>   https://lore.kernel.org/all/20231130165700.685764-1-herve.codina@bootlin.com/
> this v3 series rewrap commit log.
> 
> The potential issue related to the sysfs of_node symlink added after the
> sysfs PCI device is visible from user-space (raised during the v2
> review) is maybe not a problem according to Bjorn:
>   https://lore.kernel.org/all/20240319165430.GA1233494@bhelgaas/
> 
> IMHO, the discussions started with Rob around the interrupt-controller
> during the v2 review are out of the issue this specific series tries to
> fix. Some modifications are needed for the interrupt-controller topic
> but should be done in a specific series.
> 
> Best regards,
> Hervé
> 

I have received no feedback on this v3 series.
I know maintainers are busy but I would like to be sure that this series was
not simply missed.

Best regards,
Hervé

Re: [PATCH v3 2/2] PCI: of: Attach created of_node to existing device

[Greg Kroah-Hartman]

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: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

I need an ack from the maintainer here before I can take this.

thanks,

greg k-h

Re: [PATCH v3 2/2] PCI: of: Attach created of_node to existing device

[Rob Herring]

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: stable@vger.kernel.org
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> 
> 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

Re: [PATCH v3 2/2] PCI: of: Attach created of_node to existing device

[Greg Kroah-Hartman]

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: stable@vger.kernel.org
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > 
> > 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

Re: [PATCH v3 2/2] PCI: of: Attach created of_node to existing device

[Herve Codina]

Hi Greg, Rob,

On Fri, 12 Apr 2024 09:41:19 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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: stable@vger.kernel.org
> > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> > > 
> > > 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é