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

[Christian König]

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_zh...@163.com>
Signed-off-by: Nirmoy Das <nirmoy....@amd.com>
---
  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: