Re: [PATCH v7 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent calls to `iris_core_init` can bypass hardware initialization and use uninitialized hardware.
- [High] Failure during `iris_wait_for_system_response` leaves hardware initialized and memory allocated without cleanup, leading to permanent DMA memory leaks on subsequent opens.
--

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

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

This patch adds initialization and de-initialization hooks for platform-specific
context bank devices. These hooks are required for the glymur platform where
Linux runs as the hypervisor and the driver must manually manage 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
here before initialization completes create a concurrency flaw?

If a second thread calls iris_core_init() while the first thread unlocks
core->lock and waits for the system response:

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

	return iris_wait_for_system_response(core);
}

The second thread would see core->state == IRIS_CORE_INIT and return 0
immediately, bypassing hardware initialization and allowing user-space to
issue ioctls before the hardware finishes booting.

Can this cause firmware crashes or unhandled hardware states?

>  
> -	ret = iris_hfi_queues_init(core);
> +	ret = iris_init_cb_devs(core);
>  	if (ret)
>  		goto error;
>  
> +	ret = iris_hfi_queues_init(core);
> +	if (ret)
> +		goto error_deinit_cb_devs;
> +
>  	ret = iris_vpu_power_on(core);
>  	if (ret)
>  		goto error_queue_deinit;
[ ... ]
> @@ -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 if
iris_wait_for_system_response() times out, it sets
core->state = IRIS_CORE_ERROR and returns -ETIMEDOUT. Since the VFS release
is never called on an open failure, the queues and the newly added context
bank devices remain allocated.

When a second open() attempt calls iris_core_init(), it sees the ERROR state
and jumps to the error label:

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

At the error label, the state is simply reset to DEINIT without freeing any
previously allocated resources like queues or cb_devs. A third open() attempt
will see the DEINIT state and proceed with a full initialization, allocating
new DMA memory and overwriting the old pointers.

Does this permanently leak the previously allocated resources?

>  exit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-glymur-v7-0-afaa55d11fe0@oss.qualcomm.com?part=4