Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 13.10.21 um 17:09 schrieb Nirmoy Das:
> GTT BO cleanup code is with in the test for loop and
> we would skip cleaning up GTT BO on success.
>
> Reported-by: zhang <botton_zhang@xxxxxxx>
> Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25 ++++++++++++------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> index 909d830b513e..5fe7ff680c29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>   	struct amdgpu_bo *vram_obj = NULL;
>   	struct amdgpu_bo **gtt_obj = NULL;
>   	struct amdgpu_bo_param bp;
> +	struct dma_fence *fence = NULL;
>   	uint64_t gart_addr, vram_addr;
>   	unsigned n, size;
>   	int i, r;
> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>   		void *gtt_map, *vram_map;
>   		void **gart_start, **gart_end;
>   		void **vram_start, **vram_end;
> -		struct dma_fence *fence = NULL;
>   
>   		bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>   		r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>   
>   		DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
>   			 gart_addr - adev->gmc.gart_start);
> -		continue;
> +	}
>   
> +	--i;
>   out_lclean_unpin:
> -		amdgpu_bo_unpin(gtt_obj[i]);
> +	amdgpu_bo_unpin(gtt_obj[i]);
>   out_lclean_unres:
> -		amdgpu_bo_unreserve(gtt_obj[i]);
> +	amdgpu_bo_unreserve(gtt_obj[i]);
>   out_lclean_unref:
> -		amdgpu_bo_unref(&gtt_obj[i]);
> +	amdgpu_bo_unref(&gtt_obj[i]);
>   out_lclean:
> -		for (--i; i >= 0; --i) {
> -			amdgpu_bo_unpin(gtt_obj[i]);
> -			amdgpu_bo_unreserve(gtt_obj[i]);
> -			amdgpu_bo_unref(&gtt_obj[i]);
> -		}
> -		if (fence)
> -			dma_fence_put(fence);
> -		break;
> +	for (--i; i >= 0; --i) {

The usual idiom for cleanups like that is "while (i--)..." because that 
also works with an unsigned i.

Apart from that looks good to me.

Christian.

> +		amdgpu_bo_unpin(gtt_obj[i]);
> +		amdgpu_bo_unreserve(gtt_obj[i]);
> +		amdgpu_bo_unref(&gtt_obj[i]);
>   	}
> +	if (fence)
> +		dma_fence_put(fence);
>   
>   	amdgpu_bo_unpin(vram_obj);
>   out_unres: