[PATCH 0/4] PCI: of: warn on bogus device_type property

[PATCH 0/4] PCI: of: warn on bogus device_type property

[Alex Elder]

The purpose of this series lies in its final patch, where a new check
is added whenever a PCI devicetree node is found to already exist when
one might otherwise be dynamically created.

PCI has a well-defined bus and device discovery process.  The
PCI_DYNAMIC_OF_NODES Kconfig option allows PCI devices to *also*
have a devicetree node.  This enables certain things that are
not possible with PCI enumeration alone. 

While working on a Qualcomm platform, I learned that some PCI
endpoint nodes were defined with device_type = "pci" properties.
Herve Codina pointed out that this was not correct.  Rob Herring
indicated that people seem to have trouble getting the PCI
devicetree nodes right, and asked whether we could warn if this
particular problem occurred.

The last patch in this series implements that check and warning.
The first three patches are fairly trivial changes to clean up
some related code.

					-Alex

Alex Elder (4):
  PCI: of: drop the reg_num argument to of_pci_set_address()
  PCI: of: don't zero flags in of_pci_get_addr_flags()
  PCI: of: make a flags argument optional
  PCI: of: introduce of_pci_verify_node()

 drivers/pci/bus.c         |  1 +
 drivers/pci/of.c          | 27 +++++++++++++++++++++++++++
 drivers/pci/of_property.c | 20 ++++++++++----------
 drivers/pci/pci.h         |  2 ++
 4 files changed, 40 insertions(+), 10 deletions(-)


base-commit: 075b74841bd0065a3bda3440873c747938e69b68
-- 
2.53.0

[PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address()

[Alex Elder]

The reg_num argument passed to of_pci_set_address() is always zero,
so get rid of it.

Reviewed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/pci/of_property.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 75a358f73e694..505226b876c56 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -52,7 +52,7 @@ enum of_pci_prop_compatible {
 };
 
 static void of_pci_set_address(struct pci_dev *pdev, u32 *prop, u64 addr,
-			       u32 reg_num, u32 flags, bool reloc)
+			       u32 flags, bool reloc)
 {
 	if (pdev) {
 		prop[0] = FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) |
@@ -61,7 +61,7 @@ static void of_pci_set_address(struct pci_dev *pdev, u32 *prop, u64 addr,
 	} else
 		prop[0] = 0;
 
-	prop[0] |= flags | reg_num;
+	prop[0] |= flags;
 	if (!reloc) {
 		prop[0] |= OF_PCI_ADDR_FIELD_NONRELOC;
 		prop[1] = upper_32_bits(addr);
@@ -131,7 +131,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
 			continue;
 
 		val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
-		of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
+		of_pci_set_address(pdev, rp[i].parent_addr, val64, flags,
 				   false);
 		if (pci_is_bridge(pdev)) {
 			memcpy(rp[i].child_addr, rp[i].parent_addr,
@@ -164,7 +164,7 @@ static int of_pci_prop_reg(struct pci_dev *pdev, struct of_changeset *ocs,
 	struct of_pci_addr_pair reg = { 0 };
 
 	/* configuration space */
-	of_pci_set_address(pdev, reg.phys_addr, 0, 0, 0, true);
+	of_pci_set_address(pdev, reg.phys_addr, 0, 0, true);
 
 	return of_changeset_add_prop_u32_array(ocs, np, "reg", (u32 *)&reg,
 					       sizeof(reg) / sizeof(u32));
@@ -458,7 +458,7 @@ static int of_pci_host_bridge_prop_ranges(struct pci_host_bridge *bridge,
 		/* PCI bus address */
 		val64 = res->start;
 		of_pci_set_address(NULL, &ranges[ranges_sz],
-				   val64 - window->offset, 0, flags, false);
+				   val64 - window->offset, flags, false);
 		ranges_sz += OF_PCI_ADDRESS_CELLS;
 
 		/* Host bus address */
-- 
2.53.0

Re: [PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `of_node_put()` in `of_pci_prop_intr_map()` causes a memory leak of interrupt parent device nodes.
- [High] Missing NULL checks for `kasprintf()` returns in `of_pci_prop_compatible()` leads to a kernel crash via `strlen(NULL)`.
--

--- Patch [1]: [PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address() ---
commit 2224ebd8fd372dc1822f3ceaf7519520a5ca9bdf
Author: Alex Elder <elder@riscstar.com>

PCI: of: drop the reg_num argument to of_pci_set_address()

The reg_num argument passed to of_pci_set_address() is always zero,
so get rid of it.

> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 75a358f73e694..505226b876c56 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a potential resource leak in of_pci_prop_intr_map().

Does this code leak the interrupt parent device nodes?

drivers/pci/of_property.c:of_pci_prop_intr_map() {
    ...
    for (pin = 1; pin <= OF_PCI_MAX_INT_PIN;  pin++) {
        i = pin - 1;
        out_irq[i].np = pnode;
        out_irq[i].args_count = 1;
        out_irq[i].args[0] = pin;
        ret = of_irq_parse_raw(laddr, &out_irq[i]);
        if (ret) {
            out_irq[i].np = NULL;
            pci_dbg(pdev, "parse irq %d failed, ret %d", pin, ret);
            continue;
        }
        of_property_read_u32(out_irq[i].np, "#address-cells",
                             &addr_sz[i]);
    }
    ...
    return 0;

failed:
    kfree(int_map);
    return ret;
}

of_irq_parse_raw() populates out_irq[i].np with a device node that has an
incremented reference count.

Should there be a matching of_node_put() called on these successfully parsed
nodes before the function returns?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential NULL pointer
dereference in of_pci_prop_compatible().

Can this crash the kernel if memory allocation fails?

drivers/pci/of_property.c:of_pci_prop_compatible() {
    ...
    compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] =
        kasprintf(GFP_KERNEL, "pci%x,%x", pdev->vendor, pdev->device);
    compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] =
        kasprintf(GFP_KERNEL, "pciclass,%06x", pdev->class);
    compat_strs[PROP_COMPAT_PCICLASS_CCSS] =
        kasprintf(GFP_KERNEL, "pciclass,%04x", pdev->class >> 8);

    ret = of_changeset_add_prop_string_array(ocs, np, "compatible",
                                             compat_strs, PROP_COMPAT_NUM);
    ...
}

If kasprintf() returns NULL, the compat_strs array will contain NULL pointers.

When this unvalidated array is passed to
of_changeset_add_prop_string_array(), could it lead to a kernel crash when
the string array is processed and strlen() is called on a NULL element?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807194100.455599-1-elder@riscstar.com?part=1

Re: [PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address()

[Alex Elder]

On 8/7/26 2:54 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing `of_node_put()` in `of_pci_prop_intr_map()` causes a memory leak of interrupt parent device nodes.
> - [High] Missing NULL checks for `kasprintf()` returns in `of_pci_prop_compatible()` leads to a kernel crash via `strlen(NULL)`.
> --
> 
> --- Patch [1]: [PATCH 1/4] PCI: of: drop the reg_num argument to of_pci_set_address() ---
> commit 2224ebd8fd372dc1822f3ceaf7519520a5ca9bdf
> Author: Alex Elder <elder@riscstar.com>
> 
> PCI: of: drop the reg_num argument to of_pci_set_address()
> 
> The reg_num argument passed to of_pci_set_address() is always zero,
> so get rid of it.
> 
>> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
>> index 75a358f73e694..505226b876c56 100644
>> --- a/drivers/pci/of_property.c
>> +++ b/drivers/pci/of_property.c
> 
> [ ... ]
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing this file I
> noticed a potential resource leak in of_pci_prop_intr_map().
> 
> Does this code leak the interrupt parent device nodes?

There are two device nodes at play here.  The first is
passed in as an argument, np, and although this is referring
to the other one, I analyzed it anyway.

The only place that pointer is used is in the three
of_changeset_add_prop_u32*() calls at the end of the
function.  No reference is taken or dropped to that
node in this function, and presumably that is up to
the caller to manage.

The only caller is of_pci_add_properties(), which similarly
gets the node pointer as an argument, and relies on its
caller for managing its references.  This function's only
caller is of_pci_make_dev_node(), whose purpose is literally
to create a new device node for the purpose of attaching
it to a PCI device node that has none.

So I don't think there is any leak of the np variable passed
in as an argument.


The second device node is pnode, which comes either from
pci_device_to_OF_node() or from pci_bus_to_OF_node().
Neither of those functions takes a reference to the
node pointer they return (assuming it's non-null).

That pointer is assigned to each out_irq[x].np field
before it's passed to of_irq_parse_raw().  The value
returned from of_irq_parse_raw() will either be a null
pointer or will be another device node *with a reference*.

We'll refer to this as *THE* reference.

If this branch is taken:

         if (out_irq->args_count != intsize)
                 goto fail;

The code at fail will properly drop the reference:

fail:
         of_node_put(ipar);

         return rc;

The next block of code gets *another* reference to ipar,
then follows the OF parent pointers up, dropping the
reference taken as each new parent is found.  Once
that loop exits, we still seem to hold THE reference,
though others seem to come and go properly.

Next there is a "big while loop", and it looks to me
like this return statement:

                         pr_debug(" -> got it !\n");
                         return 0;

...does indeed leak a reference to ipar.

There is more to study on this function to be sure this is
right.  But based on this quick look, the Sashiko report
seems right.

==> Can anyone else confirm or refute this?

If nobody tells me this is wrong, I will plan to submit
a bug fix for this.

> 
> drivers/pci/of_property.c:of_pci_prop_intr_map() {
>      ...
>      for (pin = 1; pin <= OF_PCI_MAX_INT_PIN;  pin++) {
>          i = pin - 1;
>          out_irq[i].np = pnode;
>          out_irq[i].args_count = 1;
>          out_irq[i].args[0] = pin;
>          ret = of_irq_parse_raw(laddr, &out_irq[i]);
>          if (ret) {
>              out_irq[i].np = NULL;
>              pci_dbg(pdev, "parse irq %d failed, ret %d", pin, ret);
>              continue;
>          }
>          of_property_read_u32(out_irq[i].np, "#address-cells",
>                               &addr_sz[i]);
>      }
>      ...
>      return 0;
> 
> failed:
>      kfree(int_map);
>      return ret;
> }
> 
> of_irq_parse_raw() populates out_irq[i].np with a device node that has an
> incremented reference count.
> 
> Should there be a matching of_node_put() called on these successfully parsed
> nodes before the function returns?
> 
> [Severity: High]
> This is a pre-existing issue, but I also noticed a potential NULL pointer
> dereference in of_pci_prop_compatible().
> 
> Can this crash the kernel if memory allocation fails?

Yes it can.  of_changeset_add_prop_string_array() blindly calls
strlen() on each array string provided.

I will create a bug fix for this issue as well.

					-Alex

> 
> drivers/pci/of_property.c:of_pci_prop_compatible() {
>      ...
>      compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] =
>          kasprintf(GFP_KERNEL, "pci%x,%x", pdev->vendor, pdev->device);
>      compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] =
>          kasprintf(GFP_KERNEL, "pciclass,%06x", pdev->class);
>      compat_strs[PROP_COMPAT_PCICLASS_CCSS] =
>          kasprintf(GFP_KERNEL, "pciclass,%04x", pdev->class >> 8);
> 
>      ret = of_changeset_add_prop_string_array(ocs, np, "compatible",
>                                               compat_strs, PROP_COMPAT_NUM);
>      ...
> }
> 
> If kasprintf() returns NULL, the compat_strs array will contain NULL pointers.
> 
> When this unvalidated array is passed to
> of_changeset_add_prop_string_array(), could it lead to a kernel crash when
> the string array is processed and strlen() is called on a NULL element?
> 

[PATCH 2/4] PCI: of: don't zero flags in of_pci_get_addr_flags()

[Alex Elder]

The flags variable whose address is passed to of_pci_get_addr_flags()
is zeroed before assigning a value to it.  Skip the zeroing and just
assign it instead.

Reviewed-by: Herve Codina <herve.codina@bootlin.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/pci/of_property.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 505226b876c56..22fea1905a080 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -82,12 +82,10 @@ static int of_pci_get_addr_flags(const struct resource *res, u32 *flags)
 	else
 		return -EINVAL;
 
-	*flags = 0;
+	*flags = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss);
 	if (res->flags & IORESOURCE_PREFETCH)
 		*flags |= OF_PCI_ADDR_FIELD_PREFETCH;
 
-	*flags |= FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss);
-
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH 2/4] PCI: of: don't zero flags in of_pci_get_addr_flags()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The function `of_pci_is_range_resource()` incorrectly evaluates `IORESOURCE_MEM_64` using the `resource_type()` macro. The `resource_type(res)` macro applies the `IORESOURCE_TYPE_BITS` mask (`0x1f00`). Because `IORESOURCE_MEM_64` is defined as `0x00100000`, it falls completely outside this mask. Consequently, `resource_type(res) & IORESOURCE_MEM_64` evaluates to a constant 0.

As a result, the condition `!(resource_type(res) & IORESOURCE_MEM_64)` is unconditionally `true`. This simplifies the entire filter condition to `!(resource_type(res) & IORESOURCE_MEM)`. Any resource that does not have the `IORESOURCE_MEM` bit set—such as `IORESOURCE_IO` (`0x100`)—will cause the condition to evaluate to true, making the function return `false`.

This logic flaw inadvertently filters out `IORESOURCE_IO` resources entirely, which prevents PCI host bridges from exposing IO space mappings via the dynamically generated device tree `ranges` property. Furthermore, if a 64-bit memory resource were defined with `IORESOURCE_MEM_64` but without `IORESOURCE_MEM`, it would also be incorrectly rejected.
--

--- Patch [2]: [PATCH 2/4] PCI: of: don't zero flags in of_pci_get_addr_flags() ---
commit d6dba8ef5457a1efb23f2b68f270883ef856bbdb
Author: Alex Elder <elder@riscstar.com>

PCI: of: don't zero flags in of_pci_get_addr_flags()

The flags variable whose address is passed to of_pci_get_addr_flags()
is zeroed before assigning a value to it. Skip the zeroing and just
assign it instead.

> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index 505226b876c56..22fea1905a080 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c

[ ... ]

> static bool of_pci_is_range_resource(const struct resource *res, u32 *flags)
> {
[ ... ]
> 	if (!(resource_type(res) & IORESOURCE_MEM) &&
> 	    !(resource_type(res) & IORESOURCE_MEM_64))
> 		return false;

[Severity: High]
This is a pre-existing issue, but does this correctly evaluate
IORESOURCE_MEM_64?

The resource_type() macro applies the IORESOURCE_TYPE_BITS mask (0x1f00).
Because IORESOURCE_MEM_64 is defined as 0x00100000, it falls outside this
mask, causing resource_type(res) & IORESOURCE_MEM_64 to evaluate to a
constant 0.

This makes the condition !(resource_type(res) & IORESOURCE_MEM_64)
unconditionally true. This simplifies the logic to reject any resource
that does not have the IORESOURCE_MEM bit set, such as IORESOURCE_IO.

Could this inadvertently filter out IORESOURCE_IO resources entirely when
called from of_pci_host_bridge_prop_ranges()?

drivers/pci/of_property.c:of_pci_host_bridge_prop_ranges() {
	...
	resource_list_for_each_entry(window, &bridge->windows) {
		res = window->res;
		if (!of_pci_is_range_resource(res, &flags))
			continue;
	...
}

If IO windows are wrongly skipped here, will dynamically created PCI host
bridge device tree nodes lack IO mappings in their ranges property?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807194100.455599-1-elder@riscstar.com?part=2

Re: [PATCH 2/4] PCI: of: don't zero flags in of_pci_get_addr_flags()

[Alex Elder]

On 8/7/26 2:56 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The function `of_pci_is_range_resource()` incorrectly evaluates `IORESOURCE_MEM_64` using the `resource_type()` macro. The `resource_type(res)` macro applies the `IORESOURCE_TYPE_BITS` mask (`0x1f00`). Because `IORESOURCE_MEM_64` is defined as `0x00100000`, it falls completely outside this mask. Consequently, `resource_type(res) & IORESOURCE_MEM_64` evaluates to a constant 0.
> 
> As a result, the condition `!(resource_type(res) & IORESOURCE_MEM_64)` is unconditionally `true`. This simplifies the entire filter condition to `!(resource_type(res) & IORESOURCE_MEM)`. Any resource that does not have the `IORESOURCE_MEM` bit set—such as `IORESOURCE_IO` (`0x100`)—will cause the condition to evaluate to true, making the function return `false`.
> 
> This logic flaw inadvertently filters out `IORESOURCE_IO` resources entirely, which prevents PCI host bridges from exposing IO space mappings via the dynamically generated device tree `ranges` property. Furthermore, if a 64-bit memory resource were defined with `IORESOURCE_MEM_64` but without `IORESOURCE_MEM`, it would also be incorrectly rejected.
> --
> 
> --- Patch [2]: [PATCH 2/4] PCI: of: don't zero flags in of_pci_get_addr_flags() ---
> commit d6dba8ef5457a1efb23f2b68f270883ef856bbdb
> Author: Alex Elder <elder@riscstar.com>
> 
> PCI: of: don't zero flags in of_pci_get_addr_flags()
> 
> The flags variable whose address is passed to of_pci_get_addr_flags()
> is zeroed before assigning a value to it. Skip the zeroing and just
> assign it instead.
> 
>> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
>> index 505226b876c56..22fea1905a080 100644
>> --- a/drivers/pci/of_property.c
>> +++ b/drivers/pci/of_property.c
> 
> [ ... ]
> 
>> static bool of_pci_is_range_resource(const struct resource *res, u32 *flags)
>> {
> [ ... ]
>> 	if (!(resource_type(res) & IORESOURCE_MEM) &&
>> 	    !(resource_type(res) & IORESOURCE_MEM_64))
>> 		return false;
> 
> [Severity: High]
> This is a pre-existing issue, but does this correctly evaluate
> IORESOURCE_MEM_64?

It is incorrect to apply the IORESOURCE_MEM_64 flag mask
to the result of resource_type().

> The resource_type() macro applies the IORESOURCE_TYPE_BITS mask (0x1f00).
> Because IORESOURCE_MEM_64 is defined as 0x00100000, it falls outside this
> mask, causing resource_type(res) & IORESOURCE_MEM_64 to evaluate to a
> constant 0.
> 
> This makes the condition !(resource_type(res) & IORESOURCE_MEM_64)
> unconditionally true. This simplifies the logic to reject any resource
> that does not have the IORESOURCE_MEM bit set, such as IORESOURCE_IO.
> 
> Could this inadvertently filter out IORESOURCE_IO resources entirely when
> called from of_pci_host_bridge_prop_ranges()?

No, this is not an issue because the purpose of this test was
to make the function only apply to memory resources (MEM or MEM64).
It's *possible* it will skip MEM64 resources, though in some cases,
IORESOURCE_MEM and IORESOURCE_MEM_64 are *both* set in the flags
field (see decode_bar()).

So I don't think there is an issue with skipping I/O resources,
but as stated above, this bit of code is at least wrong, and
it *could* be wrong in a way that affects correct behavior.

This function is only called by of_pci_host_bridge_prop_ranges().
I'll create a fix that at least fixes the bogus test, and in
the process will try to confirm that the result is correct.

					-Alex

> drivers/pci/of_property.c:of_pci_host_bridge_prop_ranges() {
> 	...
> 	resource_list_for_each_entry(window, &bridge->windows) {
> 		res = window->res;
> 		if (!of_pci_is_range_resource(res, &flags))
> 			continue;
> 	...
> }
> 
> If IO windows are wrongly skipped here, will dynamically created PCI host
> bridge device tree nodes lack IO mappings in their ranges property?
> 

[PATCH 3/4] PCI: of: make a flags argument optional

[Alex Elder]

The address of a u32 object is passed to of_pci_get_addr_flags() so
it can be filled with the computed flags value.

Allow a null pointer to be passed, so that the validity of the
resource's flags can be checked without filling in the flags value.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/pci/of_property.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 22fea1905a080..5e1820b8bb79b 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -82,9 +82,11 @@ static int of_pci_get_addr_flags(const struct resource *res, u32 *flags)
 	else
 		return -EINVAL;
 
-	*flags = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss);
-	if (res->flags & IORESOURCE_PREFETCH)
-		*flags |= OF_PCI_ADDR_FIELD_PREFETCH;
+	if (flags) {
+		*flags = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss);
+		if (res->flags & IORESOURCE_PREFETCH)
+			*flags |= OF_PCI_ADDR_FIELD_PREFETCH;
+	}
 
 	return 0;
 }
-- 
2.53.0

Re: [PATCH 3/4] PCI: of: make a flags argument optional

[sashiko-bot]

> The address of a u32 object is passed to of_pci_get_addr_flags() so
> it can be filled with the computed flags value.
> 
> Allow a null pointer to be passed, so that the validity of the
> resource's flags can be checked without filling in the flags value.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807194100.455599-1-elder@riscstar.com?part=3

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

[Alex Elder]

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>
---
 drivers/pci/bus.c |  1 +
 drivers/pci/of.c  | 27 +++++++++++++++++++++++++++
 drivers/pci/pci.h |  2 ++
 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..b51bb14857e9e 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -663,6 +663,33 @@ void of_pci_remove_node(struct pci_dev *pdev)
 	of_node_put(np);
 }
 
+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;
+
+	/*
+	 * 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.
+	 */
+	if (pci_is_bridge(pdev))
+		return;
+
+	/*
+	 * 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");
+}
+
 void of_pci_make_dev_node(struct pci_dev *pdev)
 {
 	struct device_node *ppnode, *np = NULL;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4469e1a77f3c1..6c19895c14914 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -1265,6 +1265,7 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
 struct of_changeset;
 
 #ifdef CONFIG_PCI_DYNAMIC_OF_NODES
+void of_pci_verify_node(struct pci_dev *pdev);
 void of_pci_make_dev_node(struct pci_dev *pdev);
 void of_pci_remove_node(struct pci_dev *pdev);
 int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
@@ -1275,6 +1276,7 @@ int of_pci_add_host_bridge_properties(struct pci_host_bridge *bridge,
 				      struct of_changeset *ocs,
 				      struct device_node *np);
 #else
+static inline void of_pci_verify_node(struct pci_dev *pdev) { }
 static inline void of_pci_make_dev_node(struct pci_dev *pdev) { }
 static inline void of_pci_remove_node(struct pci_dev *pdev) { }
 static inline void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge) { }
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The static DT node verification function `of_pci_verify_node()` is mistakenly placed under `#ifdef CONFIG_PCI_DYNAMIC_OF_NODES`, making it a silent no-op on systems that only use statically-defined nodes.
--

--- Patch [4]: [PATCH 4/4] PCI: of: introduce of_pci_verify_node() ---
commit 29170ab92dd207c740eed74438cd2e09bad49131
Author: Alex Elder <elder@riscstar.com>

PCI: of: introduce of_pci_verify_node()

This commit introduces a runtime check to verify that statically-defined
(pre-existing) devicetree nodes for non-bridge PCI endpoints do not
incorrectly contain the device_type = "pci" property. It adds a call to
of_pci_verify_node() during PCI bus device addition to catch these errors.

> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1265,6 +1265,7 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
>  struct of_changeset;
>  
>  #ifdef CONFIG_PCI_DYNAMIC_OF_NODES
> +void of_pci_verify_node(struct pci_dev *pdev);

[Severity: Medium]
Since the intent is to verify statically-defined devicetree nodes, will
placing this declaration inside the CONFIG_PCI_DYNAMIC_OF_NODES block
accidentally disable the check on platforms that only use static nodes?

If CONFIG_PCI_DYNAMIC_OF_NODES is disabled, this falls back to the empty
stub below, causing the verification to be silently skipped.

>  void of_pci_make_dev_node(struct pci_dev *pdev);
>  void of_pci_remove_node(struct pci_dev *pdev);

[ ... ]

>  #else
> +static inline void of_pci_verify_node(struct pci_dev *pdev) { }
>  static inline void of_pci_make_dev_node(struct pci_dev *pdev) { }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807194100.455599-1-elder@riscstar.com?part=4

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

[Alex Elder]

On 8/7/26 2:49 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The static DT node verification function `of_pci_verify_node()` is mistakenly placed under `#ifdef CONFIG_PCI_DYNAMIC_OF_NODES`, making it a silent no-op on systems that only use statically-defined nodes.

This is a good observation.  I think it's correct that
this check is worth doing on static DT nodes even if
CONFIG_PCI_DYNAMIC_OF_NODES is not enabled.

I will plan to send v2 of this series with this
suggestion implemented.

					-Alex
> --
> 
> --- Patch [4]: [PATCH 4/4] PCI: of: introduce of_pci_verify_node() ---
> commit 29170ab92dd207c740eed74438cd2e09bad49131
> Author: Alex Elder <elder@riscstar.com>
> 
> PCI: of: introduce of_pci_verify_node()
> 
> This commit introduces a runtime check to verify that statically-defined
> (pre-existing) devicetree nodes for non-bridge PCI endpoints do not
> incorrectly contain the device_type = "pci" property. It adds a call to
> of_pci_verify_node() during PCI bus device addition to catch these errors.
> 
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -1265,6 +1265,7 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
>>   struct of_changeset;
>>   
>>   #ifdef CONFIG_PCI_DYNAMIC_OF_NODES
>> +void of_pci_verify_node(struct pci_dev *pdev);
> 
> [Severity: Medium]
> Since the intent is to verify statically-defined devicetree nodes, will
> placing this declaration inside the CONFIG_PCI_DYNAMIC_OF_NODES block
> accidentally disable the check on platforms that only use static nodes?
> 
> If CONFIG_PCI_DYNAMIC_OF_NODES is disabled, this falls back to the empty
> stub below, causing the verification to be silently skipped.
> 
>>   void of_pci_make_dev_node(struct pci_dev *pdev);
>>   void of_pci_remove_node(struct pci_dev *pdev);
> 
> [ ... ]
> 
>>   #else
>> +static inline void of_pci_verify_node(struct pci_dev *pdev) { }
>>   static inline void of_pci_make_dev_node(struct pci_dev *pdev) { }
>