Re: [PATCH v9 07/14] media: iris: Replace enum-indexed clock and power domain tables with per-block structures

[sashiko-bot@kernel.org]

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

New issues:
- [High] Bundling global device OPP rate adjustments into per-block helpers drops the global OPP rate to 0 on error paths, potentially crashing active hardware blocks.
- [Medium] Multi-core platforms will fail to power on/off secondary hardware cores due to implicit array-to-pointer decay.
- [Medium] Unconditional use of optional power domain structures in VPU4x hardware enable/disable paths leads to NULL pointer dereferences.

Pre-existing issues:
- [Critical] The `sys_error_handler` delayed work is not canceled during driver removal, leading to a Use-After-Free of the `core` structure.
--

--- Patch [7]: [PATCH v9 07/14] media: iris: Replace enum-indexed clock and power domain tables with per-block structures ---
Note: The format of this review is altered to provide a free-form summary
of the findings due to restrictions on quoting the original patch text.

Commit: 67736d0c8e0f502135f0c4a867f560515cee6311
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
Subject: media: iris: Replace enum-indexed clock and power domain tables...

This patch refactors the Iris driver to replace flat, enum-indexed arrays
with per-block data structures bundling power domains and clocks together.
It aims to simplify platform extension by operating directly on the block
instances during enable and disable paths.

[Severity: Medium]
In drivers/media/platform/qcom/iris/iris_vpu_common.c inside
iris_vpu_power_on_hw(), and in iris_vpu4x.c inside
iris_vpu4x_power_on_hardware(), the code passes the core->vcodec pointer
directly to iris_enable_power_domain_and_clocks(). Since core->vcodec is
allocated as an array based on plat->num_cores during initialization, does
passing it directly cause it to decay to a pointer to the first element?
If so, wouldn't this leave secondary hardware cores unpowered on multi-core
platforms?

[Severity: High]
Looking at iris_enable_power_domain_and_clocks() in iris_resources.c, the
error path calls iris_opp_set_rate(core->dev, 0). Because core->dev manages
the global OPP state for the entire Iris core, dropping the rate to 0 here
affects all blocks. If enabling a secondary domain like vcodec_vpp0 fails,
could this starve the already-enabled primary vcodec block of voltage while
its clocks are still running, potentially hanging or crashing the hardware?

[Severity: Medium]
In iris_vpu4x_power_on_hardware() in iris_vpu4x.c, the code checks the
hardware efuse to decide whether to enable vcodec_vpp0. It then passes
core->vcodec_vpp0 to the enable helper. If a platform omits the optional
vcodec_vpp0_data but the efuse still indicates it is present, the pointer
will be NULL. Since the helper immediately dereferences pd->pd_cnt without
any NULL checks, does this introduce a NULL pointer dereference?

[Severity: Critical]
This is a pre-existing issue, but looking at iris_remove() in iris_probe.c,
it seems the sys_error_handler delayed work is never canceled before the
driver is torn down. If driver unbind or module unload occurs right after a
fatal system error queues this work, the core structure will be freed by
devres upon returning. Could this result in a use-after-free when the
delayed work subsequently executes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-glymur-v9-0-8cf2cbe12a07@oss.qualcomm.com?part=7