Re: [PATCH v5 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:

New issues:
- [Medium] The switch to `of_parse_clkspec` in `of_clk_get_parent_name` introduces broken positional index lookups during `clock-ranges` fallback and creates an API mismatch with `of_clk_get_parent_count`.

Pre-existing issues:
- [High] Returning a pointer to the device node's name (`clkspec.np->name`) after calling `of_node_put(clkspec.np)` causes a potential Use-After-Free.
--

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

This patch consolidates the device tree parsing logic by replacing
of_parse_phandle_with_args() with of_parse_clkspec() inside
of_clk_get_parent_name(). This change provides uniform handling and aims
to support the clock-ranges property during parent clock resolution.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7d63d81ebc096..45f5d7a4ccc17 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5219,6 +5219,8 @@ static int of_parse_clkspec(const struct device_node *np, int index,
>  		 * has a "clock-ranges" property, then we can try one of its
>  		 * clocks.
>  		 */
> +		if (of_property_present(np, "clocks"))
> +			break;
>  		np = np->parent;
>  		if (np && !of_property_present(np, "clock-ranges"))
>  			break;

[Severity: Medium]
When falling back to the parent node due to clock-ranges, does resetting
the index to 0 break positional index lookups?

Right below this snippet in of_parse_clkspec(), the index is explicitly
reset to 0 before the loop continues:

	np = np->parent;
	if (np && !of_property_present(np, "clock-ranges"))
		break;
	index = 0;

If a driver calls of_clk_get_parent_name() with an index > 0 for a node
that lacks a clocks property but whose parent has clock-ranges, this reset
causes the parent's 0th clock to be returned repeatedly instead of the
subsequently requested clocks.

> @@ -5364,8 +5366,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);
>  	if (rc)
>  		return NULL;

[Severity: Medium]
Does this change create an API mismatch with of_clk_get_parent_count()?

Currently, of_clk_get_parent_count() relies on of_count_phandle_with_args()
which does not check for the clock-ranges fallback. Now that
of_clk_get_parent_name() uses of_parse_clkspec() and supports clock-ranges,
will drivers relying on of_clk_get_parent_count() receive a count of 0 and
fail to initialize?

> [ ... ]
> 		clk = of_clk_get_from_provider(&clkspec);
> 		if (IS_ERR(clk)) {
> 			if (clkspec.args_count == 0)
> 				clk_name = clkspec.np->name;
> 			else
> 				clk_name = NULL;
> 		} else {
> 			clk_name = __clk_get_name(clk);
> 			clk_put(clk);
> 		}
> 	}
> 
> 
> 	of_node_put(clkspec.np);
> 	return clk_name;

[Severity: High]
This is a pre-existing issue, but can this result in a use-after-free of the
node's name?

If the clock is unregistered and has a clock-cells value of 0, clk_name is
assigned directly to clkspec.np->name. The node reference is then dropped
immediately via of_node_put(). 

If the node was dynamically allocated, such as through a device tree
overlay, dropping the last reference here frees the node and its string
properties, leaving clk_name pointing to freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-schneider-v7-2-rc1-eip201-upstream-v5-0-8785f9531cf0@bootlin.com?part=5