Re: [PATCH v9 3/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 4 potential issue(s) to consider:

New issues:
- [High] NULL Pointer Dereference on `core->pmdomains`
- [Medium] Teardown Rollback Anti-pattern
- [Medium] Hardware Initialization Flaw for `vcodec_clks`

Pre-existing issues:
- [High] Ignored Error Code in `venc_power_v4()`
--

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

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

This commit adds power domain attachment and enable logic for Venus
vdec and venc cores. It introduces an array of vcodec clocks that
are retrieved from the device tree and enabled during decode and
encode sessions.

> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 14a4e8311a643..be1cbd5cfe842 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c

[ ... ]

> @@ -309,7 +343,77 @@ 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);

[Severity: Medium]
Is it intentional to skip getting vcodec_clks if vcodec_pmdomains is
not defined?

Clocks and power domains are physically independent hardware resources.
Tying the initialization of vcodec_clks strictly to the presence of
vcodec_pmdomains means that if a platform defines vcodec_clks but lacks
vcodec_pmdomains, the clocks will be silently ignored, which could lead
to a failure to power up the hardware properly.

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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];

[Severity: High]
If CONFIG_PM or CONFIG_PM_GENERIC_DOMAINS is disabled (which is possible
under COMPILE_TEST), the devm_pm_domain_attach_list() stub returns 0 but
does not allocate the list pointer, leaving core->pmdomains as NULL.

Since res->vcodec_pmdomains is populated from static device matching data,
could this loop execute and trigger a NULL pointer dereference on
core->pmdomains?

> +		ret = pm_runtime_resume_and_get(pd_dev);
> +		if (ret)
> +			goto err;
> +	}
> +
>  	return 0;

[ ... ]

> +static int vcodec_domains_set_hw(struct venus_core *core, bool is_hw)
> +{
> +	const struct venus_resources *res = core->res;
> +	struct device *pd_dev;
> +	int i = 0, ret;
> +
> +	for (; i < res->vcodec_pmdomains_num; i++) {
> +		pd_dev = core->pmdomains->pd_devs[i];
> +		ret = dev_pm_genpd_set_hwmode(pd_dev, is_hw);
> +		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, !is_hw);

[Severity: Medium]
If setting hardware mode fails while disabling it (is_hw == false), the
error handler loops backward and re-enables hardware mode (!is_hw) for
previously disabled power domains.

In core_power_v1()'s teardown path, the returned error is ignored, and
the driver proceeds to manually disable power domains via
vcodec_domains_disable().

Does this rollback leave the domains in hardware control mode, potentially
preventing the genpd framework from properly powering them off during
pm_runtime_put_sync() and resulting in an inconsistent power state?

> +	}
> +	return ret;
>  }

[ ... ]

> @@ -875,7 +1011,7 @@ static int venc_power_v4(struct device *dev, int on)
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but in venc_power_v4(), if
on == POWER_ON, the driver calls vcodec_clks_enable() and stores its
return value in ret:

	if (on == POWER_ON)
		ret = vcodec_clks_enable(core, core->vcodec1_clks);
	else
		vcodec_clks_disable(core, core->vcodec1_clks);

	ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);

Could the unconditional call to vcodec_control_v4() overwrite the error
code from vcodec_clks_enable()? If the clocks fail to enable, the driver
proceeds to access hardware registers without propagating the error, which
could cause a bus hang and system panic on ARM platforms.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-msm8939-venus-rfc-v9-0-bb1069f3fe02@gmail.com?part=3