Re: [PATCH v2 4/4] PCI: of: introduce of_pci_verify_node()

[Alex Elder]

On 8/19/26 5:20 AM, Herve Codina wrote:
> Hi Alex,
>
> On Wed, 12 Aug 2026 12:22:47 -0500
> Alex Elder <elder@xxxxxxxxxxxx> wrote:
>
> > Commit 407d1a51921e9 ("PCI: Create device tree node for bridge") linked
> > the PCI enumeration process together with devicetree, creating a devicetree
> > node for discovered PCI bridges.  Its successor commit ae9813db1dc5a ("PCI:
> > Add quirks to generate device tree node for Xilinx Alveo U50") shows how
> > to use a PCI final fixup quirk to also create a devicetree node for a
> > non-bridge PCI device.  These changes allowed devicetree overlays to
> > describe components downstream of a PCI device, by providing a place to
> > attach the overlay.
> >
> > Note that the dynamic devicetree node is only created if the device didn't
> > already have an assigned node.
> >
> > Later, commit aa7b4bbcb3a1d ("arm64: dts: qcom: qcs6490-rb3gen2: Add
> > TC9563 PCIe switch node") *pre-defined* devicetree nodes to represent the
> > PCI device nodes that would (also) be discovered via the PCI enumeration
> > process.  The devicetree node in this case is created with the content
> > from the DTS file.  So when a (host) bridge is done being initialized
> > during PCI enumeration, no node is dynamically created (the commits
> > mentioned above do not apply).
> >
> > Ideally, any pre-defined PCI devicetree node would contain exactly the
> > same information as whatever the dynamic creation process would produce
> > (though it could include more).
> >
> > However that is not the case for the pre-defined Qualcomm RB3gen2 nodes.
> > And in particular, the endpoint (function) nodes include this property:
> >
> >      device_type = "pci";
> >
> > This is simply wrong; that property is meant only for bridge nodes.
> >
> > Rob Herring requested that a runtime check to be added to spot this
> > specific error, only for non-bridge PCI devices.
> >
> > (There are many things that could be verified for statically-defined
> > devicetree nodes, but this is all we'll do for now.)
> >
> > Signed-off-by: Alex Elder <elder@xxxxxxxxxxxx>
> > ---
> > v2: Verify even if PCI_DYNAMIC_OF_NODES is not defined (Sashiko)
> >
> >   drivers/pci/bus.c |  1 +
> >   drivers/pci/of.c  | 26 ++++++++++++++++++++++++++
> >   drivers/pci/pci.h |  3 +++
> >   3 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 655ed53436d3e..679afbc6d3109 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -351,6 +351,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> >   	 * are not assigned yet for some devices.
> >   	 */
> >   	pcibios_bus_add_device(dev);
> > +	of_pci_verify_node(dev);
> >   	pci_fixup_device(pci_fixup_final, dev);
> >   	if (pci_is_bridge(dev))
> >   		of_pci_make_dev_node(dev);
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index 8b18c4ba845cf..0bbf1a915b7d2 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -1006,3 +1006,29 @@ int of_pci_get_equalization_presets(struct device *dev,
> >   	return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > +
> > +/*
> > + * Check a pre-existing devicetree node for errors.  The PCI enumeration
> > + * process gathered a lot of information about the device, and ideally it
> > + * matches what the devicetree node says.
> > + */
>
> Can you switch the kernel doc format [0] or move the comment into the function itself ?

Sure.  I think it could be simplified too.  I'll take another
shot at it in version 3.

> [0] https://elixir.bootlin.com/linux/v7.2/source/Documentation/doc-guide/kernel-doc.rst#L67
>
> > +void of_pci_verify_node(struct pci_dev *pdev)
> > +{
> > +	struct device_node *np = pci_device_to_OF_node(pdev);
> > +
> > +	/* If there's no pre-existing node, there's nothing to check */
> > +	if (!np)
> > +		return;
> > +
> > +	if (pci_is_bridge(pdev))
> > +		return;
>
> The property 'device_type = "pci"' is needed for bridges.
>
> You could check that it is well present in case of bridges.

I think that's a good suggestion.  As I said in the comment, there
is a lot we *could* check, but Rob seemed to indicate he didn't
want to go very far down that slippery slope.  So I limited what
this patch does to specifically looking for the device type in
non-bridge nodes.

I will add the check you suggest in v3.

> > +
> > +	/*
> > +	 * Currently we just verify that non-bridges don't contain a
> > +	 * device_type = "pci" property.
> > +	 */
> > +	if (!of_node_is_type(np, "pci"))
> > +		return;
> > +
> > +	dev_err(&pdev->dev, "\"pci\" device_type NOT VALID for PCI endpoint\n");
> > +}

Thanks a lot for the review.

					-Alex

> Best regards,
> Hervé