Re: [PATCH 3/4] media: iris: Fix power-off ordering to disable power domain after clocks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free of `iris_core` when user-space holds V4L2 device open during driver unbind.
- [High] Incomplete fix for power-off hardware hangs in nested VPU3x functions.
- [High] Use-After-Free due to missing cancellation of `sys_error_handler` delayed work.
- [High] AB-BA deadlock between `disable_irq()` and `core->lock`.
- [High] Hardware hang due to premature OPP rate drop nullifying the patch's clock ordering fix.
- [High] Hardware resets are triggered on unclocked and unpowered domains.
--

--- Patch [3]: [PATCH 3/4] media: iris: Fix power-off ordering to disable power domain after clocks ---
commit 21b9f8f2d0a12300785d96896a5ff556ccf1532a
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Fix power-off ordering to disable power domain after clocks

This patch reorders the power-off sequence in iris_vpu_power_off_hw() to
ensure that clocks are disabled before removing the power domain. This avoids
hardware hangs that occur when disabling clock controllers without power.

Cc: stable@vger.kernel.org

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index e4847c1077097..3cd493b06d84c 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -223,10 +223,10 @@ int iris_vpu_power_off_controller(struct iris_core *core)
>  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);
>  	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]);
>  }
>  
>  void iris_vpu_power_off(struct iris_core *core)
>  {
>  	iris_opp_set_rate(core->dev, 0);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
iris_opp_set_rate() drop the OPP performance state and voltage before
power_off_hw() disables the active hardware clocks?

If high-frequency clocks run with insufficient voltage before being disabled,
could this negate the structural fix and cause the same hardware hang during
suspension or teardown?

>  	core->iris_platform_data->vpu_ops->power_off_hw(core);
>  	core->iris_platform_data->vpu_ops->power_off_controller(core);
>  	iris_unset_icc_bw(core);
>  
>  	if (!iris_vpu_watchdog(core, core->intr_status))
>  		disable_irq(core->irq);

[Severity: High]
This is a pre-existing issue, but does disable_irq() synchronize and wait for
the threaded IRQ handler to complete while holding core->lock?

If iris_core_deinit() holds core->lock and calls iris_vpu_power_off():

iris_core_deinit()
    mutex_lock(&core->lock);
    iris_vpu_power_off(core);
        disable_irq(core->irq);

And concurrently, a hardware interrupt fires and wakes the threaded handler:

iris_hfi_isr_handler()
    mutex_lock(&core->lock);

Will this cause a deadlock between the interrupt handler and
iris_core_deinit()?

>  }

While reviewing this, I noticed similar pre-existing issues in other areas of
the driver:

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar pattern in
iris_vpu3x.c where the power domain is disabled before clocks:

iris_vpu33_power_off_controller() {
...
disable_power:
	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);
	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
...
}

Does this leave the same reverse-ordering problem where IRIS_CTRL_POWER_DOMAIN
is disabled before IRIS_AXI_CLK is unprepared?

Similarly, in iris_vpu35_power_off_hw():

iris_vpu35_power_off_hw() {
...
	iris_vpu33_power_off_hardware(core);

	iris_disable_unprepare_clock(core, IRIS_HW_FREERUN_CLK);
	iris_disable_unprepare_clock(core, IRIS_AXI_CLK);
...
}

Does disabling the clocks after iris_vpu33_power_off_hardware() has already
removed the hardware power domain result in the same hardware hang?

[Severity: High]
This is a pre-existing issue, but in iris_vpu_common.c, does triggering
reset_control_bulk_reset() after the clocks and power domain have already been
disabled violate hardware constraints and risk a bus lockup?

iris_vpu35_vpu4x_power_off_controller() {
...
disable_power:
	iris_disable_unprepare_clock(core, IRIS_CTRL_CLK);
...
	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);

	reset_control_bulk_reset(clk_rst_tbl_size, core->resets);
...
}

[Severity: Critical]
This isn't a bug introduced by this patch, but in iris_probe.c, if user-space
holds a file descriptor open on the video node during driver unbind, will
devres free the core structure while the video_device stays alive?

iris_remove() {
...
	iris_core_deinit(core);
	video_unregister_device(core->vdev_dec);
	video_unregister_device(core->vdev_enc);
	v4l2_device_unregister(&core->v4l2_dev);
	mutex_destroy(&core->lock);
...
}

If the file descriptor is subsequently closed, iris_remove_session() is
invoked:

iris_remove_session() {
...
	struct iris_core *core = inst->core;
	struct iris_inst *iter, *temp;

	mutex_lock(&core->lock);
...
}

Will this result in a use-after-free when attempting to acquire core->lock?

[Severity: High]
This is a pre-existing issue, but in iris_remove(), is there a missing call
to cancel_delayed_work_sync(&core->sys_error_handler)?

If devres frees core, and the delayed work subsequently runs
iris_sys_error_handler(), could this also trigger a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-iris-fixes-dma-pseq-fint-v1-0-aba0cb22f6ab@oss.qualcomm.com?part=3