Re: [PATCH 02/16] clk: qcom: gcc-sm8350: Tie the CX power domain to controller

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

Pre-existing issues:
- [High] Resource leak and Use-After-Free (UAF) in generic power domain registration error paths.
--

--- Patch [2]: [PATCH 02/16] clk: qcom: gcc-sm8350: Tie the CX power domain to controller ---
commit 2f90d172dae6e2aec9b20b94ed643f0031c58164
Author: Abel Vesa <[email protected]>

clk: qcom: gcc-sm8350: Tie the CX power domain to controller

Enable runtime PM for the controller so the common GCC probe path resumes
the attached domain while registering clocks, resets and GDSCs.

> diff --git a/drivers/clk/qcom/gcc-sm8350.c b/drivers/clk/qcom/gcc-sm8350.c
> index 2d94f3046b711..996ee8592fabe 100644
> --- a/drivers/clk/qcom/gcc-sm8350.c
> +++ b/drivers/clk/qcom/gcc-sm8350.c
> @@ -3786,6 +3786,7 @@ static const struct qcom_cc_desc gcc_sm8350_desc = {
>  	.resets = gcc_sm8350_resets,
>  	.num_resets = ARRAY_SIZE(gcc_sm8350_resets),
>  	.gdscs = gcc_sm8350_gdscs,
> +	.use_rpm = true,

[Severity: High]
This is a pre-existing issue, but since this patch relies on the GDSC
infrastructure, is there a risk of a use-after-free during error handling
in the core GDSC registration?

Looking at gdsc_register() in drivers/clk/qcom/gdsc.c, if a failure occurs
during gdsc_init(), the function returns directly:

drivers/clk/qcom/gdsc.c:gdsc_register() {
    ...
        ret = gdsc_init(scs[i]);
        if (ret)
            return ret;
    ...
}

Similarly, if of_genpd_add_provider_onecell() fails, it returns without
unregistering the initialized domains:

drivers/clk/qcom/gdsc.c:gdsc_register() {
    ...
    return of_genpd_add_provider_onecell(dev->of_node, data);

err_pm_subdomain_remove:
    gdsc_pm_subdomain_remove(desc, i);

    return ret;
}

Because pm_genpd_remove() is not called on these error paths, the domains
remain linked in the global gpd_list.

When the probe fails, devres will clean up managed resources like regmap.
Later, when the genpd framework invokes pd->power_off() (such as via
genpd_power_off_unused() at late_initcall), it calls gdsc_disable() which
could dereference these freed devres-managed pointers.

Should gdsc_register() be updated to unregister the power domains on these
error paths to prevent dangling pointers?

>  	.num_gdscs = ARRAY_SIZE(gcc_sm8350_gdscs),
>  };

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