Re: [PATCH v2 5/7] clk: Use the generic OF phandle parsing in only one place

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Switching to `of_parse_clkspec` breaks out-of-bounds indexed clock lookups on nodes with `clock-ranges`, causing infinite loops and topology corruption.
- [High] API mismatch between `of_clk_get_parent_name` and `of_clk_get_parent_count` regarding `clock-ranges` support.
--

--- Patch [5]: [PATCH v2 5/7] clk: Use the generic OF phandle parsing in only one place ---
commit bc84db19056243e06a72beb625e47749500a0824
Author: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

clk: Use the generic OF phandle parsing in only one place

This patch refactors of_clk_get_parent_name() to use of_parse_clkspec()
instead of of_parse_phandle_with_args(). The stated goal is to consolidate
Device Tree parsing and ensure consistent handling, particularly to support
the parsing of the clock-ranges property.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7d63d81ebc096..dad4bac10ad45 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5364,8 +5364,7 @@ const char *of_clk_get_parent_name(const struct device_node *np, int index)
>  	int count;
>  	struct clk *clk;
>  
> -	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> -					&clkspec);
> +	rc = of_parse_clkspec(np, index, NULL, &clkspec);

[Severity: High]
Does this change break out-of-bounds indexed clock lookups on nodes with
clock-ranges?

When falling back to the parent node, of_parse_clkspec() unconditionally
resets the index:

drivers/clk/clk.c:of_parse_clkspec() {
	...
	np = np->parent;
	if (np && !of_property_present(np, "clock-ranges"))
		break;
	index = 0;
	...
}

Iterators like of_clk_parent_fill() rely on of_clk_get_parent_name()
returning NULL to terminate when they exceed the available clock indices:

drivers/clk/clk.c:of_clk_parent_fill() {
	...
	while (i < size && (parents[i] = of_clk_get_parent_name(np, i)) != NULL)
		i++;
	...
}

Because the index is reset to 0 upon fallback, when the iterator exceeds
the child's valid clock indices, won't this successfully return the
parent's 0-th clock instead of failing?

If so, this could fill the array with duplicate copies of the parent's
0-th clock and corrupt the clock tree topology.

[Severity: High]
Does this create an API mismatch with of_clk_get_parent_count() regarding
clock-ranges support?

of_clk_get_parent_count() strictly counts the elements in the immediate
child node's clocks property and returns 0 if absent:

drivers/clk/clk.c:of_clk_get_parent_count() {
	...
	count = of_count_phandle_with_args(np, "clocks", "#clock-cells");
	...
}

Since of_clk_get_parent_name() now falls back to the parent node via
clock-ranges, what happens if a node relies entirely on clock-ranges to
inherit clocks?

If drivers dynamically allocate parent arrays based on the count, won't
they size them to 0 and skip clock initialization entirely, rendering
clock-ranges inheritance ineffective?

>  	if (rc)
>  		return NULL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-schneider-v7-2-rc1-eip201-upstream-v2-0-b4680787377e@bootlin.com?part=5