Re: [PATCH DNM RFC 16/16] pmdomain: qcom: rpmhpd: Stop forwarding MMCX pstate to CX

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pmdomain: qcom: missing error cleanup in rpmhpd_probe() corrupts gpd_list
- [High] pmdomain: qcom: missing clk-dvfs data causes system hangs
--

commit e0f5d54b21f4fa98424f55031a683613420b6a75
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

pmdomain: qcom: rpmhpd: Stop forwarding MMCX pstate to CX

This commit modifies the MMCX and MMCX_AO power domains to suppress
performance-state propagation to their CX and CX_AO parent domains. The
parent relationship is kept solely for power ordering.

> diff --git a/drivers/pmdomain/qcom/rpmhpd.c b/drivers/pmdomain/qcom/rpmhpd.c
> index fa8fed5bd3e58..4d9463b4d5df0 100644
> --- a/drivers/pmdomain/qcom/rpmhpd.c
> +++ b/drivers/pmdomain/qcom/rpmhpd.c
> @@ -154,6 +156,7 @@ static struct rpmhpd mmcx_w_cx_parent = {
>  	.pd = { .name = "mmcx", },
>  	.peer = &mmcx_ao_w_cx_parent,
>  	.parent = &cx.pd,
> +	.parent_flags = GENPD_SUBDOMAIN_NO_PERF_STATE_PROPAGATION,

[Severity: High]
The commit message explicitly warns that this patch must not be merged without
filling out clock DVFS data for all platforms first. It appears platforms
such as SM8550, SM8450, and SM8250 do not yet receive the necessary DVFS data
updates.

Will this cause the CX power domain to lack required performance state votes,
leading to system hangs and hardware lockups when multimedia blocks operate on
these older platforms?

[ ... ]

> @@ -1223,9 +1227,13 @@ static int rpmhpd_probe(struct platform_device *pdev)
>  	for (i = 0; i < num_pds; i++) {
>  		if (!rpmhpds[i])
>  			continue;
> -		if (rpmhpds[i]->parent)
> -			pm_genpd_add_subdomain(rpmhpds[i]->parent,
> -					       &rpmhpds[i]->pd);
> +		if (rpmhpds[i]->parent) {
> +			ret = pm_genpd_add_subdomain_with_flags(rpmhpds[i]->parent,
> +							        &rpmhpds[i]->pd,
> +							        rpmhpds[i]->parent_flags);
> +			if (ret)
> +				return ret;

[Severity: High]
Does returning directly here leave statically allocated PM domains globally
registered in the genpd core?

By the time this loop executes, all PM domains have already been initialized
and added to the global gpd_list by pm_genpd_init() in the preceding loop.
Because the PM domain structures are statically allocated file globals,
returning an error without cleanup leaves their gpd_list_node members in the
global gpd_list.

If probe is later retried, for example via deferred probe, pm_genpd_init()
will be invoked again on these same static structures, potentially leading to
list corruption.

Would an error path that calls pm_genpd_remove() for initialized domains and
pm_genpd_remove_subdomain() for previously added subdomains be needed here?

> +		}
>  	}
>  
>  	return of_genpd_add_provider_onecell(pdev->dev.of_node, data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-topic-qc_clk_dvfs-v1-0-3041b788e9f7@oss.qualcomm.com?part=16