Re: [PATCH v7 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] The `iris_vpu_boot_firmware` function incorrectly returns success (0) when a firmware boot error is detected.
- [High] Hardware clocks are unprepared after disabling the power domain, causing a teardown ordering violation in `iris_vpu_power_off_hw`.
--

--- Patch [11]: [PATCH v7 11/18] media: iris: Add framework support for AR50_LITE video core ---
commit 4e8fec9384d1018aa16fad1dc0afb88de83d8e99
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 implemtation or resue 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 void iris_vpu_ar50lt_power_off_hw(struct iris_core *core)
> +{
> +	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
> +	iris_disable_unprepare_clock(core, IRIS_THROTTLE_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_HW_AHB_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
> +	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +}

[Severity: High]
This is a pre-existing issue on the older platforms, but I noticed you are
correctly disabling the clocks before the power domains here for the new AR50LT
platform.

However, in the shared common function iris_vpu_power_off_hw() for the older
platforms, the power domain is disabled before the clocks:

drivers/media/platform/qcom/iris/iris_vpu_common.c:iris_vpu_power_off_hw() {
    iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
    iris_disable_unprepare_clock(core, IRIS_BSE_HW_CLK);
    iris_disable_unprepare_clock(core, IRIS_HW_AHB_CLK);
    ...
}

Does this teardown ordering violation in the older platforms cause system bus
hangs or unclocked access faults when the clock controller attempts to interact
with the unpowered block?

[ ... ]
> 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 isn't a bug introduced by this patch, but while modifying this function, I
noticed a potential issue in the firmware boot wait loop just above this code:

	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 the hardware reports an invalid uc_region, the loop breaks immediately.

Because the variable count will be less than max_tries, the subsequent timeout
check evaluates to false, and the function mistakenly falls through to return
success (0) here.

Can this bypass of the error return cause the driver to incorrectly assume the
firmware booted successfully and proceed to enable host interrupts, potentially
leading to unhandled timeouts, hardware hangs, or system crashes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-iris-ar50lt-v7-0-76af9dd4d1f6@oss.qualcomm.com?part=11