Messages in this thread Patch in this message | | | Subject | Re: [PATCH] amdgpu: fixes memleak issue when init failed | From | Christian König <> | Date | Tue, 21 Apr 2020 16:53:47 +0200 |
| |
Am 21.04.20 um 15:39 schrieb 赵军奎: > 发件人:"Christian König" <christian.koenig@amd.com> > 发送日期:2020-04-21 21:02:27 > 收件人:"赵军奎" <bernard@vivo.com> > 抄送人:Alex Deucher <alexander.deucher@amd.com>,"David (ChunMing) Zhou" <David1.Zhou@amd.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,Tom St Denis <tom.stdenis@amd.com>,Ori Messinger <Ori.Messinger@amd.com>,Sam Ravnborg <sam@ravnborg.org>,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org,opensource.kernel@vivo.com > 主题:Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um 14:09 schrieb 赵军奎: >>> From: "Christian König" <christian.koenig@amd.com> >>> Date: 2020-04-21 19:22:49 >>> To: Bernard Zhao <bernard@vivo.com>,Alex Deucher <alexander.deucher@amd.com>,"David (ChunMing) Zhou" <David1.Zhou@amd.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,Tom St Denis <tom.stdenis@amd.com>,Ori Messinger <Ori.Messinger@amd.com>,Sam Ravnborg <sam@ravnborg.org>,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org >>> Cc: opensource.kernel@vivo.com >>> Subject: Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um 13:17 schrieb Bernard Zhao: >>>>> VRAM manager and DRM MM when init failed, there is no operaction >>>>> to free kzalloc memory & remove device file. >>>>> This will lead to memleak & cause stability issue. >>>> NAK, failure to create sysfs nodes are not critical. >>>> >>>> Christian. >>>> >>> OK, get it. >>> By the way, should i modify this patch to just handle <kfree(mgr)> in error branch, or that it is also unnecessary? >> What you can do is to drop the "return ret" if anything with the sysfs >> nodes goes wrong and instead print the error code. > Emmm, for this part, i am not sure, my modify first print the error, secone release not free memory, > 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?
Yes, maybe an example makes it more clear what to do here. Currently we 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.
> >> 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@vivo.com> >>>>> --- >>>>> 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; >>>>> } >>>>> >>>>> /** >
| |