Re: [PATCH v7 6/8] media: qcom: venus: add power domain enable logic for Venus cores

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Clock initialization is incorrectly gated by the presence of power domains, leading to unclocked hardware access.
- [Medium] Runtime PM reference leak in vcodec_domains_enable() error path.
--

commit 1bdd7b26dbc6049ee656dfce05ea05b682bb5990
Author: Erikas Bitovtas <xerikasxx@gmail.com>

media: qcom: venus: add power domain enable logic for Venus cores

Attach power domains for vdec and venc cores and power them up if a vdec
or venc session is started. Vcodev clocks are added and enabled to the
core Venus device both for vcodec0 and vcodec1.

> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index f0269524ac70e..be705d4d64b53 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c

[ ... ]

> @@ -309,9 +343,63 @@ static int core_get_v1(struct venus_core *core)
>  	if (ret)
>  		return ret;
>  
> +	if (!res->vcodec_pmdomains)
> +		return 0;
> +
> +	ret = vcodec_clks_get(core, dev, core->vcodec_clks,
> +			      res->vcodec_clks);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }

Will skipping vcodec_clks_get() when power domains are not present leave
the vcodec clocks uninitialized?

If a platform defines vcodec_clks but does not require vcodec_pmdomains,
this early return skips the clock retrieval entirely.

When core_power_v1() later calls vcodec_clks_enable(), it will succeed
on the NULL pointers, but the physical hardware clocks will remain disabled.
Accessing Venus hardware registers without clocks enabled can trigger an
asynchronous external abort and a kernel panic.

> +static int vcodec_domains_enable(struct venus_core *core)
> +{
> +	const struct venus_resources *res = core->res;
> +	struct device *pd_dev;
> +	int i = 0, ret;
> +
> +	if (!res->vcodec_pmdomains)
> +		return 0;
> +
> +	for (; i < res->vcodec_pmdomains_num; i++) {
> +		pd_dev = core->pmdomains->pd_devs[i];
> +		ret = pm_runtime_resume_and_get(pd_dev);
> +		if (ret)
> +			goto err;
> +
> +		ret = dev_pm_genpd_set_hwmode(pd_dev, true);
> +		if (ret && ret != -EOPNOTSUPP)
> +			goto err;
> +	}
> +
> +	return 0;
> +err:
> +	while (i--) {
> +		pd_dev = core->pmdomains->pd_devs[i];
> +		dev_pm_genpd_set_hwmode(pd_dev, false);
> +		pm_runtime_put_sync(pd_dev);
> +	}
> +	return ret;
> +}

Does this error path leak a runtime PM reference if dev_pm_genpd_set_hwmode()
fails?

If pm_runtime_resume_and_get() succeeds but the subsequent call to
dev_pm_genpd_set_hwmode() fails, the code jumps to the err label.

Since while (i--) pre-decrements the index, it skips the current iteration
index i and rolls back from i-1 down to 0.

This misses the pm_runtime_put_sync() call for the current power domain
where dev_pm_genpd_set_hwmode() failed, leaving its reference incremented
and preventing it from ever suspending.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-msm8939-venus-rfc-v7-0-33c6c6fb9285@gmail.com?part=6