Re: [RFC PATCH v4 3/6] pmdomain/rockchip: cycle optional power-domain resets on power-on

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

Pre-existing issues:
- [High] A pre-existing resource leak occurs in `rockchip_pm_add_one_domain()` when `of_clk_get()` fails, leading to permanent leaks of previously acquired clock references and potential unbounded resource exhaustion upon probe deferral.
--

--- Patch [3]: [RFC PATCH v4 3/6] pmdomain/rockchip: cycle optional power-domain resets on power-on ---
commit c4991b83e89b1e1b5f031a43c8789950a7837730
Author: Jiaxing Hu <[email protected]>

pmdomain/rockchip: cycle optional power-domain resets on power-on

Some Rockchip domains come out of power-on with their bus interface in
an undefined state. This patch cycles the domain's resets to clear it.
It takes the domain node's resets if it has any, and pulses them between
releasing idle and restoring QoS.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index e1857f8786864..82051f60090f3 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

> @@ -857,6 +866,14 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>  	if (error)
>  		goto err_put_clocks;

[Severity: High]
This is a pre-existing issue, but looking at the clock acquisition loop just
above this in rockchip_pm_add_one_domain():

	for (i = 0; i < pd->num_clks; i++) {
		pd->clks[i].clk = of_clk_get(node, i);
		if (IS_ERR(pd->clks[i].clk)) {
			error = PTR_ERR(pd->clks[i].clk);
			dev_err(pmu->dev,
				"%pOFn: failed to get clk at index %d: %d\n",
				node, i, error);
			return error;
		}
	}

If a clock at index > 0 fails to be acquired (for example, returning
-EPROBE_DEFER), does the direct return bypass the err_put_clocks cleanup
path? It appears this might permanently leak the previously acquired clock
references, potentially leading to unbounded resource exhaustion upon probe
deferral retries. Should this code clean up the successfully acquired clocks
instead of returning directly?

> +	pd->resets = of_reset_control_array_get_optional_exclusive(node);
> +	if (IS_ERR(pd->resets)) {
> +		error = PTR_ERR(pd->resets);
> +		dev_err(pmu->dev, "%pOFn: failed to get resets: %d\n", node, error);
> +		pd->resets = NULL;
> +		goto err_unprepare_clocks;
> +	}
> +
>  	pd->num_qos = of_count_phandle_with_args(node, "pm_qos",
>  						 NULL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3