Re: [PATCH v6 1/2] accel/rocket: Fix error path handling in rocket_job_run()

[赵金明 <zhaojinming@uniontech.com>]

Hi,

Apologies for the confusion caused. I was trying to incorporate feedback from the AI to make my changes safer, but the result seems unsatisfactory.

I'd like to split the two independent fixes and tackle them separately. Let's first review [PATCH v6 1/2] accel/rocket: Fix error path handling in rocket_job_run().

I currently don't fully grasp the AI's problem description regarding [PATCH v6 2/2] accel/rocket: Fix iommu_group leak and unsafe IRQ register access.

Please let me know if I should start a new thread for [PATCH v6 1/2].

Thanks,


--------------



Zhao Jinming


>Hi,



>



>Le mercredi 10 juin 2026 à 15:10 +0800, ZhaoJinming a écrit?:



>> In rocket_job_run(), after taking an extra fence reference for



>> job->done_fence via dma_fence_get(), the error paths have three bugs:



>> 



>> - The dma_fence reference held by job->done_fence is never released,



>> ? causing a reference leak.



>> - pm_runtime_get_sync() increments the usage counter even on failure,



>> ? but the error path does not decrement it, leaking the runtime PM



>> ? reference and preventing the NPU from suspending.



>> - A valid but unsignaled fence is returned to the DRM scheduler,



>> ? which triggers WARN("Fence ... released with pending signals!")



>> ? when the scheduler drops its reference.



>> 



>> Fix by replacing pm_runtime_get_sync() with pm_runtime_resume_and_get()



>> which auto-balances the usage counter on failure, releasing both fence



>> references on error, and returning ERR_PTR(ret) instead of the



>> unsignaled fence.



>> 



>> Cc: stable@vger.kernel.org



>> Fixes: 0810d5ad88a1 ("accel/rocket: Add job submission IOCTL")



>> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>



>> ---



>



>This is a lot of versions within the same day. You should slow down a little so



>a human can provide a review, and also document the differences in this section,



>after the ---, or using a cover letter.



>



>Nicolas



>



>> ?drivers/accel/rocket/rocket_job.c | 19 ++++++++++++++-----



>> ?1 file changed, 14 insertions(+), 5 deletions(-)



>> 



>> diff --git a/drivers/accel/rocket/rocket_job.c



>> b/drivers/accel/rocket/rocket_job.c



>> index ac51bff39833..e8a073e22ac2 100644



>> --- a/drivers/accel/rocket/rocket_job.c



>> +++ b/drivers/accel/rocket/rocket_job.c



>> @@ -310,13 +310,22 @@ static struct dma_fence *rocket_job_run(struct



>> drm_sched_job *sched_job)



>> ?		dma_fence_put(job->done_fence);



>> ?	job->done_fence = dma_fence_get(fence);



>> ?



>> -	ret = pm_runtime_get_sync(core->dev);



>> -	if (ret < 0)



>> -		return fence;



>> +	ret = pm_runtime_resume_and_get(core->dev);



>> +	if (ret < 0) {



>> +		dma_fence_put(job->done_fence);



>> +		job->done_fence = NULL;



>> +		dma_fence_put(fence);



>> +		return ERR_PTR(ret);



>> +	}



>> ?



>> ?	ret = iommu_attach_group(job->domain->domain, core->iommu_group);



>> -	if (ret < 0)



>> -		return fence;



>> +	if (ret < 0) {



>> +		pm_runtime_put(core->dev);



>> +		dma_fence_put(job->done_fence);



>> +		job->done_fence = NULL;



>> +		dma_fence_put(fence);



>> +		return ERR_PTR(ret);



>> +	}



>> ?



>> ?	scoped_guard(mutex, &core->job_lock) {



>> ?		core->in_flight_job = job;