* [PATCH] drm/amdgpu: fix multiple memory leaks @ 2019-09-18 16:09 ` Navid Emamdoost 0 siblings, 0 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-09-18 16:09 UTC (permalink / raw) Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Alex Deucher, Christian König, David (ChunMing) Zhou, David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel In acp_hw_init there are some allocations that needs to be released in case of failure: 1- adev->acp.acp_genpd should be released if any allocation attemp for adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. 2- all of those allocations should be released if pm_genpd_add_device fails. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c index eba42c752bca..dd3fa85b11c5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c @@ -231,17 +231,21 @@ static int acp_hw_init(void *handle) adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), GFP_KERNEL); - if (adev->acp.acp_cell == NULL) + if (adev->acp.acp_cell == NULL) { + kfree(adev->acp.acp_genpd); return -ENOMEM; + } adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); if (adev->acp.acp_res == NULL) { + kfree(adev->acp.acp_genpd); kfree(adev->acp.acp_cell); return -ENOMEM; } i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); if (i2s_pdata == NULL) { + kfree(adev->acp.acp_genpd); kfree(adev->acp.acp_res); kfree(adev->acp.acp_cell); return -ENOMEM; @@ -348,6 +352,10 @@ static int acp_hw_init(void *handle) r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); if (r) { dev_err(dev, "Failed to add dev to genpd\n"); + kfree(adev->acp.acp_genpd); + kfree(adev->acp.acp_res); + kfree(adev->acp.acp_cell); + kfree(i2s_pdata); return r; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH] drm/amdgpu: fix multiple memory leaks @ 2019-09-18 16:09 ` Navid Emamdoost 0 siblings, 0 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-09-18 16:09 UTC (permalink / raw) Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Alex Deucher, Christian König, David (ChunMing) Zhou, David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel In acp_hw_init there are some allocations that needs to be released in case of failure: 1- adev->acp.acp_genpd should be released if any allocation attemp for adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. 2- all of those allocations should be released if pm_genpd_add_device fails. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c index eba42c752bca..dd3fa85b11c5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c @@ -231,17 +231,21 @@ static int acp_hw_init(void *handle) adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), GFP_KERNEL); - if (adev->acp.acp_cell == NULL) + if (adev->acp.acp_cell == NULL) { + kfree(adev->acp.acp_genpd); return -ENOMEM; + } adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); if (adev->acp.acp_res == NULL) { + kfree(adev->acp.acp_genpd); kfree(adev->acp.acp_cell); return -ENOMEM; } i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); if (i2s_pdata == NULL) { + kfree(adev->acp.acp_genpd); kfree(adev->acp.acp_res); kfree(adev->acp.acp_cell); return -ENOMEM; @@ -348,6 +352,10 @@ static int acp_hw_init(void *handle) r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); if (r) { dev_err(dev, "Failed to add dev to genpd\n"); + kfree(adev->acp.acp_genpd); + kfree(adev->acp.acp_res); + kfree(adev->acp.acp_cell); + kfree(i2s_pdata); return r; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/amdgpu: fix multiple memory leaks 2019-09-18 16:09 ` Navid Emamdoost (?) @ 2019-09-18 17:31 ` Koenig, Christian [not found] ` <7bab24ff-ded7-9f76-ba25-efd07cdd30dd-5C7GfCeVMHo@public.gmane.org> 2019-09-18 19:05 ` [PATCH v2] " Navid Emamdoost -1 siblings, 2 replies; 24+ messages in thread From: Koenig, Christian @ 2019-09-18 17:31 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel Am 18.09.19 um 18:09 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if pm_genpd_add_device > fails. Good catch, but please use goto error handling instead of adding more and more kfree calls. Regards, Christian. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..dd3fa85b11c5 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -231,17 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > + if (adev->acp.acp_cell == NULL) { > + kfree(adev->acp.acp_genpd); > return -ENOMEM; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > + kfree(adev->acp.acp_genpd); > kfree(adev->acp.acp_cell); > return -ENOMEM; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > + kfree(adev->acp.acp_genpd); > kfree(adev->acp.acp_res); > kfree(adev->acp.acp_cell); > return -ENOMEM; > @@ -348,6 +352,10 @@ static int acp_hw_init(void *handle) > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > + kfree(adev->acp.acp_genpd); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(i2s_pdata); > return r; > } > } ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <7bab24ff-ded7-9f76-ba25-efd07cdd30dd-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: fix multiple memory leaks [not found] ` <7bab24ff-ded7-9f76-ba25-efd07cdd30dd-5C7GfCeVMHo@public.gmane.org> @ 2019-09-18 19:04 ` Navid Emamdoost 0 siblings, 0 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-09-18 19:04 UTC (permalink / raw) To: Koenig, Christian Cc: Zhou, David(ChunMing), David Airlie, Sam Ravnborg, kjlu-OJFnDUYgAso, linux-kernel-u79uwXL29TY76Z2rM5mHXA, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, emamd001-OJFnDUYgAso, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter, smccaman-OJFnDUYgAso, Deucher, Alexander, Rex Zhu [-- Attachment #1.1: Type: text/plain, Size: 2594 bytes --] Thanks Christian for the feedback, I'll send a v2. On Wed, Sep 18, 2019 at 12:31 PM Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org> wrote: > Am 18.09.19 um 18:09 schrieb Navid Emamdoost: > > In acp_hw_init there are some allocations that needs to be released in > > case of failure: > > > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > > 2- all of those allocations should be released if pm_genpd_add_device > > fails. > > Good catch, but please use goto error handling instead of adding more > and more kfree calls. > > Regards, > Christian. > > > > > Signed-off-by: Navid Emamdoost <navid.emamdoost-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > index eba42c752bca..dd3fa85b11c5 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > @@ -231,17 +231,21 @@ static int acp_hw_init(void *handle) > > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > > GFP_KERNEL); > > > > - if (adev->acp.acp_cell == NULL) > > + if (adev->acp.acp_cell == NULL) { > > + kfree(adev->acp.acp_genpd); > > return -ENOMEM; > > + } > > > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), > GFP_KERNEL); > > if (adev->acp.acp_res == NULL) { > > + kfree(adev->acp.acp_genpd); > > kfree(adev->acp.acp_cell); > > return -ENOMEM; > > } > > > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), > GFP_KERNEL); > > if (i2s_pdata == NULL) { > > + kfree(adev->acp.acp_genpd); > > kfree(adev->acp.acp_res); > > kfree(adev->acp.acp_cell); > > return -ENOMEM; > > @@ -348,6 +352,10 @@ static int acp_hw_init(void *handle) > > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > > if (r) { > > dev_err(dev, "Failed to add dev to genpd\n"); > > + kfree(adev->acp.acp_genpd); > > + kfree(adev->acp.acp_res); > > + kfree(adev->acp.acp_cell); > > + kfree(i2s_pdata); > > return r; > > } > > } > > -- Navid. [-- Attachment #1.2: Type: text/html, Size: 3785 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] drm/amdgpu: fix multiple memory leaks 2019-09-18 17:31 ` Koenig, Christian [not found] ` <7bab24ff-ded7-9f76-ba25-efd07cdd30dd-5C7GfCeVMHo@public.gmane.org> @ 2019-09-18 19:05 ` Navid Emamdoost 2019-09-18 19:45 ` Sven Van Asbroeck ` (2 more replies) 1 sibling, 3 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-09-18 19:05 UTC (permalink / raw) To: Christian.Koenig Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Alex Deucher, Christian König, David (ChunMing) Zhou, David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel In acp_hw_init there are some allocations that needs to be released in case of failure: 1- adev->acp.acp_genpd should be released if any allocation attemp for adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. 2- all of those allocations should be released if pm_genpd_add_device fails. v2: moved the released into goto error handlings Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 30 +++++++++++++++++-------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c index eba42c752bca..c0db75b71859 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c @@ -184,7 +184,7 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) */ static int acp_hw_init(void *handle) { - int r, i; + int r, i, ret; uint64_t acp_base; u32 val = 0; u32 count = 0; @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), GFP_KERNEL); - if (adev->acp.acp_cell == NULL) - return -ENOMEM; + if (adev->acp.acp_cell == NULL) { + ret = -ENOMEM; + goto out1; + } adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); if (adev->acp.acp_res == NULL) { - kfree(adev->acp.acp_cell); - return -ENOMEM; + ret = -ENOMEM; + goto out2; } i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); if (i2s_pdata == NULL) { - kfree(adev->acp.acp_res); - kfree(adev->acp.acp_cell); - return -ENOMEM; + ret = -ENOMEM; + goto out3; } switch (adev->asic_type) { @@ -348,7 +349,8 @@ static int acp_hw_init(void *handle) r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); if (r) { dev_err(dev, "Failed to add dev to genpd\n"); - return r; + ret = r; + goto out4; } } @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); return 0; + +out4: + kfree(i2s_pdata); +out3: + kfree(adev->acp.acp_res); +out2: + kfree(adev->acp.acp_cell); +out1: + kfree(adev->acp.acp_genpd); + return ret; } /** -- 2.17.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks 2019-09-18 19:05 ` [PATCH v2] " Navid Emamdoost @ 2019-09-18 19:45 ` Sven Van Asbroeck 2019-09-19 8:03 ` Koenig, Christian 2019-09-27 16:37 ` Markus Elfring 2 siblings, 0 replies; 24+ messages in thread From: Sven Van Asbroeck @ 2019-09-18 19:45 UTC (permalink / raw) To: Navid Emamdoost Cc: Christian.Koenig, emamd001, smccaman, kjlu, Alex Deucher, David (ChunMing) Zhou, David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, Linux Kernel Mailing List On Wed, Sep 18, 2019 at 3:09 PM Navid Emamdoost <navid.emamdoost@gmail.com> wrote: > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out3; > } I don't see a corresponding kfree() for i2s_pdata in acp_hw_fini(). Could this be a memory leak? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks 2019-09-18 19:05 ` [PATCH v2] " Navid Emamdoost 2019-09-18 19:45 ` Sven Van Asbroeck @ 2019-09-19 8:03 ` Koenig, Christian 2019-09-19 14:28 ` Sven Van Asbroeck ` (2 more replies) 2019-09-27 16:37 ` Markus Elfring 2 siblings, 3 replies; 24+ messages in thread From: Koenig, Christian @ 2019-09-19 8:03 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel Am 18.09.19 um 21:05 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if pm_genpd_add_device > fails. > > v2: moved the released into goto error handlings > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 30 +++++++++++++++++-------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..c0db75b71859 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -184,7 +184,7 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) > */ > static int acp_hw_init(void *handle) > { > - int r, i; > + int r, i, ret; > uint64_t acp_base; > u32 val = 0; > u32 count = 0; > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; > + if (adev->acp.acp_cell == NULL) { > + ret = -ENOMEM; > + goto out1; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out2; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto out3; > } > > switch (adev->asic_type) { > @@ -348,7 +349,8 @@ static int acp_hw_init(void *handle) > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > - return r; > + ret = r; > + goto out4; > } > } > > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +out4: > + kfree(i2s_pdata); > +out3: > + kfree(adev->acp.acp_res); > +out2: > + kfree(adev->acp.acp_cell); > +out1: > + kfree(adev->acp.acp_genpd); kfree on a NULL pointer is harmless, so a single error label should be sufficient. Christian. > + return ret; > } > > /** ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks 2019-09-19 8:03 ` Koenig, Christian @ 2019-09-19 14:28 ` Sven Van Asbroeck 2019-09-19 16:48 ` Koenig, Christian 2019-09-30 21:26 ` [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init Navid Emamdoost 2019-09-30 21:31 ` [PATCH v2] drm/amdgpu: fix multiple memory leaks Navid Emamdoost 2 siblings, 1 reply; 24+ messages in thread From: Sven Van Asbroeck @ 2019-09-19 14:28 UTC (permalink / raw) To: Koenig, Christian Cc: Navid Emamdoost, emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel Hi Christian, On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > > +out4: > > + kfree(i2s_pdata); > > +out3: > > + kfree(adev->acp.acp_res); > > +out2: > > + kfree(adev->acp.acp_cell); > > +out1: > > + kfree(adev->acp.acp_genpd); > > kfree on a NULL pointer is harmless, so a single error label should be > sufficient. That is true, but I notice that the adev structure comes from outside this driver: static int acp_hw_init(void *handle) { ... struct amdgpu_device *adev = (struct amdgpu_device *)handle; ... } As far as I can tell, the init() does not explicitly set these to NULL: adev->acp.acp_res adev->acp.acp_cell adev->acp.acp_genpd I am assuming that core code sets these to NULL, before calling acp_hw_init(). But is that guaranteed or simply a happy accident? Ie. if they are NULL today, are they likely to remain NULL tomorrow? Because calling kfree() on a stale pointer would be, well not good. Especially not on an error path, which typically does not receive much testing, if any. My apologies if I have misinterpreted this, I am not familiar with this code base. Sven ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks @ 2019-09-19 16:48 ` Koenig, Christian 0 siblings, 0 replies; 24+ messages in thread From: Koenig, Christian @ 2019-09-19 16:48 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Navid Emamdoost, emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel Am 19.09.19 um 16:28 schrieb Sven Van Asbroeck: > Hi Christian, > > On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian > <Christian.Koenig@amd.com> wrote: >>> +out4: >>> + kfree(i2s_pdata); >>> +out3: >>> + kfree(adev->acp.acp_res); >>> +out2: >>> + kfree(adev->acp.acp_cell); >>> +out1: >>> + kfree(adev->acp.acp_genpd); >> kfree on a NULL pointer is harmless, so a single error label should be >> sufficient. > That is true, but I notice that the adev structure comes from outside this > driver: adev is a very integral part of the driver and always zero initialized: adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); Regards, Christian. > > static int acp_hw_init(void *handle) > { > ... > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > ... > } > > As far as I can tell, the init() does not explicitly set these to NULL: > adev->acp.acp_res > adev->acp.acp_cell > adev->acp.acp_genpd > > I am assuming that core code sets these to NULL, before calling > acp_hw_init(). But is that guaranteed or simply a happy accident? > Ie. if they are NULL today, are they likely to remain NULL tomorrow? > > Because calling kfree() on a stale pointer would be, well > not good. Especially not on an error path, which typically > does not receive much testing, if any. > > My apologies if I have misinterpreted this, I am not familiar with > this code base. > > Sven ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks @ 2019-09-19 16:48 ` Koenig, Christian 0 siblings, 0 replies; 24+ messages in thread From: Koenig, Christian @ 2019-09-19 16:48 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Zhou, David(ChunMing), David Airlie, Sam Ravnborg, kjlu-OJFnDUYgAso, linux-kernel-u79uwXL29TY76Z2rM5mHXA, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, emamd001-OJFnDUYgAso, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter, smccaman-OJFnDUYgAso, Deucher, Alexander, Rex Zhu, Navid Emamdoost Am 19.09.19 um 16:28 schrieb Sven Van Asbroeck: > Hi Christian, > > On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian > <Christian.Koenig@amd.com> wrote: >>> +out4: >>> + kfree(i2s_pdata); >>> +out3: >>> + kfree(adev->acp.acp_res); >>> +out2: >>> + kfree(adev->acp.acp_cell); >>> +out1: >>> + kfree(adev->acp.acp_genpd); >> kfree on a NULL pointer is harmless, so a single error label should be >> sufficient. > That is true, but I notice that the adev structure comes from outside this > driver: adev is a very integral part of the driver and always zero initialized: adev = kzalloc(sizeof(struct amdgpu_device), GFP_KERNEL); Regards, Christian. > > static int acp_hw_init(void *handle) > { > ... > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > ... > } > > As far as I can tell, the init() does not explicitly set these to NULL: > adev->acp.acp_res > adev->acp.acp_cell > adev->acp.acp_genpd > > I am assuming that core code sets these to NULL, before calling > acp_hw_init(). But is that guaranteed or simply a happy accident? > Ie. if they are NULL today, are they likely to remain NULL tomorrow? > > Because calling kfree() on a stale pointer would be, well > not good. Especially not on an error path, which typically > does not receive much testing, if any. > > My apologies if I have misinterpreted this, I am not familiar with > this code base. > > Sven _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init 2019-09-19 8:03 ` Koenig, Christian 2019-09-19 14:28 ` Sven Van Asbroeck @ 2019-09-30 21:26 ` Navid Emamdoost 2019-10-01 11:24 ` Markus Elfring 2019-10-01 12:19 ` Koenig, Christian 2019-09-30 21:31 ` [PATCH v2] drm/amdgpu: fix multiple memory leaks Navid Emamdoost 2 siblings, 2 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-09-30 21:26 UTC (permalink / raw) To: Christian.Koenig Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Alex Deucher, Christian König, David (ChunMing) Zhou, David Airlie, Daniel Vetter, Sam Ravnborg, Rex Zhu, amd-gfx, dri-devel, linux-kernel In acp_hw_init there are some allocations that needs to be released in case of failure: 1- adev->acp.acp_genpd should be released if any allocation attemp for adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. 2- all of those allocations should be released if mfd_add_hotplug_devices or pm_genpd_add_device fail. 3- Release is needed in case of time out values expire. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- Changes in v2: -- moved the releases under goto Changes in v3: -- fixed multiple goto issue -- added goto for 3 other failure cases: one when mfd_add_hotplug_devices fails, and two when time out values expires. --- drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 41 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c index eba42c752bca..7809745ec0f1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) */ static int acp_hw_init(void *handle) { - int r, i; + int r, i, ret; uint64_t acp_base; u32 val = 0; u32 count = 0; struct device *dev; - struct i2s_platform_data *i2s_pdata; + struct i2s_platform_data *i2s_pdata = NULL; struct amdgpu_device *adev = (struct amdgpu_device *)handle; @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), GFP_KERNEL); - if (adev->acp.acp_cell == NULL) - return -ENOMEM; + if (adev->acp.acp_cell == NULL) { + ret = -ENOMEM; + goto failure; + } adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); if (adev->acp.acp_res == NULL) { - kfree(adev->acp.acp_cell); - return -ENOMEM; + ret = -ENOMEM; + goto failure; } i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); if (i2s_pdata == NULL) { - kfree(adev->acp.acp_res); - kfree(adev->acp.acp_cell); - return -ENOMEM; + ret = -ENOMEM; + goto failure; } switch (adev->asic_type) { @@ -340,15 +341,18 @@ static int acp_hw_init(void *handle) r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, ACP_DEVS); - if (r) - return r; + if (r) { + ret = r; + goto failure; + } for (i = 0; i < ACP_DEVS ; i++) { dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); if (r) { dev_err(dev, "Failed to add dev to genpd\n"); - return r; + ret = r; + goto failure; } } @@ -367,7 +371,8 @@ static int acp_hw_init(void *handle) break; if (--count == 0) { dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto failure; } udelay(100); } @@ -384,7 +389,8 @@ static int acp_hw_init(void *handle) break; if (--count == 0) { dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto failure; } udelay(100); } @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle) val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); return 0; + +failure: + kfree(i2s_pdata); + kfree(adev->acp.acp_res); + kfree(adev->acp.acp_cell); + kfree(adev->acp.acp_genpd); + return ret; } /** -- 2.17.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init 2019-09-30 21:26 ` [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init Navid Emamdoost @ 2019-10-01 11:24 ` Markus Elfring 2019-10-01 12:19 ` Koenig, Christian 1 sibling, 0 replies; 24+ messages in thread From: Markus Elfring @ 2019-10-01 11:24 UTC (permalink / raw) To: Navid Emamdoost, Christian König, amd-gfx, dri-devel Cc: Navid Emamdoost, Kangjie Lu, Stephen McCamant, Alex Deucher, Chunming Zhou, Daniel Vetter, David Airlie, Sam Ravnborg, Sven Van Asbroeck, LKML, kernel-janitors > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) … > + struct i2s_platform_data *i2s_pdata = NULL; … I propose to reconsider this update suggestion. > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; … I suggest to keep this source code place unchanged (at the moment). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c#n456 > @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** I would prefer separate jump targets for efficient exception handling. Please choose more appropriate labels for this function implementation. > --- I suggest to replace this second delimiter by a blank line. Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init @ 2019-10-01 11:24 ` Markus Elfring 0 siblings, 0 replies; 24+ messages in thread From: Markus Elfring @ 2019-10-01 11:24 UTC (permalink / raw) To: Navid Emamdoost, Christian König, amd-gfx, dri-devel Cc: Navid Emamdoost, Kangjie Lu, Stephen McCamant, Alex Deucher, Chunming Zhou, Daniel Vetter, David Airlie, Sam Ravnborg, Sven Van Asbroeck, LKML, kernel-janitors > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) … > + struct i2s_platform_data *i2s_pdata = NULL; … I propose to reconsider this update suggestion. > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; … I suggest to keep this source code place unchanged (at the moment). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c#n456 > @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** I would prefer separate jump targets for efficient exception handling. Please choose more appropriate labels for this function implementation. > --- I suggest to replace this second delimiter by a blank line. Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init @ 2019-10-01 12:19 ` Koenig, Christian 0 siblings, 0 replies; 24+ messages in thread From: Koenig, Christian @ 2019-10-01 12:19 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Sam Ravnborg, Rex Zhu, amd-gfx, dri-devel, linux-kernel Am 30.09.19 um 23:26 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if > mfd_add_hotplug_devices or pm_genpd_add_device fail. > 3- Release is needed in case of time out values expire. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > Changes in v2: > -- moved the releases under goto > > Changes in v3: > -- fixed multiple goto issue > -- added goto for 3 other failure cases: one when > mfd_add_hotplug_devices fails, and two when time out values expires. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 41 ++++++++++++++++--------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..7809745ec0f1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) > */ > static int acp_hw_init(void *handle) > { > - int r, i; > + int r, i, ret; Please don't add another "ret" variable, instead always use "r" here. Apart from that looks good to me, Christian. > uint64_t acp_base; > u32 val = 0; > u32 count = 0; > struct device *dev; > - struct i2s_platform_data *i2s_pdata; > + struct i2s_platform_data *i2s_pdata = NULL; > > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; > + if (adev->acp.acp_cell == NULL) { > + ret = -ENOMEM; > + goto failure; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto failure; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto failure; > } > > switch (adev->asic_type) { > @@ -340,15 +341,18 @@ static int acp_hw_init(void *handle) > > r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, > ACP_DEVS); > - if (r) > - return r; > + if (r) { > + ret = r; > + goto failure; > + } > > for (i = 0; i < ACP_DEVS ; i++) { > dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > - return r; > + ret = r; > + goto failure; > } > } > > @@ -367,7 +371,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -384,7 +389,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init @ 2019-10-01 12:19 ` Koenig, Christian 0 siblings, 0 replies; 24+ messages in thread From: Koenig, Christian @ 2019-10-01 12:19 UTC (permalink / raw) To: Navid Emamdoost Cc: Zhou, David(ChunMing), David Airlie, Rex Zhu, kjlu-OJFnDUYgAso, linux-kernel-u79uwXL29TY76Z2rM5mHXA, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, emamd001-OJFnDUYgAso, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter, smccaman-OJFnDUYgAso, Deucher, Alexander, Sam Ravnborg Am 30.09.19 um 23:26 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if > mfd_add_hotplug_devices or pm_genpd_add_device fail. > 3- Release is needed in case of time out values expire. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > --- > Changes in v2: > -- moved the releases under goto > > Changes in v3: > -- fixed multiple goto issue > -- added goto for 3 other failure cases: one when > mfd_add_hotplug_devices fails, and two when time out values expires. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 41 ++++++++++++++++--------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..7809745ec0f1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) > */ > static int acp_hw_init(void *handle) > { > - int r, i; > + int r, i, ret; Please don't add another "ret" variable, instead always use "r" here. Apart from that looks good to me, Christian. > uint64_t acp_base; > u32 val = 0; > u32 count = 0; > struct device *dev; > - struct i2s_platform_data *i2s_pdata; > + struct i2s_platform_data *i2s_pdata = NULL; > > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; > + if (adev->acp.acp_cell == NULL) { > + ret = -ENOMEM; > + goto failure; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto failure; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + ret = -ENOMEM; > + goto failure; > } > > switch (adev->asic_type) { > @@ -340,15 +341,18 @@ static int acp_hw_init(void *handle) > > r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, > ACP_DEVS); > - if (r) > - return r; > + if (r) { > + ret = r; > + goto failure; > + } > > for (i = 0; i < ACP_DEVS ; i++) { > dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > - return r; > + ret = r; > + goto failure; > } > } > > @@ -367,7 +371,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -384,7 +389,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + ret = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init 2019-10-01 12:19 ` Koenig, Christian (?) @ 2019-10-02 3:46 ` Navid Emamdoost 2019-10-02 5:47 ` Markus Elfring 2019-10-02 6:58 ` Koenig, Christian -1 siblings, 2 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-10-02 3:46 UTC (permalink / raw) To: Christian.Koenig Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Alex Deucher, Christian König, David (ChunMing) Zhou, David Airlie, Daniel Vetter, Sam Ravnborg, Rex Zhu, amd-gfx, dri-devel, linux-kernel In acp_hw_init there are some allocations that needs to be released in case of failure: 1- adev->acp.acp_genpd should be released if any allocation attemp for adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. 2- all of those allocations should be released if mfd_add_hotplug_devices or pm_genpd_add_device fail. 3- Release is needed in case of time out values expire. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 34 ++++++++++++++++--------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c index eba42c752bca..82155ac3288a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c @@ -189,7 +189,7 @@ static int acp_hw_init(void *handle) u32 val = 0; u32 count = 0; struct device *dev; - struct i2s_platform_data *i2s_pdata; + struct i2s_platform_data *i2s_pdata = NULL; struct amdgpu_device *adev = (struct amdgpu_device *)handle; @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), GFP_KERNEL); - if (adev->acp.acp_cell == NULL) - return -ENOMEM; + if (adev->acp.acp_cell == NULL) { + r = -ENOMEM; + goto failure; + } adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); if (adev->acp.acp_res == NULL) { - kfree(adev->acp.acp_cell); - return -ENOMEM; + r = -ENOMEM; + goto failure; } i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); if (i2s_pdata == NULL) { - kfree(adev->acp.acp_res); - kfree(adev->acp.acp_cell); - return -ENOMEM; + r = -ENOMEM; + goto failure; } switch (adev->asic_type) { @@ -341,14 +342,14 @@ static int acp_hw_init(void *handle) r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, ACP_DEVS); if (r) - return r; + goto failure; for (i = 0; i < ACP_DEVS ; i++) { dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); if (r) { dev_err(dev, "Failed to add dev to genpd\n"); - return r; + goto failure; } } @@ -367,7 +368,8 @@ static int acp_hw_init(void *handle) break; if (--count == 0) { dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); - return -ETIMEDOUT; + r = -ETIMEDOUT; + goto failure; } udelay(100); } @@ -384,7 +386,8 @@ static int acp_hw_init(void *handle) break; if (--count == 0) { dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); - return -ETIMEDOUT; + r = -ETIMEDOUT; + goto failure; } udelay(100); } @@ -393,6 +396,13 @@ static int acp_hw_init(void *handle) val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); return 0; + +failure: + kfree(i2s_pdata); + kfree(adev->acp.acp_res); + kfree(adev->acp.acp_cell); + kfree(adev->acp.acp_genpd); + return r; } /** -- 2.17.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init 2019-10-02 3:46 ` [PATCH v4] " Navid Emamdoost @ 2019-10-02 5:47 ` Markus Elfring 2019-10-02 6:58 ` Koenig, Christian 1 sibling, 0 replies; 24+ messages in thread From: Markus Elfring @ 2019-10-02 5:47 UTC (permalink / raw) To: Navid Emamdoost, Christian König, Chunming Zhou, Alex Deucher, amd-gfx, dri-devel Cc: Navid Emamdoost, Kangjie Lu, Stephen McCamant, Daniel Vetter, David Airlie, Sam Ravnborg, linux-kernel, kernel-janitors > --- Why did you omit the patch change log at this place? > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 34 ++++++++++++++++--------- > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -189,7 +189,7 @@ static int acp_hw_init(void *handle) … > + struct i2s_platform_data *i2s_pdata = NULL; … I propose to reconsider this update suggestion once more. > @@ -393,6 +396,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return r; > } > > /** Are you going to follow a known programming guideline? https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12-C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28copy_process%28%29fromLinuxkernel%29 Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init @ 2019-10-02 5:47 ` Markus Elfring 0 siblings, 0 replies; 24+ messages in thread From: Markus Elfring @ 2019-10-02 5:47 UTC (permalink / raw) To: Navid Emamdoost, Christian König, Chunming Zhou, Alex Deucher, amd-gfx, dri-devel Cc: Navid Emamdoost, Kangjie Lu, Stephen McCamant, Daniel Vetter, David Airlie, Sam Ravnborg, linux-kernel, kernel-janitors > --- Why did you omit the patch change log at this place? > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 34 ++++++++++++++++--------- > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -189,7 +189,7 @@ static int acp_hw_init(void *handle) … > + struct i2s_platform_data *i2s_pdata = NULL; … I propose to reconsider this update suggestion once more. > @@ -393,6 +396,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return r; > } > > /** Are you going to follow a known programming guideline? https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12-C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28copy_process%28%29fromLinuxkernel%29 Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init @ 2019-10-02 6:58 ` Koenig, Christian 0 siblings, 0 replies; 24+ messages in thread From: Koenig, Christian @ 2019-10-02 6:58 UTC (permalink / raw) To: Navid Emamdoost Cc: emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Sam Ravnborg, Rex Zhu, amd-gfx, dri-devel, linux-kernel Am 02.10.19 um 05:46 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if > mfd_add_hotplug_devices or pm_genpd_add_device fail. > 3- Release is needed in case of time out values expire. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 34 ++++++++++++++++--------- > 1 file changed, 22 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..82155ac3288a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -189,7 +189,7 @@ static int acp_hw_init(void *handle) > u32 val = 0; > u32 count = 0; > struct device *dev; > - struct i2s_platform_data *i2s_pdata; > + struct i2s_platform_data *i2s_pdata = NULL; > > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; > + if (adev->acp.acp_cell == NULL) { > + r = -ENOMEM; > + goto failure; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + r = -ENOMEM; > + goto failure; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + r = -ENOMEM; > + goto failure; > } > > switch (adev->asic_type) { > @@ -341,14 +342,14 @@ static int acp_hw_init(void *handle) > r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, > ACP_DEVS); > if (r) > - return r; > + goto failure; > > for (i = 0; i < ACP_DEVS ; i++) { > dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > - return r; > + goto failure; > } > } > > @@ -367,7 +368,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + r = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -384,7 +386,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + r = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -393,6 +396,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return r; > } > > /** ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init @ 2019-10-02 6:58 ` Koenig, Christian 0 siblings, 0 replies; 24+ messages in thread From: Koenig, Christian @ 2019-10-02 6:58 UTC (permalink / raw) To: Navid Emamdoost Cc: Zhou, David(ChunMing), David Airlie, Rex Zhu, kjlu-OJFnDUYgAso, linux-kernel-u79uwXL29TY76Z2rM5mHXA, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, emamd001-OJFnDUYgAso, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Daniel Vetter, smccaman-OJFnDUYgAso, Deucher, Alexander, Sam Ravnborg Am 02.10.19 um 05:46 schrieb Navid Emamdoost: > In acp_hw_init there are some allocations that needs to be released in > case of failure: > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > 2- all of those allocations should be released if > mfd_add_hotplug_devices or pm_genpd_add_device fail. > 3- Release is needed in case of time out values expire. > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 34 ++++++++++++++++--------- > 1 file changed, 22 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > index eba42c752bca..82155ac3288a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > @@ -189,7 +189,7 @@ static int acp_hw_init(void *handle) > u32 val = 0; > u32 count = 0; > struct device *dev; > - struct i2s_platform_data *i2s_pdata; > + struct i2s_platform_data *i2s_pdata = NULL; > > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > GFP_KERNEL); > > - if (adev->acp.acp_cell == NULL) > - return -ENOMEM; > + if (adev->acp.acp_cell == NULL) { > + r = -ENOMEM; > + goto failure; > + } > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > if (adev->acp.acp_res == NULL) { > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + r = -ENOMEM; > + goto failure; > } > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > if (i2s_pdata == NULL) { > - kfree(adev->acp.acp_res); > - kfree(adev->acp.acp_cell); > - return -ENOMEM; > + r = -ENOMEM; > + goto failure; > } > > switch (adev->asic_type) { > @@ -341,14 +342,14 @@ static int acp_hw_init(void *handle) > r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, > ACP_DEVS); > if (r) > - return r; > + goto failure; > > for (i = 0; i < ACP_DEVS ; i++) { > dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > if (r) { > dev_err(dev, "Failed to add dev to genpd\n"); > - return r; > + goto failure; > } > } > > @@ -367,7 +368,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + r = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -384,7 +386,8 @@ static int acp_hw_init(void *handle) > break; > if (--count == 0) { > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > - return -ETIMEDOUT; > + r = -ETIMEDOUT; > + goto failure; > } > udelay(100); > } > @@ -393,6 +396,13 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +failure: > + kfree(i2s_pdata); > + kfree(adev->acp.acp_res); > + kfree(adev->acp.acp_cell); > + kfree(adev->acp.acp_genpd); > + return r; > } > > /** _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init 2019-10-01 12:19 ` Koenig, Christian (?) (?) @ 2019-10-02 3:47 ` Navid Emamdoost -1 siblings, 0 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-10-02 3:47 UTC (permalink / raw) To: Koenig, Christian Cc: emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Sam Ravnborg, Rex Zhu, amd-gfx, dri-devel, linux-kernel On Tue, Oct 1, 2019 at 7:19 AM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > Am 30.09.19 um 23:26 schrieb Navid Emamdoost: > > In acp_hw_init there are some allocations that needs to be released in > > case of failure: > > > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > > 2- all of those allocations should be released if > > mfd_add_hotplug_devices or pm_genpd_add_device fail. > > 3- Release is needed in case of time out values expire. > > > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > > --- > > Changes in v2: > > -- moved the releases under goto > > > > Changes in v3: > > -- fixed multiple goto issue > > -- added goto for 3 other failure cases: one when > > mfd_add_hotplug_devices fails, and two when time out values expires. > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 41 ++++++++++++++++--------- > > 1 file changed, 27 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > index eba42c752bca..7809745ec0f1 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) > > */ > > static int acp_hw_init(void *handle) > > { > > - int r, i; > > + int r, i, ret; > > Please don't add another "ret" variable, instead always use "r" here. > Done! > Apart from that looks good to me, > Christian. > > > uint64_t acp_base; > > u32 val = 0; > > u32 count = 0; > > struct device *dev; > > - struct i2s_platform_data *i2s_pdata; > > + struct i2s_platform_data *i2s_pdata = NULL; > > > > struct amdgpu_device *adev = (struct amdgpu_device *)handle; > > > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > > GFP_KERNEL); > > > > - if (adev->acp.acp_cell == NULL) > > - return -ENOMEM; > > + if (adev->acp.acp_cell == NULL) { > > + ret = -ENOMEM; > > + goto failure; > > + } > > > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > > if (adev->acp.acp_res == NULL) { > > - kfree(adev->acp.acp_cell); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto failure; > > } > > > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > > if (i2s_pdata == NULL) { > > - kfree(adev->acp.acp_res); > > - kfree(adev->acp.acp_cell); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto failure; > > } > > > > switch (adev->asic_type) { > > @@ -340,15 +341,18 @@ static int acp_hw_init(void *handle) > > > > r = mfd_add_hotplug_devices(adev->acp.parent, adev->acp.acp_cell, > > ACP_DEVS); > > - if (r) > > - return r; > > + if (r) { > > + ret = r; > > + goto failure; > > + } > > > > for (i = 0; i < ACP_DEVS ; i++) { > > dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i); > > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > > if (r) { > > dev_err(dev, "Failed to add dev to genpd\n"); > > - return r; > > + ret = r; > > + goto failure; > > } > > } > > > > @@ -367,7 +371,8 @@ static int acp_hw_init(void *handle) > > break; > > if (--count == 0) { > > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > > - return -ETIMEDOUT; > > + ret = -ETIMEDOUT; > > + goto failure; > > } > > udelay(100); > > } > > @@ -384,7 +389,8 @@ static int acp_hw_init(void *handle) > > break; > > if (--count == 0) { > > dev_err(&adev->pdev->dev, "Failed to reset ACP\n"); > > - return -ETIMEDOUT; > > + ret = -ETIMEDOUT; > > + goto failure; > > } > > udelay(100); > > } > > @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle) > > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > > return 0; > > + > > +failure: > > + kfree(i2s_pdata); > > + kfree(adev->acp.acp_res); > > + kfree(adev->acp.acp_cell); > > + kfree(adev->acp.acp_genpd); > > + return ret; > > } > > > > /** > -- Navid. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks 2019-09-19 8:03 ` Koenig, Christian 2019-09-19 14:28 ` Sven Van Asbroeck 2019-09-30 21:26 ` [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init Navid Emamdoost @ 2019-09-30 21:31 ` Navid Emamdoost 2 siblings, 0 replies; 24+ messages in thread From: Navid Emamdoost @ 2019-09-30 21:31 UTC (permalink / raw) To: Koenig, Christian Cc: emamd001, smccaman, kjlu, Deucher, Alexander, Zhou, David(ChunMing), David Airlie, Daniel Vetter, Rex Zhu, Sam Ravnborg, amd-gfx, dri-devel, linux-kernel On Thu, Sep 19, 2019 at 3:03 AM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > Am 18.09.19 um 21:05 schrieb Navid Emamdoost: > > In acp_hw_init there are some allocations that needs to be released in > > case of failure: > > > > 1- adev->acp.acp_genpd should be released if any allocation attemp for > > adev->acp.acp_cell, adev->acp.acp_res or i2s_pdata fails. > > 2- all of those allocations should be released if pm_genpd_add_device > > fails. > > > > v2: moved the released into goto error handlings > > > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 30 +++++++++++++++++-------- > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > index eba42c752bca..c0db75b71859 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c > > @@ -184,7 +184,7 @@ static struct device *get_mfd_cell_dev(const char *device_name, int r) > > */ > > static int acp_hw_init(void *handle) > > { > > - int r, i; > > + int r, i, ret; > > uint64_t acp_base; > > u32 val = 0; > > u32 count = 0; > > @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle) > > adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell), > > GFP_KERNEL); > > > > - if (adev->acp.acp_cell == NULL) > > - return -ENOMEM; > > + if (adev->acp.acp_cell == NULL) { > > + ret = -ENOMEM; > > + goto out1; > > + } > > > > adev->acp.acp_res = kcalloc(5, sizeof(struct resource), GFP_KERNEL); > > if (adev->acp.acp_res == NULL) { > > - kfree(adev->acp.acp_cell); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto out2; > > } > > > > i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL); > > if (i2s_pdata == NULL) { > > - kfree(adev->acp.acp_res); > > - kfree(adev->acp.acp_cell); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto out3; > > } > > > > switch (adev->asic_type) { > > @@ -348,7 +349,8 @@ static int acp_hw_init(void *handle) > > r = pm_genpd_add_device(&adev->acp.acp_genpd->gpd, dev); > > if (r) { > > dev_err(dev, "Failed to add dev to genpd\n"); > > - return r; > > + ret = r; > > + goto out4; > > } > > } > > > > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > > return 0; > > + > > +out4: > > + kfree(i2s_pdata); > > +out3: > > + kfree(adev->acp.acp_res); > > +out2: > > + kfree(adev->acp.acp_cell); > > +out1: > > + kfree(adev->acp.acp_genpd); > > kfree on a NULL pointer is harmless, so a single error label should be > sufficient. > I fixed this by just using failure label. > Christian. > > > + return ret; > > } > > > > /** > In addition to previous cases, I covered 3 more error handling cases that seemed need to goto failure. One where mfd_add_hotplug_devices fails and the other two cases where time out values expire. -- Navid. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks 2019-09-18 19:05 ` [PATCH v2] " Navid Emamdoost @ 2019-09-27 16:37 ` Markus Elfring 2019-09-19 8:03 ` Koenig, Christian 2019-09-27 16:37 ` Markus Elfring 2 siblings, 0 replies; 24+ messages in thread From: Markus Elfring @ 2019-09-27 16:37 UTC (permalink / raw) To: Navid Emamdoost, Christian König, amd-gfx, dri-devel Cc: Chunming Zhou, Rex Zhu, Alex Deucher, Sam Ravnborg, David Airlie, Daniel Vetter, Navid Emamdoost, Kangjie Lu, Stephen A McCamant, linux-kernel, kernel-janitors > v2: moved the released into goto error handlings A better version comment should be moved below the triple dashes. Will the tag “Fixes” be added? > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +out4: > + kfree(i2s_pdata); > +out3: > + kfree(adev->acp.acp_res); > +out2: > + kfree(adev->acp.acp_cell); > +out1: > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** I suggest to reconsider the label selection according to the Linux coding style. Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks @ 2019-09-27 16:37 ` Markus Elfring 0 siblings, 0 replies; 24+ messages in thread From: Markus Elfring @ 2019-09-27 16:37 UTC (permalink / raw) To: Navid Emamdoost, Christian König, amd-gfx, dri-devel Cc: Chunming Zhou, Rex Zhu, Alex Deucher, Sam Ravnborg, David Airlie, Daniel Vetter, Navid Emamdoost, Kangjie Lu, Stephen A McCamant, linux-kernel, kernel-janitors > v2: moved the released into goto error handlings A better version comment should be moved below the triple dashes. Will the tag “Fixes” be added? > @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle) > val &= ~ACP_SOFT_RESET__SoftResetAud_MASK; > cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val); > return 0; > + > +out4: > + kfree(i2s_pdata); > +out3: > + kfree(adev->acp.acp_res); > +out2: > + kfree(adev->acp.acp_cell); > +out1: > + kfree(adev->acp.acp_genpd); > + return ret; > } > > /** I suggest to reconsider the label selection according to the Linux coding style. Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-10-02 6:58 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-18 16:09 [PATCH] drm/amdgpu: fix multiple memory leaks Navid Emamdoost 2019-09-18 16:09 ` Navid Emamdoost 2019-09-18 17:31 ` Koenig, Christian [not found] ` <7bab24ff-ded7-9f76-ba25-efd07cdd30dd-5C7GfCeVMHo@public.gmane.org> 2019-09-18 19:04 ` Navid Emamdoost 2019-09-18 19:05 ` [PATCH v2] " Navid Emamdoost 2019-09-18 19:45 ` Sven Van Asbroeck 2019-09-19 8:03 ` Koenig, Christian 2019-09-19 14:28 ` Sven Van Asbroeck 2019-09-19 16:48 ` Koenig, Christian 2019-09-19 16:48 ` Koenig, Christian 2019-09-30 21:26 ` [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init Navid Emamdoost 2019-10-01 11:24 ` Markus Elfring 2019-10-01 11:24 ` Markus Elfring 2019-10-01 12:19 ` Koenig, Christian 2019-10-01 12:19 ` Koenig, Christian 2019-10-02 3:46 ` [PATCH v4] " Navid Emamdoost 2019-10-02 5:47 ` Markus Elfring 2019-10-02 5:47 ` Markus Elfring 2019-10-02 6:58 ` Koenig, Christian 2019-10-02 6:58 ` Koenig, Christian 2019-10-02 3:47 ` [PATCH v3] " Navid Emamdoost 2019-09-30 21:31 ` [PATCH v2] drm/amdgpu: fix multiple memory leaks Navid Emamdoost 2019-09-27 16:37 ` Markus Elfring 2019-09-27 16:37 ` Markus Elfring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.