[PATCH 05/10] drm/amdgpu:fix memleak in takedown

[Liu, Monk]

Actually " drm_mm_takedown" below will give you the warning if some BO is still in MM
That means below part is totally useless:

>>   -    spin_lock(&mgr->lock);
>> -    if (!drm_mm_clean(&mgr->mm)) {
>> -        spin_unlock(&mgr->lock);
>> -        return -EBUSY;
>> -    }
>> -

And the worse thing is this "return -EBUSY" would lead to memory leak because it blocks following kfree() on mgr...
BR Monk

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com] 
Sent: 2017年11月14日 20:01
To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 05/10] drm/amdgpu:fix memleak in takedown

Am 14.11.2017 um 12:52 schrieb Christian König:
> Am 14.11.2017 um 10:07 schrieb Monk Liu:
>> this can fix the memory leak under the case that not all BO are freed 
>> during "takedown" stage, because originally it blocks following kfree 
>> on mgr.
>
> NAK, all BOs must be freed before we take down at least the GTT manager.

Ah, wait a second. That is the address space manager here!

Mhm, returning -EBUSY is probably not a good idea, but we should at least print some warning.

Regards,
Christian.

> That we run into a memory leak is intended, cause that is still better 
> than crashing the system because we access already freed up memory.
>
> Regards,
> Christian.
>
>>
>> Change-Id: I56e832ac317215a4d9f58edb8e75ea722158715c
>> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c  | 6 ------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 5 -----
>>   2 files changed, 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> index 88709f9..72819cc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>> @@ -76,12 +76,6 @@ static int amdgpu_gtt_mgr_fini(struct 
>> ttm_mem_type_manager *man)
>>   {
>>       struct amdgpu_gtt_mgr *mgr = man->priv;
>>   -    spin_lock(&mgr->lock);
>> -    if (!drm_mm_clean(&mgr->mm)) {
>> -        spin_unlock(&mgr->lock);
>> -        return -EBUSY;
>> -    }
>> -
>>       drm_mm_takedown(&mgr->mm);
>>       spin_unlock(&mgr->lock);
>>       kfree(mgr);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> index 4ca622b..3e883e5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> @@ -68,11 +68,6 @@ static int amdgpu_vram_mgr_fini(struct 
>> ttm_mem_type_manager *man)
>>       struct amdgpu_vram_mgr *mgr = man->priv;
>>         spin_lock(&mgr->lock);
>> -    if (!drm_mm_clean(&mgr->mm)) {
>> -        spin_unlock(&mgr->lock);
>> -        return -EBUSY;
>> -    }
>> -
>>       drm_mm_takedown(&mgr->mm);
>>       spin_unlock(&mgr->lock);
>>       kfree(mgr);
>
>