Re: [PATCH] amdgpu: fixes memleak issue when init failed

[Christian KÃnig]

Am 22.04.20 um 17:51 schrieb Ruhl, Michael J:
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Bernard Zhao
> > Sent: Tuesday, April 21, 2020 7:17 AM
> > To: Alex Deucher <alexander.deucher@xxxxxxx>; Christian KÃnig
> > <christian.koenig@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>; Bernard
> > Zhao <bernard@xxxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
> > devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Cc: opensource.kernel@xxxxxxxx
> > Subject: [PATCH] amdgpu: fixes memleak issue when init failed
> >
> > 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.
> >
> > 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);
> Have you looked at the DEVICE_ATTR mechanism?

Yeah, I've thought about that as well. But didn't had time to look into 
detail if that could be applied here or not.

Regards,
Christian.

> It is set up to add device files.  You won't get the granularity of each file,
> but it has a lot more automatic-ness to setting this stuff up.
>
> Mike
>
> > 	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;
> > }
> >
> > /**
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx