åääï"Christian KÃnig" <christian.koenig@xxxxxxx>
åéææï2020-04-21 22:53:47
æääï"èåå" <bernard@xxxxxxxx>
æéäïAlex Deucher <alexander.deucher@xxxxxxx>,"David (ChunMing) Zhou" <David1.Zhou@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,Tom St Denis <tom.stdenis@xxxxxxx>,Ori Messinger <Ori.Messinger@xxxxxxx>,Sam Ravnborg <sam@xxxxxxxxxxxx>,amd-gfx@xxxxxxxxxxxxxxxxxxxxx,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx
äéïRe: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um 15:39 schrieb èåå:
Emmm, i am still confused about two points:åääï"Christian KÃnig" <christian.koenig@xxxxxxx>Yes, maybe an example makes it more clear what to do here. Currently we
åéææï2020-04-21 21:02:27
æääï"èåå" <bernard@xxxxxxxx>
æéäïAlex Deucher <alexander.deucher@xxxxxxx>,"David (ChunMing) Zhou" <David1.Zhou@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,Tom St Denis <tom.stdenis@xxxxxxx>,Ori Messinger <Ori.Messinger@xxxxxxx>,Sam Ravnborg <sam@xxxxxxxxxxxx>,amd-gfx@xxxxxxxxxxxxxxxxxxxxx,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,opensource.kernel@xxxxxxxx
äéïRe: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um 14:09 schrieb èåå:
Emmm, for this part, i am not sure, my modify first print the error, secone release not free memory,From: "Christian KÃnig" <christian.koenig@xxxxxxx>What you can do is to drop the "return ret" if anything with the sysfs
Date: 2020-04-21 19:22:49
To: Bernard Zhao <bernard@xxxxxxxx>,Alex Deucher <alexander.deucher@xxxxxxx>,"David (ChunMing) Zhou" <David1.Zhou@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,Tom St Denis <tom.stdenis@xxxxxxx>,Ori Messinger <Ori.Messinger@xxxxxxx>,Sam Ravnborg <sam@xxxxxxxxxxxx>,amd-gfx@xxxxxxxxxxxxxxxxxxxxx,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx
Cc: opensource.kernel@xxxxxxxx
Subject: Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um 13:17 schrieb Bernard Zhao:
OK, get it.VRAM manager and DRM MM when init failed, there is no operactionNAK, failure to create sysfs nodes are not critical.
to free kzalloc memory & remove device file.
This will lead to memleak & cause stability issue.
Christian.
By the way, should i modify this patch to just handle <kfree(mgr)> in error branch, or that it is also unnecessary?
nodes goes wrong and instead print the error code.
and last return error, make everything clear to the system.
I think it`s the same with what you mentioned, is there something that I misunderstood?
print and error and return when something with the sysfs files goes wrong:
if (ret) {
ÂÂÂ DRM_ERROR("Failed to create device file mem_info_vram_total\n");
ÂÂÂ return ret;
}
But what we should do instead is just to print an error and continue and
in the end return success status:
if (ret)
ÂÂÂ DRM_ERROR("Failed to create device file mem_info_vram_total
(%d)\n", r);
...
return 0;
Regards,
Christian.
1 Does that mean there is no failed case in this function?
2 There is no need to free the kzmalloc space(no possibility of memory leak )?
Regards,
Bernard
It's really annoying that loading, unloading and loading the driver
again sometimes fails because we have a bug in the sysfs files cleanup.
We certainly should fix those bugs as well, but they are just not
critical for correct driver functionality.
Regards,
Christian.
Regards,
Bernard
Signed-off-by: Bernard Zhao <bernard@xxxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 24 ++++++++++++++++----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 82a3299e53c0..4c5fb153e6b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -175,30 +175,44 @@ static int amdgpu_vram_mgr_init(struct ttm_mem_type_manager *man,
ret = device_create_file(adev->dev, &dev_attr_mem_info_vram_total);
if (ret) {
DRM_ERROR("Failed to create device file mem_info_vram_total\n");
- return ret;
+ goto VRAM_TOTAL_FAIL;
}
ret = device_create_file(adev->dev, &dev_attr_mem_info_vis_vram_total);
if (ret) {
DRM_ERROR("Failed to create device file mem_info_vis_vram_total\n");
- return ret;
+ goto VIS_VRAM_TOTA_FAIL;
}
ret = device_create_file(adev->dev, &dev_attr_mem_info_vram_used);
if (ret) {
DRM_ERROR("Failed to create device file mem_info_vram_used\n");
- return ret;
+ goto VRAM_USED_FAIL;
}
ret = device_create_file(adev->dev, &dev_attr_mem_info_vis_vram_used);
if (ret) {
DRM_ERROR("Failed to create device file mem_info_vis_vram_used\n");
- return ret;
+ goto VIS_VRAM_USED_FAIL;
}
ret = device_create_file(adev->dev, &dev_attr_mem_info_vram_vendor);
if (ret) {
DRM_ERROR("Failed to create device file mem_info_vram_vendor\n");
- return ret;
+ goto VRAM_VERDOR_FAIL;
}
return 0;
+
+VRAM_VERDOR_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram_used);
+VIS_VRAM_USED_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vram_used);
+RVAM_USED_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vis_vram_total);
+VIS_VRAM_TOTA_FAIL:
+ device_remove_file(adev->dev, &dev_attr_mem_info_vram_total);
+VRAM_TOTAL_FAIL:
+ kfree(mgr);
+ man->priv = NULL;
+
+ return ret;
}
/**