发件人："Christian König" <[email protected]> 发送日期：2020-04-21 22:53:47 收件人："赵军奎" <[email protected]> 抄送人：Alex Deucher <[email protected]>,"David (ChunMing) Zhou" <[email protected]>,David Airlie <[email protected]>,Daniel Vetter <[email protected]>,Tom St Denis <[email protected]>,Ori Messinger <[email protected]>,Sam Ravnborg <[email protected]>,[email protected],[email protected],[email protected],[email protected] 主题：Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um 15:39 schrieb 赵军奎: >> 发件人："Christian König" <[email protected]> >> 发送日期：2020-04-21 21:02:27 >> 收件人："赵军奎" <[email protected]> >> 抄送人：Alex Deucher <[email protected]>,"David (ChunMing) Zhou" >> <[email protected]>,David Airlie <[email protected]>,Daniel Vetter >> <[email protected]>,Tom St Denis <[email protected]>,Ori Messinger >> <[email protected]>,Sam Ravnborg >> <[email protected]>,[email protected],[email protected],[email protected],[email protected] >> 主题：Re: [PATCH] amdgpu: fixes memleak issue when init failed>Am 21.04.20 um >> 14:09 schrieb 赵军奎: >>>> From: "Christian König" <[email protected]> >>>> Date: 2020-04-21 19:22:49 >>>> To: Bernard Zhao <[email protected]>,Alex Deucher >>>> <[email protected]>,"David (ChunMing) Zhou" >>>> <[email protected]>,David Airlie <[email protected]>,Daniel Vetter >>>> <[email protected]>,Tom St Denis <[email protected]>,Ori Messinger >>>> <[email protected]>,Sam Ravnborg >>>> <[email protected]>,[email protected],[email protected],[email protected] >>>> Cc: [email protected] >>>> 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. >
Emmm, i am still confused about two points: 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 <[email protected]> >>>>>> --- >>>>>> 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; >>>>>> } >>>>>> >>>>>> /** >> > _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
