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; } }
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;
Thanks Christian for the feedback, I'll send a v2.
On Wed, Sep 18, 2019 at 12:31 PM Koenig, Christian Christian.Koenig@amd.com 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@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; } }
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; }
/**
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?
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; }
/**
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
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
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; }
/**
+++ 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/Docu...
@@ -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
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;@@ -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;@@ -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; }
/**
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; }
/**
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...
Regards, Markus
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;@@ -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;@@ -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; }
/**
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;}
/**
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.
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
dri-devel@lists.freedesktop.org
-
Koenig, Christian -
Markus Elfring -
Navid Emamdoost -
Sven Van Asbroeck