PCI: of: warn on bogus device_type property
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
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
@@ -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_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 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 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.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v7 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);
@@ -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 *)®,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
[email protected]
AI reviewRe: [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_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.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v7 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 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.Reviewed-by Herve Codina <[email protected]>Signed-off-by Alex Elder <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v7 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 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
@@ -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
@@ -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
[email protected]
AI reviewRe: [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@@ -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