Re: [PATCH] accel/ivpu: Use threaded IRQ for IPC callback processing

["Wachowski, Karol" <karol.wachowski@linux.intel.com>]

On 11-Jun-26 8:09, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Firmware command queue leak and duplicate hardware initialization on the retry path in `ivpu_job_submit`.
> --

This is pre-existing issue that will exhibit even without this patch.
It is already addressed with:
https://lore.kernel.org/all/20260611055140.948684-1-karol.wachowski@linux.intel.com/

Karol

> 
> commit 91623d3f3cc1b38781bd2f9f2354ef0fa3591d50
> Author: Karol Wachowski <karol.wachowski@linux.intel.com>
> 
> accel/ivpu: Use threaded IRQ for IPC callback processing
> 
> Dispatching IPC callbacks from system_percpu_wq adds scheduling latency
> that is neither bounded nor predictable, which hurts job completion
> turnaround. Handle them from a threaded IRQ instead, and defer job
> resource teardown to a dedicated unbounded workqueue.
> 
>> diff --git a/drivers/accel/ivpu/ivpu_job.c b/drivers/accel/ivpu/ivpu_job.c
>> index 521931d1f7fca..7320c97762832 100644
>> --- a/drivers/accel/ivpu/ivpu_job.c
>> +++ b/drivers/accel/ivpu/ivpu_job.c
> [ ... ]
>> @@ -709,6 +748,14 @@ static int ivpu_job_submit(struct ivpu_job *job, u8 priority, u32 cmdq_id)
>>   	}
>>   
>>   	ret = ivpu_cmdq_register(file_priv, cmdq);
>> +	if (ret == -EBUSY && !flushed) {
>> +		/* Doorbell may be held by jobs pending deferred cleanup */
>> +		mutex_unlock(&file_priv->lock);
>> +		mutex_unlock(&vdev->submitted_jobs_lock);
>> +		flush_work(&vdev->job_destroy_work);
>> +		flushed = true;
>> +		goto retry;
>> +	}
> 
> [Severity: High]
> Does this retry loop cause duplicate firmware queue initialization and leak
> resources?
> 
> If ivpu_cmdq_register() successfully calls ivpu_hws_cmdq_init() but then
> ivpu_register_db() fails with -EBUSY, the function returns -EBUSY without
> undoing the firmware queue initialization and leaves cmdq->db_id as 0:
> 
> drivers/accel/ivpu/ivpu_job.c:ivpu_cmdq_register() {
>      ...
> 	if (vdev->fw->sched_mode == VPU_SCHEDULING_MODE_HW) {
> 		ret = ivpu_hws_cmdq_init(file_priv, cmdq, VPU_ENGINE_COMPUTE, cmdq->priority);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	ret = ivpu_register_db(file_priv, cmdq);
> 	if (ret)
> 		return ret;  // Returns -EBUSY here, db_id is 0
>      ...
> }
> 
> When the goto retry path is taken, ivpu_cmdq_register() is called again
> with the same cmdq. Because cmdq->db_id is still 0, the early bailout is
> skipped, and ivpu_hws_cmdq_init() is called a second time.
> 
> Furthermore, if the retry also fails (or is not attempted), will this
> leak the firmware resource? The unregister path seems to bail early if
> db_id is 0:
> 
> drivers/accel/ivpu/ivpu_job.c:ivpu_cmdq_unregister() {
> 	if (!cmdq->db_id)
> 		return 0;
>      ... // ivpu_jsm_hws_destroy_cmdq() is skipped
> }
> 
>>   	if (ret) {
>>   		ivpu_err(vdev, "Failed to register command queue: %d\n", ret);
>>   		goto err_unlock;
>