PCI: of: warn on bogus device_type property
v6 cover PCI: of: warn on bogus device_type property
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.
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. During review, Herve Codina also suggested that bridge nodes be checked to ensure they *do* have the proper device_type property, and later, Sashiko suggested that "pciex" (and "cardbus") also be accepted as valid bridge device_type property values.
The first patch prevents a possible null pointer dereference that Sashiko pointed out some time back. This is the only patch that has changed in this version. (Last time I neglected to enable the Kconfig option that ensured this go compiled, and it contained two obvious compile errors.)
The second and third patches are simple cleanups. The last adds the new PCI devicetree node checks and warnings.
At this point we are waiting for (and requesting) an ack from Rob Herring on the last patch.
-Alex
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 | 47 ++++++++++++++++++++++----------------- drivers/pci/pci.h | 3 +++ 4 files changed, 63 insertions(+), 20 deletions(-)
base-commit: 93f51579e7df248780214094418f205253383cc5
Signature · 1 line
2.53.0
PATCH v6 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().
--- drivers/pci/of_property.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)
drivers/pci/of_property.c +21 -12
@@ -324,27 +324,36 @@ 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/* Max size of each compatible string */ 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 ] = { }; +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); +if (ret >=PROP_SIZE ) +return -EINVAL ; + 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); +if (ret >=PROP_SIZE ) +return -EINVAL ; + 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_SIZEint of_pci_add_properties (struct pci_dev *pdev,struct of_changeset *ocs,struct device_node *np)
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v6 1/4] PCI: of: avoid allocations in of_pci_prop_compatible()
Sashiko has reviewed this patch and found no issues. It looks great!
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 inof_changeset_add_prop_string_array().Reported-by Sashiko <[email protected]>Signed-off-by Alex Elder <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v6 2/4 PCI: of: drop the reg_num argument to of_pci_set_address()
In reply to Alex ElderThe 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
@@ -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 *)®,sizeof (reg) /sizeof (u32));
@@ -467,7 +467,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
[email protected]
AI reviewRe: [PATCH v6 2/4] PCI: of: drop the reg_num argument to of_pci_set_address()
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
The
reg_numargument passed toof_pci_set_address()is always zero, so get rid of it.Reviewed-by Herve Codina <[email protected]>Signed-off-by Alex Elder <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v6 3/4 PCI: of: don't zero flags in of_pci_get_addr_flags()
In reply to Alex ElderThe 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
@@ -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
[email protected]
AI reviewRe: [PATCH v6 3/4] PCI: of: don't zero flags in of_pci_get_addr_flags()
Sashiko has reviewed this patch and found no issues. It looks great!
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.Reviewed-by Herve Codina <[email protected]>Signed-off-by Alex Elder <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v6 4/4 PCI: of: introduce of_pci_verify_node()
In reply to Alex ElderThe 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 them.
--- 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
@@ -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
@@ -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
@@ -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
[email protected]
AI reviewRe: [PATCH v6 4/4] PCI: of: introduce of_pci_verify_node()
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] pci: of: unintended error-level logging in of_pci_verify_node() --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: of: unintended error-level logging in of_pci_verify_node() --
commit 0e8b20efbbe63f895449027a5712b691b9eeaa56
Author: Alex Elder <[email protected]>
PCI: of: introduce of_pci_verify_node()
This commit introduces of_pci_verify_node() to verify the device_type property for PCI devices. It adds a runtime check during device enumeration to warn if a bridge lacks a "pci" or "pciex" device type or if an endpoint improperly includes one.
Quoted 4 lines
drivers/pci/of.c +0
-0
[ ... ]
Quoted 10 lines
drivers/pci/of.c +10
-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 1118
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" ) || +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: Medium]
The commit message states that these warnings are "meant to help developers; users should never see them."
However, dev_err() logs unconditionally at the error level in of_pci_verify_node(), making it prominently visible to end users and potentially failing automated test suites on boards with legacy devicetree nodes.
Should dev_dbg() or dev_info() be used here instead to match the stated developer-only intent?
Quoted 1 line
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v6 4/4] PCI: of: introduce of_pci_verify_node()
Quoted 37 lines · On 9/24/26 10:09 AM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: of: unintended error-level logging inof_pci_verify_node()--commit
0e8b20efbbe63f895449027a5712b691b9eeaa56
Author: Alex Elder <[email protected]>PCI: of: introduce
of_pci_verify_node()This commit introduces
of_pci_verify_node()to verify thedevice_typeproperty for PCI devices. It adds a runtime check during device enumeration to warn if a bridge lacks a "pci" or "pciex" device type or if an endpoint improperly includes one.Quoted 4 lines
drivers/pci/of.c +0
-0[ ... ]
Quoted 16 lines
drivers/pci/of.c +16
-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 ; + + 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)
Quoted 8 lines
drivers/pci/of.c int of_pci_get_equalization_presets(struct device *dev, · line 1118
Quoted 4 lines
drivers/pci/of.c +4
-0+return ; + +dev_err (&pdev->dev,"PCI %s have \"pci\" device_type property\n" , + device_is_bridge ?"bridge should" :"endpoint should not" );[Severity: Medium]
The commit message states that these warnings are "meant to help developers; users should never see them."
I suppose I should have said "error messages" rather than "warnings" here. I think dev_err() is appropriate, and unless a human objects to this, I do not plan to update the series to address this.
-Alex
Quoted 10 lines
However,
dev_err()logs unconditionally at the error level inof_pci_verify_node(), making it prominently visible to end users and potentially failing automated test suites on boards with legacy devicetree nodes.Should
dev_dbg()ordev_info()be used here instead to match the stated developer-only intent?Quoted 1 line
+}
Quoted 21 lines · Alex Elder
Add a check when adding a PCI device to ensure the
device_typeproperty is (or is not) correctly defined when the device has a non-null devicetree node pointer.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. During review, Herve Codina also suggested that bridge nodes be checked to ensure they *do* have the properdevice_typeproperty, and later, Sashiko suggested that "pciex" (and "cardbus") also be accepted as valid bridgedevice_typeproperty values.The first patch prevents a possible null pointer dereference that Sashiko pointed out some time back. This is the only patch that has changed in this version. (Last time I neglected to enable the
Kconfigoption that ensured this go compiled, and it contained two obvious compile errors.)
Quoted 5 lines
The second and third patches are simple cleanups. The last adds the new PCI devicetree node checks and warnings.
At this point we are waiting for (and requesting) an ack from Rob Herring on the last patch.
Rob, do you have any input on this series? I created it initially based on a suggestion from you. It simply warns in the PCI code if someone provides a device_type property somewhere other than a bridge, as well as when a bridge is defined *without* a device_type property.
Thanks.
-Alex
Quoted 57 lines
-Alex
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 changesVersion 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 patchVersion 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 bridgedevice_typeproperty value
- Add Herve's Reviewed-by tag on the last patchVersion 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 nodevice_typeproperty
- Added a patch to remove a duplicate warning in the devicetree codeVersion 2 is available here:
https://lore.kernel.org/lkml/[email protected]/Between version 1 and version 2:
- Check the PCI devicetree node even whenPCI_DYNAMIC_OF_NODESis not enabledVersion 1 is available here:
https://lore.kernel.org/lkml/[email protected]/Alex Elder (4):
PCI: of: avoid allocations inof_pci_prop_compatible()
PCI: of: drop thereg_numargument toof_pci_set_address()
PCI: of: don't zero flags inof_pci_get_addr_flags()
PCI: of: introduceof_pci_verify_node()drivers/pci/bus.c | 1 + drivers/pci/of.c | 32 ++++++++++++++++++++++++++ drivers/pci/of_property.c | 47 ++++++++++++++++++++++----------------- drivers/pci/pci.h | 3 +++ 4 files changed, 63 insertions(+), 20 deletions(-)base-commit:
93f51579e7df248780214094418f205253383cc5