Re: [PATCH v1] of: Put coreboot node after compatibility check

[sashiko-bot@kernel.org]

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 <dbgh9129@gmail.com>

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.

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3f..8d04eaed98053 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -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?

> +	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802023821.494178-1-dbgh9129@gmail.com?part=1