of: Put coreboot node after compatibility check
PATCH 1/1 of: Put coreboot node after compatibility check
of_find_compatible_node() returns a referenced device node, but EXCLUDED_DEFAULT_CELLS_PLATFORMS used the result only as a boolean and discarded the reference. This leaked a reference each time the address-cells or size-cells warning condition was evaluated on a coreboot system.
Use a helper that drops the node reference after checking whether the coreboot node exists.
--- drivers/of/base.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
drivers/of/base.c +13 -1
@@ -95,9 +95,21 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)return !strcmp (match, type); } +static bool of_coreboot_present (void ) +{ +struct device_node *np; +bool found; + + np =of_find_compatible_node (NULL ,NULL ,"coreboot" ); + found = np !=NULL ; +of_node_put (np); + +return found; +} +#define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \ IS_ENABLED(CONFIG_SPARC) || \ - of_find_compatible_node(NULL, NULL, "coreboot") \ + of_coreboot_present() \ )int of_bus_n_addr_cells (struct device_node *np)
Signature · 1 line
2.43.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The EXCLUDED_DEFAULT_CELLS_PLATFORMS macro evaluates its condition on every execution of WARN_ONCE, causing repeated full device tree traversals under a raw spinlock.
- [High] of_msi_xlate() incorrectly ignores the msi-parent DT binding when the caller passes NULL for msi_np, leading to erroneous MSI ID mapping. --
--- Patch [1]: [PATCH v1] of: Put coreboot node after compatibility check --- commit d698ca1f75edc245216563869cb5c61be66a91d2
Author: Yuho Choi <[email protected]>
of: Put coreboot node after compatibility check
of_find_compatible_node() returns a referenced device node, but EXCLUDED_DEFAULT_CELLS_PLATFORMS used the result only as a boolean and discarded the reference. This leaked a reference each time the address-cells or size-cells warning condition was evaluated on a coreboot system. Use a helper that drops the node reference after checking whether the coreboot node exists.
Quoted 5 lines
drivers/of/base.c +0
-0
drivers/of/base.c static bool __of_node_is_type(const struct device_node *np, const char *type) · line 103
Quoted 9 lines
drivers/of/base.c +6
-0@@ -95,9 +95,21 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)return !strcmp (match, type); } +static bool of_coreboot_present (void ) +{ +struct device_node *np; +bool found; + + np =of_find_compatible_node (NULL ,NULL ,"coreboot" );
[Severity: High]
This isn't a bug introduced by this patch, but WARN_ONCE unconditionally evaluates its condition on every call, even if the warning has already fired.
When of_bus_n_addr_cells() or of_bus_n_size_cells() are called on a node missing these properties, EXCLUDED_DEFAULT_CELLS_PLATFORMS is evaluated, which now calls of_coreboot_present().
Since of_find_compatible_node() acquires the global devtree_lock and linearly scans the entire device tree for a 'coreboot' compatible node, can this cause significant performance regressions and lock contention on systems where missing address/size cells are common?
drivers/of/base.c static bool __of_node_is_type(const struct device_node *np, const char *type) · line 115
Quoted 13 lines
drivers/of/base.c +7
-1+ found = np !=NULL ; +of_node_put (np); + +return found; +} +#define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \ IS_ENABLED(CONFIG_SPARC) || \ - of_find_compatible_node(NULL, NULL, "coreboot") \ + of_coreboot_present() \ )int of_bus_n_addr_cells (struct device_node *np)
I also noticed a pre-existing issue in drivers/of/irq.c:
drivers/of/irq.c:of_msi_xlate() {
for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
...
/* -ENODEV: msi-map absent -> check for msi-parent */
if (msi_np && !of_check_msi_parent(parent_dev->of_node, msi_np))
break;
}
return id_out;
}[Severity: High]
This isn't a bug introduced by this patch, but does of_msi_xlate() incorrectly ignore the msi-parent DT binding when the caller passes NULL for msi_np?
If a caller requests MSI ID translation but passes NULL for msi_np, the loop termination condition short-circuits since it requires msi_np to be non-NULL. This causes traversal to incorrectly continue up the device tree.
If an ancestor node defines an msi-map, it will erroneously apply that mapping to the device's ID, leading to incorrect interrupt mapping and potential device isolation failure. Should this check handle a NULL msi_np?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 20 lines · Yuho Choi
of_find_compatible_node()returns a referenced device node, butEXCLUDED_DEFAULT_CELLS_PLATFORMSused the result only as a boolean and discarded the reference. This leaked a reference each time the address-cells or size-cells warning condition was evaluated on a coreboot system.Use a helper that drops the node reference after checking whether the coreboot node exists.
Fixes 8600058ba28a7 ("of: Add coreboot firmware to excluded default cells list")Signed-off-by Yuho Choi <[email protected]>--- drivers/of/base.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)drivers/of/base.c +0
-0
drivers/of/base.c static bool __of_node_is_type(const struct device_node *np, const char *type) · line 103
Quoted 9 lines
drivers/of/base.c +6
-0@@ -95,9 +95,21 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)return !strcmp (match, type); } +static bool of_coreboot_present (void ) +{ +struct device_node *np; +bool found; + + np =of_find_compatible_node (NULL ,NULL ,"coreboot" );
Code is simple, but could be even simpler with __free. Basically two instructions instead of four and no need for this assignment comparison.
drivers/of/base.c static bool __of_node_is_type(const struct device_node *np, const char *type) · line 105
Quoted 2 lines
drivers/of/base.c +2
-0+ found = np !=NULL ; +of_node_put (np);
Best regards,
Krzysztof