Re: [PATCH v8 2/5] media: iris: Add hardware power on/off ops for X1P42100

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown ordering in `iris_vpu_power_off_hw()` disables power domains before hardware clocks.
--

commit 99a4b6ceed030f2e0c99e6828095cd2e45157242
Author: Wangao Wang <wangao.wang@oss.qualcomm.com>

media: iris: Add hardware power on/off ops for X1P42100

This commit wires an extra BSE clock into the power on/off sequence for
the Iris block on the X1P42100 platform. The BSE clock drives the Bin
Stream Engine and is required to be enabled separately from the core clock.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index c6cfc1d9fd9ec..a3238bf118eb9 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -224,6 +224,7 @@ void iris_vpu_power_off_hw(struct iris_core *core)
>  {
>  	dev_pm_genpd_set_hwmode(core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN], false);
>  	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +	iris_disable_unprepare_clock(core, IRIS_BSE_HW_CLK);

[Severity: High]
This isn't a bug introduced by this patch, but does this function disable
power domains before disabling the hardware clocks?

In iris_vpu_power_on_hw(), the sequence first enables the power domain,
then enables the clocks, and finally sets the hwmode. The error path of
iris_vpu_power_on_hw() correctly follows a last-in, first-out (LIFO) order
and disables the clocks before disabling the power domain.

However, iris_vpu_power_off_hw() calls iris_disable_power_domains() before
disabling the clocks. On ARM/Qualcomm platforms, turning off a power domain
(such as a GDSC) while its associated clocks are still running can prevent
the domain from fully collapsing, lead to bus hangs, or trigger
asynchronous external aborts (SErrors) when the clock controller
subsequently attempts to access unpowered registers.

By inserting the new IRIS_BSE_HW_CLK here, this patch perpetuates the
unsafe sequence. Should the teardown in iris_vpu_power_off_hw() be updated
to strictly mirror the reverse order used in the error path of
iris_vpu_power_on_hw()?

>  	iris_disable_unprepare_clock(core, IRIS_HW_AHB_CLK);
>  	iris_disable_unprepare_clock(core, IRIS_HW_CLK);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-enable_iris_on_purwa-v8-0-b1b9670459ab@oss.qualcomm.com?part=2