[PATCH] drm/amdgpu: fix multiple memory leaks

[PATCH] drm/amdgpu: fix multiple memory leaks

[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.

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

Re: [PATCH] drm/amdgpu: fix multiple memory leaks

[Koenig, Christian]

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;
>   		}
>   	}

[PATCH v2] drm/amdgpu: fix multiple memory leaks

[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);
+	return ret;
 }
 
 /**
-- 
2.17.1

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

[Sven Van Asbroeck]

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?

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

[Koenig, Christian]

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;
>   }
>   
>   /**

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

[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:

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

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

[Koenig, Christian]

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

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

[Markus Elfring]

> 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

[PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[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;
 	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

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

[Navid Emamdoost]

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.

Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[Markus Elfring]

> +++ 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

Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[Koenig, Christian]

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;
>   }
>   
>   /**

[PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[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>
---
 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

Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[Navid Emamdoost]

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.

Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[Markus Elfring]

> ---

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

Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init

[Koenig, Christian]

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;
>   }
>   
>   /**