Re: [PATCH v8 7/8] PCI: of: Set fwnode device of newly created PCI device nodes

[Richard Cheng <icheng@nvidia.com>]

On Tue, Jun 30, 2026 at 12:28:01PM +0800, Herve Codina wrote:
> Device-tree node can be created when CONFIG_PCI_DYNAMIC_OF_NODES. Those
> nodes are created and filled based on PCI core information but the
> fwnode device field is not set.
> 
> When later an overlay is applied, this confuses fw_devlink. Indeed,
> without any device attached to the node, fw_devlink considers that this
> node will never become a device. When this node is pointed as a
> supplier, devlink looks at its ancestors in order to find a node with a
> device that could be used as the supplier.
> 
> In the PCI use case, this leads to links that wrongly use the PCI root
> bridge device as the supplier instead of the expected PCI device.
> 
> Setting the fwnode device to the device of the PCI device allows devlink
> to use this device as a supplier and so, correct links are created.
> 
> Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/of.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index ee9eb384b377..eda14cefca5e 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -709,6 +709,13 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
>  	if (ret)
>  		goto out_free_node;
>  
> +	/*
> +	 * Set the fwnode device in order to have fw_devlink creating links
> +	 * pointing to this PCI device instead of walking up to the PCI host
> +	 * bridge.
> +	 */
> +	fw_devlink_set_device(&np->fwnode, &pdev->dev);
> +
>  	ret = of_changeset_apply(cset);
>  	if (ret)
>  		goto out_free_node;
> -- 
> 2.54.0
> 
>

Hi Herve,

I wonder if this part has some issue, it sets np->fwnode.dev = &pdev->dev,
but I don't see am matching clear on removal path, I doubt the back-pointer
can outlive the pci_dev.

device_del() do the check
"""
if (dev->fwnode && dev->fwnode->dev == dev)
    fw_devlink_set_device(dev->fwnode, NULL);
"""

On removal, pci_stop_dev() calls of_pci_remove_node() before pci_destroy_dev()
calls device_del(), and of_pci_remove_node() -> device_remove_of_node() has already NULLed pdev->dev.fwnode by then, so the "dev->fwnode" guard is false, and
of_pci_remove_node() itself never clears np->fwnode.dev

If something holds an extra ref on np past removal, e.g. a DT overlay applied via configfs that pins np through its gragment targets,
np survives, the pci_dev is freed, and np->fwnode.dev dnalges into freed memory.
Then fw_devlink walker that resolve it via get_dev_from_fwnode() -> get_device() would hit a use-after-free .

I think of_pci_remove_node() should cleaer the back-pointer it set,
before dropping the node's ref, e.g.

"""
np = pci_device_to_OF_node(pdev);
if (!np || !of_node_check_flag(np, OF_DYNAMIC))
    return;

fw_devlink_set_device(&np->fwnode, NULL);
device_remove_of_node(&pdev->dev);
of_changeset_revert(np->data);
"""

Does that make sense to you ?

Best regards,
Richard Cheng.