Re: [PATCH v6 11/18] media: iris: Add framework support for AR50_LITE video core

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Firmware boot errors in `iris_vpu_boot_firmware()` are silently ignored due to a premature loop break without returning an error code.
- [High] Early returns on error in `iris_enable_power_domains()` and `iris_disable_power_domains()` leak runtime PM usage counts.
--

--- Patch [11]: [PATCH v6 11/18] media: iris: Add framework support for AR50_LITE video core ---
commit 8407376927e1cbde94a3ada7a51a697669409b40
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Add framework support for AR50_LITE video core

Add power sequence for ar5lt core. Add register handling for ar50lt by
hooking up vpu op with ar50lt specific implementation or reuse from
earlier generation wherever feasible.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c
> new file mode 100644
> index 0000000000000..e084a5b49f2e3
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c

[ ... ]

> +static int iris_vpu_ar50lt_power_off_controller(struct iris_core *core)
> +{
> +	iris_disable_unprepare_clock(core, IRIS_AHB_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_CTRL_CLK);
> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);

[Severity: High]
This is a pre-existing issue, but could this leak the PM reference count if
iris_disable_power_domains() fails internally?

Looking at the implementation of iris_disable_power_domains() in 
iris_resources.c:

int iris_disable_power_domains(struct iris_core *core, struct device *pd_dev)
{
        int ret;

        ret = iris_opp_set_rate(core->dev, 0);
        if (ret)
                return ret;

        pm_runtime_put_sync(pd_dev);

        return 0;
}

If iris_opp_set_rate() fails, the function returns early and skips the
required pm_runtime_put_sync(pd_dev). Will this permanently prevent the
device and its power domain from suspending?

> +
> +	return 0;
> +}

[ ... ]

> +static int iris_vpu_ar50lt_power_on_controller(struct iris_core *core)
> +{
> +	int ret;
> +
> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
iris_enable_power_domains() here leak the runtime PM usage count on
failure?

Looking at iris_enable_power_domains():

int iris_enable_power_domains(struct iris_core *core, struct device *pd_dev)
{
        ...
        ret = pm_runtime_get_sync(pd_dev);
        if (ret < 0)
                return ret;

        return ret;
}

When pm_runtime_get_sync() fails, it increments the usage counter but the
early return leaves it incremented without dropping the reference. Could
pm_runtime_resume_and_get() be used instead to safely handle the failure?

> +	if (ret)
> +		return ret;

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 41498f94480e8..d64e7745a63dc 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -97,7 +97,8 @@ int iris_vpu_boot_firmware(struct iris_core *core)
>  	}
>  
>  	writel(HOST2XTENSA_INTR_ENABLE, core->reg_base + CPU_CS_H2XSOFTINTEN);
> -	writel(0x0, core->reg_base + CPU_CS_X2RPMH);
> +	if (!core->iris_platform_data->no_rpmh)
> +		writel(0x0, core->reg_base + CPU_CS_X2RPMH);
>  
>  	return 0;
>  }

[Severity: High]
This is also a pre-existing issue, but does iris_vpu_boot_firmware()
erroneously return success when the firmware fails to boot?

Looking earlier in iris_vpu_boot_firmware():

    while (!ctrl_status && count < max_tries) {
        ctrl_status = readl(core->reg_base + CTRL_STATUS);
        if ((ctrl_status & CTRL_ERROR_STATUS__M) == 0x4) {
            dev_err(core->dev, "invalid setting for uc_region\n");
            break;
        }
        ...
    }

If this break occurs because of an invalid uc_region setting, the loop
exits without setting an error code. Since count is still less than
max_tries, the subsequent timeout check is bypassed, and the function
falls through to return 0 here.

Will this cause the driver to continue initialization with an unusable
VPU, potentially leading to cascading failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-iris-ar50lt-v6-0-374f0a46c23b@oss.qualcomm.com?part=11