Re: [PATCH 1/5] drm/amdgpu: Fix possible refcount leak for release of external_hw_fence

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 21.06.22 um 00:02 schrieb Andrey Grodzovsky:
> Problem:
> In amdgpu_job_submit_direct - The refcount should drop by 2
> but it drops only by 1.
>
> amdgpu_ib_sched->emit -> refcount 1 from first fence init
> dma_fence_get -> refcount 2
> dme_fence_put -> refcount 1
>
> Fix:
> Add put for external_hw_fence in amdgpu_job_free/free_cb

Well what is the external_hw_fence good for in this construct?

>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 10aa073600d4..58568fdde2d0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -152,8 +152,10 @@ static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>       /* only put the hw fence if has embedded fence */
>   	if (job->hw_fence.ops != NULL)
>   		dma_fence_put(&job->hw_fence);
> -	else
> +	else {

When one side of the if uses {} the other side should use {} as well, 
e.g. use } else { here.

Christian.

> +		dma_fence_put(job->external_hw_fence);
>   		kfree(job);
> +	}
>   }
>   
>   void amdgpu_job_free(struct amdgpu_job *job)
> @@ -165,8 +167,10 @@ void amdgpu_job_free(struct amdgpu_job *job)
>   	/* only put the hw fence if has embedded fence */
>   	if (job->hw_fence.ops != NULL)
>   		dma_fence_put(&job->hw_fence);
> -	else
> +	else {
> +		dma_fence_put(job->external_hw_fence);
>   		kfree(job);
> +	}
>   }
>   
>   int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,