Skip to contentSkip to conversation

PCI: of: warn on bogus device_type property

PATCH New Alex Elder 2 participants 9 messages Nov 5 PCI

lore
Share & tools
lore

Alex Elder

author
#0 lore raw

v7 cover PCI: of: warn on bogus device_type property

*** Bjorn seems happy with this series, and Herve has reviewed *** *** almost all of it, but we'd prefer to get an ACK from Rob ***

Add a check when adding a PCI device to ensure the device_type property is (or is not) correctly defined when the device has a non-null devicetree node pointer.

This series also includes three other minor cleanups to the PCI devicetree code. The first patch prevents a possible null pointer dereference that Sashiko pointed out some time back. The second and third patches are simple cleanups. The last adds the new PCI devicetree node checks and warnings.

-Alex

Between version 6 and version 7:
- Simplified the wording on this cover letter
- No longer check for snprintf() overflow in of_pci_prop_compatible()
- Rebased onto 7.4-rc1

Version 6 is available here:
https://lore.kernel.org/lkml/[email protected]/

Between version 5 and version 6:
- Simplified the patch header on the last patch, as suggested by Bjorn
- Rebased on v7.3-rc4; otherwise no code changes

Version 5 is available here:
https://lore.kernel.org/lkml/[email protected]/

Between version 4 and version 5:
- Made a local variable (bufp) a pointer type in the first patch
- Eliminated an unused local variable (i) in the first patch

Version 4 is available here:
https://lore.kernel.org/lkml/[email protected]/

Between version 3 and version 4:
- Insert a first patch that fixes a Sashiko-reported issue
- Drop the final "duplicate warning" patch from v3
- Include "pciex" as a valid PCI bridge device_type property value
- Add Herve's Reviewed-by tag on the last patch

Version 3 is available here:
https://lore.kernel.org/lkml/[email protected]/

Between version 2 and version 3:
- Drop a patch that made a change only needed by a different series
- Switch a function header to use kernel-doc format
- Add a warning if a PCI bridge node has no device_type property
- Added a patch to remove a duplicate warning in the devicetree code

Version 2 is available here:
https://lore.kernel.org/lkml/[email protected]/

Between version 1 and version 2:
- Check the PCI devicetree node even when PCI_DYNAMIC_OF_NODES is not enabled

Version 1 is available here:
https://lore.kernel.org/lkml/[email protected]/

Alex Elder (4):
PCI: of: avoid allocations in of_pci_prop_compatible()
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: introduce of_pci_verify_node()

 drivers/pci/bus.c         |  1 +
 drivers/pci/of.c          | 32 +++++++++++++++++++++++++++++
 drivers/pci/of_property.c | 43 +++++++++++++++++++++------------------
 drivers/pci/pci.h         |  3 +++
 4 files changed, 59 insertions(+), 20 deletions(-)

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

Signature · 1 line
2.53.0
author
#1 lore raw

PATCH v7 1/4 PCI: of: avoid allocations in of_pci_prop_compatible()

Three compatible strings are formatted in of_pci_prop_compatible(). Their sizes are known in advance, and the largest is 16 bytes.

Rather than dynamically allocating the space for those strings, just set aside a buffer on the stack large enough to hold all three.

This avoids a problem that Sashiko pointed out, where an allocation failure would cause subsequent crash because strlen() is called unconditionally in of_changeset_add_prop_string_array().

---
v7: - Don't check for snprintf() buffer overflow (buffer is big enough)

 drivers/pci/of_property.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)
drivers/pci/of_property.c +17 -12
index 1500740cc55d2..d351f500c2ce1 100644
@@ -331,27 +331,32 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
 	return ret;
 }
 
+/* The three compatible property strings have max sizes 12+1, 15+1, and 13+1 */
+#define PROP_SIZE	16	/* Enough for any of the three strings */
 static int of_pci_prop_compatible(struct pci_dev *pdev,
 				  struct of_changeset *ocs,
 				  struct device_node *np)
 {
 	const char *compat_strs[PROP_COMPAT_NUM] = { 0 };
-	int i, ret;
+	char buf[PROP_COMPAT_NUM * PROP_SIZE] = { 0 };
+	char *bufp = buf;
+	int ret;
 
-	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 = snprintf(bufp, PROP_SIZE, "pci%x,%x", pdev->vendor, pdev->device);
+	compat_strs[PROP_COMPAT_PCI_VVVV_DDDD] = bufp;
+	bufp += ret + 1;
 
-	ret = of_changeset_add_prop_string_array(ocs, np, "compatible",
-						 compat_strs, PROP_COMPAT_NUM);
-	for (i = 0; i < PROP_COMPAT_NUM; i++)
-		kfree(compat_strs[i]);
+	ret = snprintf(bufp, PROP_SIZE, "pciclass,%06x", pdev->class);
+	compat_strs[PROP_COMPAT_PCICLASS_CCSSPP] = bufp;
+	bufp += ret + 1;
 
-	return ret;
+	ret = snprintf(bufp, PROP_SIZE, "pciclass,%04x", pdev->class >> 8);
+	compat_strs[PROP_COMPAT_PCICLASS_CCSS] = bufp;
+
+	return of_changeset_add_prop_string_array(ocs, np, "compatible",
+						  compat_strs, PROP_COMPAT_NUM);
 }
+#undef PROP_SIZE
 
 int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
 			  struct device_node *np)
Signature · 1 line
2.53.0
#2 lore raw

Re: [PATCH v7 1/4] PCI: of: avoid allocations in of_pci_prop_compatible()

Sashiko has reviewed this patch and found no issues.
Quoted 6 lines

Three compatible strings are formatted in of_pci_prop_compatible(). Their sizes are known in advance, and the largest is 16 bytes.

Rather than dynamically allocating the space for those strings, just set aside a buffer on the stack large enough to hold all three.

Quoted 7 lines

This avoids a problem that Sashiko pointed out, where an allocation failure would cause subsequent crash because strlen() is called unconditionally in of_changeset_add_prop_string_array().

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author
#3 lore raw

PATCH v7 2/4 PCI: of: drop the reg_num argument to of_pci_set_address()

In reply to Alex Elder

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

---
 drivers/pci/of_property.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/pci/of_property.c +5 -5
index d351f500c2ce1..6236dfaaae628 100644
@@ -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);
@@ -135,7 +135,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,
@@ -168,7 +168,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));
@@ -470,7 +470,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 */
Signature · 1 line
2.53.0
#4 lore raw

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

Sashiko has reviewed this patch and found no issues.
Quoted 5 lines

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

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author
#5 lore raw

PATCH v7 3/4 PCI: of: don't zero flags in of_pci_get_addr_flags()

In reply to 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.

---
 drivers/pci/of_property.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/pci/of_property.c +1 -3
index 6236dfaaae628..702675672fbc5 100644
@@ -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;
 }
 
Signature · 1 line
2.53.0
#6 lore raw

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

Sashiko has reviewed this patch and found no issues.
Quoted 6 lines

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.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
author
#7 lore raw

PATCH v7 4/4 PCI: of: introduce of_pci_verify_node()

In reply to Alex Elder

The Open Firmware PCI Bus Supplement and Devicetree Specification reserves the device_type property for PCI bridge nodes (with value "pci" or "pciex"). Non-bridge PCI endpoint nodes must not include this property. Rob Herring observed that developers seem to get this wrong. Commit df4107fc729e3 ("arm64: dts: qcom: qcs6490-rb3gen2:
clean up PCI function nodes") and a few that follow demonstrate this.

Rob requested that a runtime check 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" or "pciex".

Implement these suggested warnings in of_pci_verify_node(), a new function called by pci_bus_add_device() for both bridges and endpoints. If a PCI bridge has a devicetree node, a warning is issued if it has a device_type property whose value is not "pci" or "pciex" (or if it has no such property). Similarly, a warning is issued for an endpoint if it has a device_type property having one of those two values.

To be clear, this adds warnings, but otherwise ignores the errors it warns about. It is meant to help developers; users should never see these messages.

---
 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 77efa4c5c960e..e93f602b9ca48 100644
@@ -1259,6 +1259,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)
@@ -1314,6 +1315,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
#8 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] PCI: of: of_pci_verify_node() defined outside CONFIG_PCI guard --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] PCI: of: of_pci_verify_node() defined outside CONFIG_PCI guard
--

commit 3ae9eba1c84ba51b7f2ee89655e65140eb2d2cd0
Author: Alex Elder <[email protected]>

PCI: of: introduce of_pci_verify_node()

This commit introduces of_pci_verify_node() and calls it from pci_bus_add_device() to check that devicetree nodes for PCI bridges have a device_type property of "pci" or "pciex", and that PCI endpoint nodes do not.

Quoted 30 lines
drivers/pci/of.c +22 -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);

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

Quoted 10 lines
drivers/pci/of.c +10 -0
+	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");
+}

[Severity: Low]
Should of_pci_verify_node() be placed inside the #ifdef CONFIG_PCI block in drivers/pci/of.c rather than at the end of the file?

drivers/pci/of.o is built whenever CONFIG_OF is enabled so that PCI endpoint controller drivers can use the exported devicetree helpers defined after the #endif /* CONFIG_PCI */ guard:

drivers/pci/of.c:

    bool of_pci_supply_present(struct device_node *np)
    {
        ...
    }
    EXPORT_SYMBOL_GPL(of_pci_supply_present);

    #endif /* CONFIG_PCI */

    int of_pci_get_max_link_speed(struct device_node *node)

Since of_pci_verify_node() is only called from pci_bus_add_device() in drivers/pci/bus.c (which is only built when CONFIG_PCI is enabled), defining it after #endif /* CONFIG_PCI */ causes it to be compiled as unused code when CONFIG_OF=y and CONFIG_PCI=n.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4