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] Leaked OF node reference counts in of_pci_prop_intr_map() --

commit b3af32f45eed6da355a4a96246bdedb364fe0855
Author: Alex Elder <[email protected]>

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.

Reviewed-by Herve Codina <[email protected]>

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

Quoted 4 lines

drivers/pci/of_property.c +0 -0

index d66c702218081..1caabbd4c18b5 100644

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this code leak device node reference counts in of_pci_prop_intr_map()?

When of_irq_parse_raw() successfully parses an interrupt, it acquires a device node reference and stores it in out_irq[i].np:

drivers/pci/of_property.c:of_pci_prop_intr_map() {
     ...
		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;
		}
     ...

However, the function returns without calling of_node_put() on the successfully parsed nodes in the out_irq array. This leak occurs on all return paths, such as when map_sz is zero or when kcalloc() fails:

     ...
	if (!map_sz)
		return 0;

	int_map = kcalloc(map_sz, sizeof(u32), GFP_KERNEL);
	if (!int_map)
		return -ENOMEM;
     ...

Are we missing calls to of_node_put() for these references before returning?

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". We also accept "pciex" as the value of the device_type property for bridges.

Reviewed-by Herve Codina <[email protected]>

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

---
 drivers/pci/bus.c |  1 +
 drivers/pci/of.c  | 32 ++++++++++++++++++++++++++++++++
 drivers/pci/pci.h |  3 +++
 3 files changed, 36 insertions(+)

drivers/pci/bus.c +1 -0

index 655ed53436d3e..679afbc6d3109 100644
@@ -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);

drivers/pci/of.c +32 -0

index a51dff91b196d..5a040ed836744 100644
@@ -1085,3 +1085,35 @@ 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
+ *
+ * PCI enumeration authoritatively discovers what we need to know about
+ * a PCI device.  A devicetree-based platform will represent a PCI root
+ * bridge with a node, but otherwise devicetree doesn't typically include
+ * many PCI nodes.  Where such nodes do exist, experience has shown that
+ * the "device_type" property is sometimes wrong, so warn about that.
+ */
+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;
+
+	device_is_bridge = pci_is_bridge(pdev);
+	device_type_pci = of_node_is_type(np, "pci") ||
+			  of_node_is_type(np, "pciex");
+
+	/* 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");
+}

drivers/pci/pci.h +3 -0

index ba3c3fddddc23..2e33d3bd4b0ba 100644
@@ -1253,6 +1253,7 @@ bool of_pci_supply_present(struct device_node *np);
 int of_pci_get_equalization_presets(struct device *dev,
 				    struct pci_eq_presets *presets,
 				    int num_lanes);
+void of_pci_verify_node(struct pci_dev *pdev);
 #else
 static inline int
 of_get_pci_domain_nr(struct device_node *node)

@@ -1308,6 +1309,8 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
 
 	return 0;
 }
+
+static inline void of_pci_verify_node(struct pci_dev *pdev) { }
 #endif /* CONFIG_OF */
 
 struct of_changeset;

Signature · 1 line

2.53.0

Quoted 29 lines · 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";

Quoted 7 lines

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". We also accept "pciex" as the value of the device_type property for bridges.

IIUC this patch basically adds a warning for ill-formed DTs but doesn't otherwise change any behavior. Obviously a developer could fix the DT, but what would a user do in response to the warning?

Is there something that doesn't work when this warning is emitted, and fixing the DT would make it work? If so, is there any hint that could be included in the warning? E.g., "DT device_type is missing; can't support X"?

The commit log doesn't actually say directly what the patch does. I asked gemini to work on it, and it produced this, which is straightforward but omits a lot of detail you might want:

PCI: of: Verify "device_type" property for PCI device nodes

According to the Open Firmware PCI Bus Supplement and Devicetree Specification, the device_type property is reserved strictly for PCI bridge nodes (with value "pci" or "pciex"). Non-bridge PCI endpoint nodes must not include a device_type property.

Pre-defined Device Tree nodes representing PCI devices sometimes violate this requirement—such as endpoint nodes incorrectly specifying device_type = "pci", or bridge nodes omitting device_type altogether.

Introduce of_pci_verify_node() to sanity-check pre-defined Device Tree nodes assigned to PCI devices as they are registered in pci_bus_add_device():

- Warn if a PCI bridge node lacks a "pci" or "pciex" device_type property.

- Warn if a non-bridge PCI endpoint node contains a device_type property.

I'd like to get Rob's Reviewed-by since I'm not really a DT expert.

Quoted 85 lines

Reviewed-by Herve Codina <[email protected]>

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

---
  drivers/pci/bus.c |  1 +
  drivers/pci/of.c  | 32 ++++++++++++++++++++++++++++++++
  drivers/pci/pci.h |  3 +++
  3 files changed, 36 insertions(+)

drivers/pci/bus.c +1 -0

index 655ed53436d3e..679afbc6d3109 100644
@@ -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);

drivers/pci/of.c +32 -0

index a51dff91b196d..5a040ed836744 100644
@@ -1085,3 +1085,35 @@ 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
+ *
+ * PCI enumeration authoritatively discovers what we need to know about
+ * a PCI device.  A devicetree-based platform will represent a PCI root
+ * bridge with a node, but otherwise devicetree doesn't typically include
+ * many PCI nodes.  Where such nodes do exist, experience has shown that
+ * the "device_type" property is sometimes wrong, so warn about that.
+ */
+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;
+
+	device_is_bridge = pci_is_bridge(pdev);
+	device_type_pci = of_node_is_type(np, "pci") ||
+			  of_node_is_type(np, "pciex");
+
+	/* 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");
+}

drivers/pci/pci.h +3 -0

index ba3c3fddddc23..2e33d3bd4b0ba 100644
@@ -1253,6 +1253,7 @@ bool of_pci_supply_present(struct device_node *np);
  int of_pci_get_equalization_presets(struct device *dev,
  				    struct pci_eq_presets *presets,
  				    int num_lanes);
+void of_pci_verify_node(struct pci_dev *pdev);
  #else
  static inline int
  of_get_pci_domain_nr(struct device_node *node)

@@ -1308,6 +1309,8 @@ static inline int of_pci_get_equalization_presets(struct device *dev,
 
  	return 0;
  }
+
+static inline void of_pci_verify_node(struct pci_dev *pdev) { }
  #endif /* CONFIG_OF */
 
  struct of_changeset;

Signature · 1 line

2.53.0