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

[Alex Elder <elder@riscstar.com>]

On 8/19/26 5:20 AM, Herve Codina wrote:
> Hi Alex,
> 
> On Wed, 12 Aug 2026 12:22:47 -0500
> Alex Elder <elder@riscstar.com> 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@riscstar.com>
>> ---
>> 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é