Re: [PATCH v8 04/12] media: iris: Add context bank hooks for platform specific initialization

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Timeout during core initialization causes a permanent resource leak of DMA queues and context bank devices upon subsequent initialization attempts.
- [High] Premature state transition to `IRIS_CORE_INIT` allows concurrent threads to operate on an uninitialized core.
- [High] Unprotected modification of shared hardware state leads to data races.
--

commit f9ddd8e9794631e591729427a306735897ceefd1
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add context bank hooks for platform specific initialization

This commit adds context bank initialization and deinitialization hooks
for the Iris VPU operations. This is necessary for platforms like glymur
that require dedicated firmware context bank devices where Linux manually
manages the firmware IOMMU mapping.

> diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> index 52bf56e517f91..89dbe9bbf6b0a 100644
> --- a/drivers/media/platform/qcom/iris/iris_core.c
> +++ b/drivers/media/platform/qcom/iris/iris_core.c
[ ... ]
> @@ -58,10 +59,14 @@ int iris_core_init(struct iris_core *core)
>  
>  	core->state = IRIS_CORE_INIT;

[Severity: High]
This is a pre-existing issue, but does setting the state to IRIS_CORE_INIT
before the hardware initialization completes allow concurrent threads to
operate on an uninitialized core?

After starting the boot process, the function drops core->lock to wait for a
completion signal in iris_wait_for_system_response(). If another thread
calls open() and thus iris_core_init() concurrently during this wait, it
will acquire the lock, see core->state == IRIS_CORE_INIT, immediately
return 0, and proceed to interact with the device before the VPU hardware
has finished booting.

> -	ret = iris_hfi_queues_init(core);
> +	ret = iris_init_cb_devs(core);
>  	if (ret)
>  		goto error;
[ ... ]
> @@ -94,6 +99,8 @@ int iris_core_init(struct iris_core *core)
>  	iris_vpu_power_off(core);
>  error_queue_deinit:
>  	iris_hfi_queues_deinit(core);
> +error_deinit_cb_devs:
> +	iris_deinit_cb_devs(core);
>  error:
>  	core->state = IRIS_CORE_DEINIT;

[Severity: High]
This isn't a bug introduced by this patch, but does the handling of the error
state here cause a resource leak?

When iris_wait_for_system_response() times out, it returns -ETIMEDOUT but
leaves resources unfreed since it executes after the cleanup jump labels in
iris_core_init(). 

On a subsequent open() call, iris_core_init() sees
core->state == IRIS_CORE_ERROR and jumps to this label, transitioning
the state to IRIS_CORE_DEINIT without performing teardown. A third open() will
then allocate resources again, leaking previously mapped DMA buffers and
potentially unbalancing power votes.

>  exit:

[Severity: High]
This is a pre-existing issue, but does iris_wait_for_system_response() modify
core->state locklessly and introduce a data race?

In iris_core_init(), the core->lock is dropped just before returning
iris_wait_for_system_response(). On a timeout, iris_wait_for_system_response()
writes to core->state:

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

Because core->state is evaluated and modified in other paths like
iris_core_deinit() under core->lock, modifying it locklessly on a timeout
could lead to inconsistent hardware teardown and potential use-after-free
if a concurrent iris_core_deinit() reads it just before the timeout fires.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-glymur-v8-0-1c79b9d51fc0@oss.qualcomm.com?part=4