Re: [PATCH v11 10/14] pmdomain/rockchip: cycle optional power-domain resets on power-on

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Resource leak of clock references on of_clk_get failure
- [High] Use-after-free when pm_genpd_remove fails during probe error cleanup
--

commit c4349e9db6c9eecdba71ab60966e3312c6588638
Author: Jiaxing Hu <gahing@gahingwoo.com>

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. On the RK3576 NPU this shows up as a hang on the
first register access after the domain is switched on, and pulsing the
domain's resets at this point clears it.

Take the domain node's resets if it has any, and pulse them between
releasing idle and restoring QoS. The resets are optional, so domains
that do not list any are unaffected.

> diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
> index 39988efd86aaa..8f2fd8a83e8e9 100644
> --- a/drivers/pmdomain/rockchip/pm-domains.c
> +++ b/drivers/pmdomain/rockchip/pm-domains.c

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak the
clocks on of_clk_get() failure?

Looking at rockchip_pm_add_one_domain(), just above this code:

	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 PM domain has multiple clocks defined in its device tree node, and
of_clk_get() succeeds for the first clock(s) but fails for a subsequent
clock (e.g., returning -EPROBE_DEFER), the function returns the error
directly without calling clk_put() on the successfully acquired clocks.

Could this direct return bypass the err_put_clocks cleanup path and leak
the clocks acquired in previous loop iterations?

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

[ ... ]

> @@ -949,6 +967,7 @@ static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
>  
>  	clk_bulk_unprepare(pd->num_clks, pd->clks);
>  	clk_bulk_put(pd->num_clks, pd->clks);

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free when
pm_genpd_remove() fails during probe error cleanup?

In rockchip_pm_remove_one_domain(), right before this code:

	ret = pm_genpd_remove(&pd->genpd);
	if (ret < 0)
		dev_err(pd->pmu->dev, "failed to remove domain '%s' : %d - state may be inconsistent\n",
			pd->genpd.name, ret);

If rockchip_pm_add_subdomain() or pm_genpd_provider_add_of() fails,
rockchip_pm_domain_cleanup() iterates over all domains in reg ID order.
If a child domain has a smaller ID than its parent, the child is removed
first. Because its parent link still exists, pm_genpd_remove() returns
-EBUSY and fails to remove the child from the global gpd_list.

Despite the failure, rockchip_pm_remove_one_domain() returns and the
rockchip_pm_domain structure is freed by devm when the probe fails.

Does this leave a pointer to freed memory in the global gpd_list, leading
to a use-after-free when the list is next accessed (e.g., by
genpd_power_off_unused())?

> +	reset_control_put(pd->resets);
>  
>  	/* protect the zeroing of pm->num_clks */
>  	mutex_lock(&pd->pmu->mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831081956.84871-1-gahing@gahingwoo.com?part=10