Re: [PATCH v6 02/16] device property: Add fwnode_graph_get_next_port_endpoint()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Hardcoded OF naming conventions break ACPI graph node endpoint identification.
- [Medium] `fwnode_graph_get_next_port_endpoint()` leaks the `prev` reference when called with a `NULL` `port`, violating its own docstring contract.

Pre-existing issues:
- [High] `fwnode_get_next_child_node()` causes a double-free, use-after-free, and an infinite loop when iterating over a node with both primary and secondary fwnodes.
--

--- Patch [2]: [PATCH v6 02/16] device property: Add fwnode_graph_get_next_port_endpoint() ---
commit 0b043b08c9a4c2f484d73d298735da40162c82f7
Author: Chen-Yu Tsai <wenst@chromium.org>

device property: Add fwnode_graph_get_next_port_endpoint()

This commit introduces a helper to get the next endpoint under a given
"port" firmware node. It checks if the child node name matches "endpoint"
and continues until it finds a match or exhausts all children.

> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 0bd45f8b01c14..62c6221d65d05 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c

[ ... ]

> @@ -1099,6 +1099,31 @@ int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name)
>  }
>  EXPORT_SYMBOL(fwnode_irq_get_byname);
>  
> +/**
> + * fwnode_graph_get_next_port_endpoint - Get next endpoint firmware node in port
> + * @port: Pointer to the target port firmware node
> + * @prev: Previous endpoint node or %NULL to get the first
> + *
> + * The caller is responsible for calling fwnode_handle_put() on the returned
> + * fwnode pointer. Note that this function also puts a reference to @prev
> + * unconditionally.
> + *
> + * Return: an endpoint firmware node pointer or %NULL if no more endpoints
> + * are available.
> + */
> +struct fwnode_handle *fwnode_graph_get_next_port_endpoint(const struct fwnode_handle *port,
> +							  struct fwnode_handle *prev)
> +{
> +	do {
> +		prev = fwnode_get_next_child_node(port, prev);

[Severity: Medium]
Will this leak the prev reference if port is NULL?

The docstring states this function puts a reference to prev unconditionally.
However, if port is NULL, fwnode_get_next_child_node() returns NULL without
putting prev. The loop then terminates, returning NULL and silently
overwriting the original prev reference, resulting in a leak.

[Severity: High]
This isn't a bug introduced by this patch, but can the call to
fwnode_get_next_child_node() cause a double-free and a potential infinite
loop if the port node has both primary and secondary fwnodes?

Looking at fwnode_get_next_child_node(), if child is the last child of the
primary fwnode, fwnode_call_ptr_op() drops the reference and returns NULL.
The function then falls through and passes the already-freed child pointer
to the secondary backend:

fwnode_get_next_child_node() {
    ...
	next = fwnode_call_ptr_op(fwnode, get_next_child_node, child);
	if (next)
		return next;

	/* When no more children in primary, continue with secondary */
	return fwnode_call_ptr_op(fwnode->secondary, get_next_child_node, child);
}

If the secondary backend is a software node, it unconditionally puts the
child again, causing a double-free. Furthermore, if it returns a valid
secondary child, the next iteration passes it back to the primary backend
which evaluates it to NULL, looping back to the first primary child.

> +		if (fwnode_name_eq(prev, "endpoint"))

[Severity: High]
Is it safe to hardcode the "endpoint" string match here for all firmware
types, such as ACPI?

While this works for Device Tree, ACPI evaluates endpoints differently using
is_acpi_graph_node(), which allows endpoints to be identified by an
"endpoint" property or a "reg" property with an "endpoint@" prefix,
regardless of the literal node name.

By relying strictly on fwnode_name_eq(), this generic helper bypasses
the backend abstractions and might fail for ACPI endpoints that use
property-based identification or lack a 'reg' property.

> +			break;
> +	} while (prev);
> +
> +	return prev;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_graph_get_next_port_endpoint);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721065413.2306137-1-wenst@chromium.org?part=2