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

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Using `dev_err()` to log a failure to acquire resets causes log spam when the error is `-EPROBE_DEFER`.

Pre-existing issues:
- [Critical] Critical use-after-free and state corruption due to `pm_genpd_remove()` failure during probe error handling.
- [High] Missing cleanup of previously acquired clocks if `of_clk_get()` fails during initialization.
- [High] Use-after-free on the PM domain's device tree node reference (`pd->node`).
--

--- Patch [3]: [RFC PATCH v3 3/6] pmdomain/rockchip: cycle optional power-domain resets on power-on ---
commit 7b67005dcd951b64c273c1af0e37dfcc0b7586dd
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 access after a cold power-on; the vendor kernel cycles the
domain's resets at this point and that 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 declare any behave exactly as before.

> 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 does this function leak clock handles if
of_clk_get() fails?

Looking at the loop just before this context:

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)) {
            ...
            return error;
        }
    }
}

If of_clk_get() returns -EPROBE_DEFER for an index > 0, it returns
immediately without calling clk_put() for the clocks successfully acquired in
earlier iterations. Should this jump to a cleanup path instead?

[Severity: High]
This is another pre-existing issue in this function, but is it safe to store
pd->node without incrementing its reference count?

In rockchip_pm_domain_probe(), domains are initialized within a loop that
automatically drops the node reference. Later, rockchip_pd_power() uses this
potentially freed node:

rockchip_pm_add_one_domain()
    pd->node = node;

rockchip_pd_power()
    devm_of_regulator_get(..., pd->node, ...);

Could this lead to a use-after-free if the node is dynamically removed?

>  
> +	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);

[Severity: Low]
Could this dev_err() cause log spam during normal boot?

If of_reset_control_array_get_optional_exclusive() returns -EPROBE_DEFER
because the reset controller hasn't probed yet, this will print an error.
Would dev_err_probe() be better here to suppress the spam and cleanly track
the deferral?

> +		pd->resets = NULL;
> +		goto err_unprepare_clocks;
> +	}

[ ... ]

> @@ -945,6 +963,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);
> +	reset_control_put(pd->resets);
>  
>  	/* protect the zeroing of pm->num_clks */
>  	mutex_lock(&pd->pmu->mutex);

[Severity: Critical]
This isn't a bug introduced by this patch, but could ignoring the return value
of pm_genpd_remove() lead to a use-after-free and state corruption?

During probe error paths, rockchip_pm_domain_cleanup() tries to remove
domains. If child links aren't removed first, pm_genpd_remove() fails with
-EBUSY. Since this failure is ignored:

rockchip_pm_remove_one_domain()
    ret = pm_genpd_remove(&pd->genpd);
    // ret is -EBUSY, but we continue tearing down

    clk_bulk_unprepare(...);
    reset_control_put(pd->resets);

The domain remains active in the global gpd_list, but its internal resources
are freed, and devm eventually frees the rockchip_pm_domain struct. When the
subsystem later iterates over domains, won't it dereference the freed memory
or the newly dangling pd->resets pointer?

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