Re: [PATCH v8 07/18] media: iris: add vpu op hook to disable ARP buffer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race conditions and data races in `iris_core_init` state machine allow concurrent callers to bypass initialization wait and modify `core->state` unprotected.
- [High] The `iris_core_init()` function fails to clean up initialized resources (such as DMA memory for queues, firmware mappings, and VPU power) if the hardware fails to respond in time.
--

--- Patch [7]: [PATCH v8 07/18] media: iris: add vpu op hook to disable ARP buffer ---
commit 53560a9c747444f7e2c1b9214a99baec4904f498
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: add vpu op hook to disable ARP buffer

This patch adds a disable_arp callback to the VPU operations for the iris
driver. This optional callback is invoked during core initialization and
resume paths to disable ARP routing on AR50LT platforms, allowing the
firmware to configure the ARP internal buffer as non-secure for encoder
usage.

> diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> index 6dbe18be5b495..526f925752646 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.c
> +++ b/drivers/media/platform/qcom/iris/iris_core.c
> @@ -46,6 +46,7 @@ static int iris_wait_for_system_response(struct iris_core *core)
>  
>  int iris_core_init(struct iris_core *core)
>  {
> +	const struct vpu_ops *vpu_ops = core->iris_platform_data->vpu_ops;
>  	int ret;
>  
>  	mutex_lock(&core->lock);

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition in
the state machine of iris_core_init()?

If one thread calls iris_core_init(), it takes core->lock, sets
core->state = IRIS_CORE_INIT, drops the lock, and blocks in
iris_wait_for_system_response():

drivers/media/platform/qcom/iris/iris_core.c:iris_core_init() {
    ...
    core->state = IRIS_CORE_INIT;
    ...
    mutex_unlock(&core->lock);

    return iris_wait_for_system_response(core);
}

Concurrently, if another thread calls iris_core_init(), it can see
core->state == IRIS_CORE_INIT and return success immediately without waiting
for firmware readiness:

drivers/media/platform/qcom/iris/iris_core.c:iris_core_init() {
    mutex_lock(&core->lock);
    if (core->state == IRIS_CORE_INIT) {
        ret = 0;
        goto exit;
    }
    ...
}

Additionally, if the first thread's wait times out,
iris_wait_for_system_response() modifies core->state without holding
core->lock:

drivers/media/platform/qcom/iris/iris_core.c:iris_wait_for_system_response() {
    ...
    if (!ret) {
        core->state = IRIS_CORE_ERROR;
        return -ETIMEDOUT;
    }
    ...
}

Can this lead to a data race with other threads checking or modifying the
state, and allow threads to bypass initialization wait?

[Severity: High]
This is a pre-existing issue, but does iris_core_init() fail to clean up
initialized resources if the hardware fails to respond in time?

In iris_core_init(), if iris_wait_for_system_response() returns an error
(e.g., -ETIMEDOUT), it doesn't trigger the teardown labels:

drivers/media/platform/qcom/iris/iris_core.c:iris_core_init() {
    ...
    mutex_unlock(&core->lock);

    return iris_wait_for_system_response(core);
    ...
}

If wait_for_completion_timeout() expires, iris_wait_for_system_response()
returns -ETIMEDOUT and sets core->state to IRIS_CORE_ERROR:

drivers/media/platform/qcom/iris/iris_core.c:iris_wait_for_system_response() {
    ...
    if (!ret) {
        core->state = IRIS_CORE_ERROR;
        return -ETIMEDOUT;
    }
    ...
}

If user space retries, the next call to iris_core_init() sees
core->state == IRIS_CORE_ERROR, jumps to the error label, and sets the
state to IRIS_CORE_DEINIT without releasing the resources leaked in the
first attempt:

drivers/media/platform/qcom/iris/iris_core.c:iris_core_init() {
    ...
    } else if (core->state == IRIS_CORE_ERROR) {
        ret = -EINVAL;
        goto error;
    }
    ...
error:
    core->state = IRIS_CORE_DEINIT;
    ...
}

A third attempt would then see IRIS_CORE_DEINIT, re-run initialization,
allocate new DMA memory (via iris_hfi_queues_init), and overwrite the
previous pointers, resulting in a permanent memory leak.

Does this sequence cause a memory leak when the firmware hangs?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-iris-ar50lt-v8-0-6597c08cea6f@oss.qualcomm.com?part=7