[PATCH] drm/amdgpu/display: fix ref count leak when pm_runtime_get_sync fails
The call to pm_runtime_get_sync increments the counter even in case of failure, leading to incorrect ref count. In case of failure, decrement the ref count before returning. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index f355d9a752d2..a1aec205435d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -716,8 +716,10 @@ amdgpu_connector_lvds_detect(struct drm_connector *connector, bool force) if (!drm_kms_helper_is_poll_worker()) { r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) + if (r < 0) { + pm_runtime_put_autosuspend(connector->dev->dev); return connector_status_disconnected; + } } if (encoder) { @@ -854,8 +856,10 @@ amdgpu_connector_vga_detect(struct drm_connector *connector, bool force) if (!drm_kms_helper_is_poll_worker()) { r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) + if (r < 0) { + pm_runtime_put_autosuspend(connector->dev->dev); return connector_status_disconnected; + } } encoder = amdgpu_connector_best_single_encoder(connector); @@ -977,8 +981,10 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force) if (!drm_kms_helper_is_poll_worker()) { r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) + if (r < 0) { + pm_runtime_put_autosuspend(connector->dev->dev); return connector_status_disconnected; + } } if (!force && amdgpu_connector_check_hpd_status_unchanged(connector)) { @@ -1328,8 +1334,10 @@ amdgpu_connector_dp_detect(struct drm_connector *connector, bool force) if (!drm_kms_helper_is_poll_worker()) { r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) + if (r < 0) { + pm_runtime_put_autosuspend(connector->dev->dev); return connector_status_disconnected; + } } if (!force && amdgpu_connector_check_hpd_status_unchanged(connector)) { -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: fix ref count leak in amdgpu_drm_ioctl
in amdgpu_drm_ioctl the call to pm_runtime_get_sync increments the counter even in case of failure, leading to incorrect ref count. In case of failure, decrement the ref count before returning. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 126e74758a34..d73924e35a57 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1373,11 +1373,12 @@ long amdgpu_drm_ioctl(struct file *filp, dev = file_priv->minor->dev; ret = pm_runtime_get_sync(dev->dev); if (ret < 0) - return ret; + goto out; ret = drm_ioctl(filp, cmd, arg); pm_runtime_mark_last_busy(dev->dev); +out: pm_runtime_put_autosuspend(dev->dev); return ret; } -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: fix ref count leak in amdgpu_display_crtc_set_config
in amdgpu_display_crtc_set_config, the call to pm_runtime_get_sync increments the counter even in case of failure, leading to incorrect ref count. In case of failure, decrement the ref count before returning. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index f7143d927b6d..5e51f0acf744 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -282,7 +282,7 @@ int amdgpu_display_crtc_set_config(struct drm_mode_set *set, ret = pm_runtime_get_sync(dev->dev); if (ret < 0) - return ret; + goto out; ret = drm_crtc_helper_set_config(set, ctx); @@ -297,7 +297,7 @@ int amdgpu_display_crtc_set_config(struct drm_mode_set *set, take the current one */ if (active && !adev->have_disp_power_ref) { adev->have_disp_power_ref = true; - return ret; + goto out; } /* if we have no active crtcs, then drop the power ref we got before */ @@ -306,6 +306,7 @@ int amdgpu_display_crtc_set_config(struct drm_mode_set *set, adev->have_disp_power_ref = false; } +out: /* drop the power reference we got coming in here */ pm_runtime_put_autosuspend(dev->dev); return ret; -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: fix ref count leak in amdgpu_driver_open_kms
in amdgpu_driver_open_kms the call to pm_runtime_get_sync increments the counter even in case of failure, leading to incorrect ref count. In case of failure, decrement the ref count before returning. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index d7e17e34fee1..bd40aa307462 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -991,7 +991,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) r = pm_runtime_get_sync(dev->dev); if (r < 0) - return r; + goto pm_put; fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL); if (unlikely(!fpriv)) { @@ -1042,6 +1042,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv) out_suspend: pm_runtime_mark_last_busy(dev->dev); +pm_put: pm_runtime_put_autosuspend(dev->dev); return r; -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: Fix memory leak in amdgpu_fence_emit
In the impelementation of amdgpu_fence_emit() if dma_fence_wait() fails and returns an errno, before returning the allocated memory for fence should be released. Fixes: 3d2aca8c8620 ("drm/amdgpu: fix old fence check in amdgpu_fence_emit") Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 23085b352cf2..2f59c9270a7e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -166,8 +166,10 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, if (old) { r = dma_fence_wait(old, false); dma_fence_put(old); - if (r) + if (r) { + dma_fence_put(fence); return r; + } } } -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init
On Tue, Oct 1, 2019 at 7:19 AM Koenig, Christian 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 > > --- > > 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(>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(>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(>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.
[PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init
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 --- 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(>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(>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(>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
Re: [PATCH] drm/amd/display: memory leak
Would you please review this patch? Thanks, Navid. On Mon, Sep 16, 2019 at 10:21 PM Navid Emamdoost wrote: > > In dcn*_clock_source_create when dcn20_clk_src_construct fails allocated > clk_src needs release. > > Signed-off-by: Navid Emamdoost > --- > drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c | 3 ++- > drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c | 1 + > drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c | 1 + > drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c | 1 + > drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c | 1 + > drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c | 1 + > drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 1 + > 7 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c > b/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c > index 6248c8455314..21de230b303a 100644 > --- a/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c > @@ -667,7 +667,8 @@ struct clock_source *dce100_clock_source_create( > clk_src->base.dp_clk_src = dp_clk_src; > return _src->base; > } > - > + > + kfree(clk_src); > BREAK_TO_DEBUGGER(); > return NULL; > } > diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c > b/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c > index 764329264c3b..0cb83b0e0e1e 100644 > --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c > @@ -714,6 +714,7 @@ struct clock_source *dce110_clock_source_create( > return _src->base; > } > > + kfree(clk_src); > BREAK_TO_DEBUGGER(); > return NULL; > } > diff --git a/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c > b/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c > index c6136e0ed1a4..147d77173e2b 100644 > --- a/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c > @@ -687,6 +687,7 @@ struct clock_source *dce112_clock_source_create( > return _src->base; > } > > + kfree(clk_src); > BREAK_TO_DEBUGGER(); > return NULL; > } > diff --git a/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c > b/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c > index 4a6ba3173a5a..0b5eeff17d00 100644 > --- a/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c > @@ -500,6 +500,7 @@ static struct clock_source *dce120_clock_source_create( > return _src->base; > } > > + kfree(clk_src); > BREAK_TO_DEBUGGER(); > return NULL; > } > diff --git a/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c > b/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c > index 860a524ebcfa..952440893fbb 100644 > --- a/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c > @@ -701,6 +701,7 @@ struct clock_source *dce80_clock_source_create( > return _src->base; > } > > + kfree(clk_src); > BREAK_TO_DEBUGGER(); > return NULL; > } > diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c > b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c > index a12530a3ab9c..3f25e8da5396 100644 > --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c > @@ -786,6 +786,7 @@ struct clock_source *dcn10_clock_source_create( > return _src->base; > } > > + kfree(clk_src); > BREAK_TO_DEBUGGER(); > return NULL; > } > diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > index b949e202d6cb..418fdcf1f492 100644 > --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > @@ -955,6 +955,7 @@ struct clock_source *dcn20_clock_source_create( > return _src->base; > } > > + kfree(clk_src) > BREAK_TO_DEBUGGER(); > return NULL; > } > -- > 2.17.1 > -- Navid.
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
On Thu, Sep 19, 2019 at 3:03 AM Koenig, Christian 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 > > --- > > 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(>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.
[PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init
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 --- 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(>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(>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(>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
[PATCH] drm/amd/display: prevent memory leak
In dcn*_create_resource_pool the allocated memory should be released if construct pool fails. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c | 1 + 5 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c b/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c index afc61055eca1..1787b9bf800a 100644 --- a/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c @@ -1091,6 +1091,7 @@ struct resource_pool *dce100_create_resource_pool( if (construct(num_virtual_links, dc, pool)) return >base; + kfree(pool); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c index c66fe170e1e8..318e9c2e2ca8 100644 --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c @@ -1462,6 +1462,7 @@ struct resource_pool *dce110_create_resource_pool( if (construct(num_virtual_links, dc, pool, asic_id)) return >base; + kfree(pool); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c b/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c index 3ac4c7e73050..3199d493d13b 100644 --- a/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c @@ -1338,6 +1338,7 @@ struct resource_pool *dce112_create_resource_pool( if (construct(num_virtual_links, dc, pool)) return >base; + kfree(pool); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c b/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c index 7d08154e9662..bb497f43f6eb 100644 --- a/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c @@ -1203,6 +1203,7 @@ struct resource_pool *dce120_create_resource_pool( if (construct(num_virtual_links, dc, pool)) return >base; + kfree(pool); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c index 5a89e462e7cc..59305e411a66 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c @@ -1570,6 +1570,7 @@ struct resource_pool *dcn10_create_resource_pool( if (construct(init_data->num_virtual_links, dc, pool)) return >base; + kfree(pool); BREAK_TO_DEBUGGER(); return NULL; } -- 2.17.1
[PATCH] drm/amdgpu: release allocated memory
In amdgpu_vmid_grab_idle, fences is being leaked in one execution path. The missing kfree was added. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index 57b3d8a9bef3..9063cd36fa94 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c @@ -244,6 +244,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm, r = amdgpu_sync_fence(adev, sync, >base, false); dma_fence_put(ring->vmid_wait); ring->vmid_wait = >base; + kfree(fences); return r; } kfree(fences); -- 2.17.1
Re: [PATCH] drm/amdgpu: fix multiple memory leaks
Thanks Christian for the feedback, I'll send a v2. On Wed, Sep 18, 2019 at 12:31 PM Koenig, Christian 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 > > --- > > 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(>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. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2] drm/amdgpu: fix multiple memory leaks
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 --- 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(>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
[PATCH] drm/amdgpu: fix multiple memory leaks
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 --- 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(>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
[PATCH] drm/amd/display: memory leak
In dcn*_clock_source_create when dcn20_clk_src_construct fails allocated clk_src needs release. Signed-off-by: Navid Emamdoost --- drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c | 3 ++- drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 1 + 7 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c b/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c index 6248c8455314..21de230b303a 100644 --- a/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce100/dce100_resource.c @@ -667,7 +667,8 @@ struct clock_source *dce100_clock_source_create( clk_src->base.dp_clk_src = dp_clk_src; return _src->base; } - + + kfree(clk_src); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c index 764329264c3b..0cb83b0e0e1e 100644 --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_resource.c @@ -714,6 +714,7 @@ struct clock_source *dce110_clock_source_create( return _src->base; } + kfree(clk_src); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c b/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c index c6136e0ed1a4..147d77173e2b 100644 --- a/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce112/dce112_resource.c @@ -687,6 +687,7 @@ struct clock_source *dce112_clock_source_create( return _src->base; } + kfree(clk_src); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c b/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c index 4a6ba3173a5a..0b5eeff17d00 100644 --- a/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce120/dce120_resource.c @@ -500,6 +500,7 @@ static struct clock_source *dce120_clock_source_create( return _src->base; } + kfree(clk_src); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c b/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c index 860a524ebcfa..952440893fbb 100644 --- a/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dce80/dce80_resource.c @@ -701,6 +701,7 @@ struct clock_source *dce80_clock_source_create( return _src->base; } + kfree(clk_src); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c index a12530a3ab9c..3f25e8da5396 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c @@ -786,6 +786,7 @@ struct clock_source *dcn10_clock_source_create( return _src->base; } + kfree(clk_src); BREAK_TO_DEBUGGER(); return NULL; } diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c index b949e202d6cb..418fdcf1f492 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c @@ -955,6 +955,7 @@ struct clock_source *dcn20_clock_source_create( return _src->base; } + kfree(clk_src) BREAK_TO_DEBUGGER(); return NULL; } -- 2.17.1