Re: [PATCH v3 1/3] remoteproc: qcom_q6v5_pas: propagate dev_pm_genpd_set_performance_state() errors

[sashiko-bot@kernel.org]

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

New issues:
- [High] The added error check for dev_pm_genpd_set_performance_state() is dead code and completely fails to fix the silent failure masking.

Pre-existing issues:
- [High] Pre-existing identical sequence flaw in sister remoteproc drivers silently masks performance state errors.
--

commit 79e5a0b96d1b1d35117390df652884f8c2c16c71
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

remoteproc: qcom_q6v5_pas: propagate dev_pm_genpd_set_performance_state() errors

This commit attempts to capture and handle errors from
dev_pm_genpd_set_performance_state() in qcom_pas_pds_enable() to avoid
silently masking performance state failures.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index ca8e61254c449..42bcbe72f95bd 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -167,7 +167,13 @@ static int qcom_pas_pds_enable(struct qcom_pas *pas, struct device **pds,
>  	int i;
>  
>  	for (i = 0; i < pd_count; i++) {
> -		dev_pm_genpd_set_performance_state(pds[i], INT_MAX);
> +		ret = dev_pm_genpd_set_performance_state(pds[i], INT_MAX);
> +		if (ret) {
> +			dev_err(pas->dev,
> +				"failed to set proxy PD %d state %u: %d\n",
> +				i, INT_MAX, ret);
> +			goto unroll_pd_votes;
> +		}
>  		ret = pm_runtime_get_sync(pds[i]);

[Severity: High]
Is this newly added error check able to catch performance state errors?

Because dev_pm_genpd_set_performance_state() is called before
pm_runtime_get_sync(), the proxy PD device is still in the RPM_SUSPENDED
state when this executes.

When called on an RPM_SUSPENDED device, genpd_dev_pm_set_performance_state()
merely caches the request in rpm_pstate and unconditionally returns 0.

As a result, this check will never evaluate to true. The actual application
of the state is deferred to pm_runtime_get_sync(), which calls
genpd_runtime_resume() and genpd_restore_performance_state().

Because genpd_restore_performance_state() returns void, it drops any errors
from the underlying provider.

Does this mean the failure remains silently masked despite this patch?

[Severity: High]
This isn't a bug introduced by this patch, but this identical sequence flaw
exists in several sister remoteproc drivers.

In qcom_q6v5_mss.c:q6v5_pds_enable(),
qcom_q6v5_adsp.c:qcom_rproc_pds_enable(), and qcom_wcnss.c:wcnss_start(),
the drivers also call dev_pm_genpd_set_performance_state() while the proxy
PDs are in the RPM_SUSPENDED state (prior to calling pm_runtime_get_sync()).

Since they are suspended, the genpd framework defers execution to the resume
path where errors are discarded, silently masking performance state failures.

Should the initialization sequence be adjusted across all these drivers so
the devices are resumed before setting the performance state?

>  		if (ret < 0) {
>  			pm_runtime_put_noidle(pds[i]);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909105049.1317985-1-mukesh.ojha@oss.qualcomm.com?part=1