PCI: of: warn on bogus device_type property

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

Pre-existing issues:
- [High] Potential NULL pointer dereference in of_pci_prop_compatible if kasprintf fails.

--

commit 036872e87dc6e9201c6f86349afb91815a638d1e
Author: Alex Elder <[email protected]>

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.

Quoted 7 lines

drivers/pci/of_property.c +0 -0

index 505226b876c56..22fea1905a080 100644
@@ -82,12 +82,10 @@ static int of_pci_get_addr_flags(const struct resource *res, u32 *flags)
  	else
  		return -EINVAL;

drivers/pci/of_property.c static int of_pci_get_addr_flags(const struct resource *res, u32 *flags) · line 90

Quoted 10 lines

drivers/pci/of_property.c +1 -3

 
-	*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;
  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential null pointer dereference in this file under low memory conditions.

Looking at of_pci_prop_compatible(), kasprintf() can fail and return a null pointer:

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);
	...
	ret = of_changeset_add_prop_string_array(ocs, np, "compatible",
						 compat_strs, PROP_COMPAT_NUM);
	...
}

If kasprintf() fails, the compat_strs array will contain a null pointer. When this array is passed to of_changeset_add_prop_string_array(), it unconditionally dereferences the strings to calculate their lengths:

drivers/of/dynamic.c:of_changeset_add_prop_string_array() {
	...
	prop.length = 0;
	for (i = 0; i < sz; i++)
		prop.length += strlen(str_array[i]) + 1;
	...
}

Would it be better to add a check for allocation failures in of_pci_prop_compatible() before passing the array to of_changeset_add_prop_string_array() to prevent this?

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The check in of_pci_verify_node() falsely flags compliant CardBus and PowerPC PCIe bridges as having an invalid device_type.

--

commit 72a980b9469901bec37d04c04dd8de642179aa44
Author: Alex Elder <[email protected]>

PCI: of: introduce of_pci_verify_node()

This commit introduces a runtime check to verify PCI devicetree nodes. It ensures that bridge devices have the "pci" device_type property and that endpoint devices do not.

Quoted 4 lines

drivers/pci/of.c +0 -0

index a51dff91b196d..1838a3109514c 100644

[ ... ]

Quoted 8 lines

drivers/pci/of.c +4 -0

@@ -1085,3 +1085,34 @@ int of_pci_get_equalization_presets(struct device *dev,
  	return 0;
  }
  EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
+
+/**
+ * of_pci_verify_node - Sanity check some PCI device node properties
+ * @pdev: The PCI device whose device node is checked

[ ... ]

Quoted 9 lines

drivers/pci/of.c +9 -0

+void of_pci_verify_node(struct pci_dev *pdev)
+{
+	struct device_node *np = pci_device_to_OF_node(pdev);
+	bool device_is_bridge;
+	bool device_type_pci;
+
+	/* Nothing to check if there's no pre-existing devicetree node */
+	if (!np)
+		return;

drivers/pci/of.c int of_pci_get_equalization_presets(struct device *dev, · line 1117

Quoted 10 lines

drivers/pci/of.c +10 -0

+
+	device_is_bridge = pci_is_bridge(pdev);
+	device_type_pci = of_node_is_type(np, "pci");
+
+	/* Bridges should have device type "pci"; endpoints should not */
+	if (device_is_bridge == device_type_pci)
+		return;
+
+	dev_err(&pdev->dev, "PCI %s have \"pci\" device_type property\n",
+		device_is_bridge ? "bridge should" : "endpoint should not");

[Severity: Medium]
Will this falsely flag compliant CardBus and PowerPC PCIe bridges as having an invalid device type?

The check in of_pci_verify_node() expects all bridges to have a device type of exactly "pci". However, valid PCI bridges can legitimately use "cardbus" or "pciex" as their device_type property.

When a PCI bridge with one of these valid types is added, the following call chain unconditionally triggers this error:

pci_bus_add_device()
of_pci_verify_node()

This could emit a false dev_err() during boot on platforms using these standard bridges, which might alarm users by polluting the kernel logs.

Quoted 1 line

+}

Quoted 2 lines · On 8/31/26 8:21 PM, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The check in of_pci_verify_node() falsely flags compliant CardBus and PowerPC PCIe bridges as having an invalid device_type.

I was unfortunately not considering those types of bridges, and don't have experience with (or access to) either.

In the devicetree code, of_bus_pci_match() does check for device_type "pciex" but does not handle "cardbus" (at least, not explicitly--the "default-flags" bus type might do the right thing).

I would love to get some pointers from others on how to do this correctly, but in any case I'll do a little more research on this before I post version 4.

Quoted 12 lines · Alex Elder

Quoted 2 lines · On 8/31/26 8:21 PM, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The check in of_pci_verify_node() falsely flags compliant CardBus and PowerPC PCIe bridges as having an invalid device_type.

I was unfortunately not considering those types of bridges, and don't have experience with (or access to) either.

In the devicetree code, of_bus_pci_match() does check for device_type "pciex" but does not handle "cardbus" (at least, not explicitly--the "default-flags" bus type might do the right thing).

Quoted 3 lines

I would love to get some pointers from others on how to do this correctly, but in any case I'll do a little more research on this before I post version 4.

I'm willing to wait and see if anyone cares about cardbus. I doubt it. Maybe there's some old PowerBooks with cardbus?

"pciex" should probably be handled.

Rob

Quoted 6 lines

Quoted 3 lines

I would love to get some pointers from others on how to do this correctly, but in any case I'll do a little more research on this before I post version 4.

I'm willing to wait and see if anyone cares about cardbus. I doubt it. Maybe there's some old PowerBooks with cardbus?

I'll give it another few days before posting v4.

Quoted 1 line

"pciex" should probably be handled.

I'll add that.  I'll also try to find what the range of "compliant" device types is; I presume it's in the devicetree spec.

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. Herve Codina further suggested we ensure that bridge PCI devices *do* define the device_type property, with value "pci".

Signed-off-by Alex Elder <[email protected]>

---
v3: - Switch a function header to use kernel-doc format
- Add a warning if a PCI bridge node has no device_type property

 drivers/pci/bus.c |  1 +
 drivers/pci/of.c  | 31 +++++++++++++++++++++++++++++++
 drivers/pci/pci.h |  3 +++
 3 files changed, 35 insertions(+)

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Outdated comment in of_bus_pci_match() incorrectly claims a warning is issued. --

commit 0ffaade5a7eebd20cc4d79d7c8ed0d49f6da1857
Author: Alex Elder <[email protected]>

of: address: kill of_node_is_pcie()

The of_bus->match function for the "PCI" bus type is fairly liberal in what it accepts as a PCI bus devicetree node. If a node has no device_type property, it even allows a node named "pcie@" to be accepted as represnting a devicetree bus, though it issues a warning in that case.

Quoted 4 lines

drivers/of/address.c +0 -0

index 499d37ceae210..ee2eb44884d85 100644

[ ... ]

drivers/of/address.c static int of_bus_pci_match(struct device_node *np) · line 150

Quoted 11 lines

drivers/of/address.c +1 -1

@@ -156,7 +146,7 @@ static int of_bus_pci_match(struct device_node *np)
  	 * "ht" is hypertransport
  	 *
  	 * If none of the device_type match, and that the node name is
  	 * "pcie", accept the device as PCI (with a warning).
  	 */
  	return of_node_is_type(np, "pci") || of_node_is_type(np, "pciex") ||
  		of_node_is_type(np, "vci") || of_node_is_type(np, "ht") ||
-		of_node_is_pcie(np);
+		of_node_name_eq(np, "pcie");
  }

[Severity: Low]
Does the comment block above this return statement need to be updated?

Since the patch replaces the call to of_node_is_pcie() with a direct check of the node name to avoid duplicate warnings, the comment in of_bus_pci_match() claiming that it accepts the device "(with a warning)" seems to be a minor documentation regression.

Could we update the comment to reflect the new behavior?

The of_bus->match function for the "PCI" bus type is fairly liberal in what it accepts as a PCI bus devicetree node. If a node has no device_type property, it even allows a node named "pcie@" to be accepted as represnting a devicetree bus, though it issues a warning in that case.

A recent PCI commit introduced of_pci_verify_node(). When a PCI device is added, if it has a devicetree node, that function checks its device_type property. For PCI bridge devices, if there is no device_type property (value "pci"), a warning is issued.

That warning duplicates the warning made by of_node_is_pcie(), and there's no point in that. Avoid the second (OF) warning by just checking the node name directly in of_bus_pci_match().

That leaves of_node_is_pcie() unused, so get rid of it.

Signed-off-by Alex Elder <[email protected]>

---
v3: - Added (new) in this version of the series

 drivers/of/address.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

drivers/of/address.c +0 -10

index 499d37ceae210..ee2eb44884d85 100644
@@ -134,16 +134,6 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
  * PCI bus specific translator
  */
 
-static bool of_node_is_pcie(const struct device_node *np)
-{
-	bool is_pcie = of_node_name_eq(np, "pcie");
-
-	if (is_pcie)
-		pr_warn_once("%pOF: Missing device_type\n", np);
-
-	return is_pcie;
-}
-
 static int of_bus_pci_match(struct device_node *np)
 {
 	/*

Hi Alex,

Quoted 53 lines · On Mon, 31 Aug 2026 20:13:37 -0500 Alex Elder <[email protected]> wrote:

The of_bus->match function for the "PCI" bus type is fairly liberal in what it accepts as a PCI bus devicetree node. If a node has no device_type property, it even allows a node named "pcie@" to be accepted as represnting a devicetree bus, though it issues a warning in that case.

A recent PCI commit introduced of_pci_verify_node(). When a PCI device is added, if it has a devicetree node, that function checks its device_type property. For PCI bridge devices, if there is no device_type property (value "pci"), a warning is issued.

That warning duplicates the warning made by of_node_is_pcie(), and there's no point in that. Avoid the second (OF) warning by just checking the node name directly in of_bus_pci_match().

That leaves of_node_is_pcie() unused, so get rid of it.

Signed-off-by Alex Elder <[email protected]>

---
v3: - Added (new) in this version of the series

  drivers/of/address.c | 12 +-----------
  1 file changed, 1 insertion(+), 11 deletions(-)

drivers/of/address.c +1 -11

index 499d37ceae210..ee2eb44884d85 100644
@@ -134,16 +134,6 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
   * PCI bus specific translator
   */
 
-static bool of_node_is_pcie(const struct device_node *np)
-{
-	bool is_pcie = of_node_name_eq(np, "pcie");
-
-	if (is_pcie)
-		pr_warn_once("%pOF: Missing device_type\n", np);
-
-	return is_pcie;
-}
-
  static int of_bus_pci_match(struct device_node *np)
  {
  	/*

@@ -156,7 +146,7 @@ static int of_bus_pci_match(struct device_node *np)
  	 */
  	return of_node_is_type(np, "pci") || of_node_is_type(np, "pciex") ||
  		of_node_is_type(np, "vci") || of_node_is_type(np, "ht") ||
-		of_node_is_pcie(np);
+		of_node_name_eq(np, "pcie");
  }
 

drivers/of/address.c static int of_bus_pci_match(struct device_node *np) · line 152

Quoted 1 line

drivers/of/address.c +0 -0

  static void of_bus_pci_count_cells(struct device_node *np,

The warning here was printed based on the node name whereas of_node_is_pcie() prints the message based on the 'device_type' property of a pci_dev node.

For PCI to PCI bridges, no problem the warning is indeed duplicated but what happens for the PCI host controller?

PCI host controller drivers calls pci_host_probe() and are seen by the PCI core as a struct pci_host_bridge.

of_node_is_pcie() is called for children of the PCI host controller (i.e. PCI devices scanned on the PCI bus handled by the host controller) but not for the PCI host controller itself.

The OF node of the host controller must have the 'device_type' property set to "pci".

I am not so sure that this warning was duplicated when we consider the PCI host controller node.

Can you double check on your side?

Best regards,
Hervé

Quoted 58 lines · Herve Codina

Hi Alex,

Quoted 53 lines · On Mon, 31 Aug 2026 20:13:37 -0500 Alex Elder <[email protected]> wrote:

The of_bus->match function for the "PCI" bus type is fairly liberal in what it accepts as a PCI bus devicetree node. If a node has no device_type property, it even allows a node named "pcie@" to be accepted as represnting a devicetree bus, though it issues a warning in that case.

A recent PCI commit introduced of_pci_verify_node(). When a PCI device is added, if it has a devicetree node, that function checks its device_type property. For PCI bridge devices, if there is no device_type property (value "pci"), a warning is issued.

That warning duplicates the warning made by of_node_is_pcie(), and there's no point in that. Avoid the second (OF) warning by just checking the node name directly in of_bus_pci_match().

That leaves of_node_is_pcie() unused, so get rid of it.

Signed-off-by Alex Elder <[email protected]>

---
v3: - Added (new) in this version of the series

  drivers/of/address.c | 12 +-----------
  1 file changed, 1 insertion(+), 11 deletions(-)

drivers/of/address.c +1 -11

index 499d37ceae210..ee2eb44884d85 100644
@@ -134,16 +134,6 @@ static unsigned int of_bus_pci_get_flags(const __be32 *addr)
   * PCI bus specific translator
   */
 
-static bool of_node_is_pcie(const struct device_node *np)
-{
-	bool is_pcie = of_node_name_eq(np, "pcie");
-
-	if (is_pcie)
-		pr_warn_once("%pOF: Missing device_type\n", np);
-
-	return is_pcie;
-}
-
  static int of_bus_pci_match(struct device_node *np)
  {
  	/*

@@ -156,7 +146,7 @@ static int of_bus_pci_match(struct device_node *np)
  	 */
  	return of_node_is_type(np, "pci") || of_node_is_type(np, "pciex") ||
  		of_node_is_type(np, "vci") || of_node_is_type(np, "ht") ||
-		of_node_is_pcie(np);
+		of_node_name_eq(np, "pcie");
  }
 

Quoted 4 lines

drivers/of/address.c static int of_bus_pci_match(struct device_node *np) · line 152

Quoted 1 line

drivers/of/address.c +0 -0

  static void of_bus_pci_count_cells(struct device_node *np,  

The warning here was printed based on the node name whereas of_node_is_pcie() prints the message based on the 'device_type' property of a pci_dev node.

You're right. Both warnings were getting reported for me, but for different reasons.

I would be happy to just drop this patch and live with two warnings in some cases (we should rarely see either one of them anyway, right?).

Is that OK with you? Does anyone feel this patch should be kept?

Quoted 9 lines

Quoted 2 lines

The warning here was printed based on the node name whereas of_node_is_pcie() prints the message based on the 'device_type' property of a pci_dev node.

You're right. Both warnings were getting reported for me, but for different reasons.

I would be happy to just drop this patch and live with two warnings in some cases (we should rarely see either one of them anyway, right?).

Is that OK with you? Does anyone feel this patch should be kept?

On my side, I have no problem to see two warnings and so, IHMO, this patch can be dropped.