From: Markus Elfring elfring@users.sourceforge.net Date: Sun, 28 Jun 2015 10:27:35 +0200
The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index fec487d..a85cd08 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1575,8 +1575,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev) amdgpu_fence_driver_fini(adev); amdgpu_fbdev_fini(adev); r = amdgpu_fini(adev); - if (adev->ip_block_enabled) - kfree(adev->ip_block_enabled); + kfree(adev->ip_block_enabled); adev->ip_block_enabled = NULL; adev->accel_working = false; /* free i2c buses */
On Sun, Jun 28, 2015 at 4:45 AM, SF Markus Elfring elfring@users.sourceforge.net wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Sun, 28 Jun 2015 10:27:35 +0200
The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
I already have the same patch from Maninder Singh.
Thanks!
Alex
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index fec487d..a85cd08 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -1575,8 +1575,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev) amdgpu_fence_driver_fini(adev); amdgpu_fbdev_fini(adev); r = amdgpu_fini(adev);
if (adev->ip_block_enabled)kfree(adev->ip_block_enabled);
kfree(adev->ip_block_enabled); adev->ip_block_enabled = NULL; adev->accel_working = false; /* free i2c buses */-- 2.4.4
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 16:23:21 +0200
Further update suggestions were taken into account after patches were applied from static source code analysis.
Markus Elfring (8): Delete an unnecessary check before drm_gem_object_unreference_unlocked() Delete unnecessary checks before the function call "kfree" One function call less in amdgpu_cgs_acpi_eval_object() after error detection Delete a variable in amdgpu_cgs_acpi_eval_object() Delete an unnecessary variable initialisation in amdgpu_cgs_acpi_eval_object() Change assignment for a variable in amdgpu_cgs_acpi_eval_object() Change assignment for a buffer variable in phm_dispatch_table() Delete an unnecessary variable initialisation in phm_dispatch_table()
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 28 ++++++++++------------ drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 +--- .../gpu/drm/amd/powerplay/hwmgr/functiontables.c | 12 ++++------ 3 files changed, 19 insertions(+), 25 deletions(-)
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 11:28:36 +0200
The drm_gem_object_unreference_unlocked() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index a6eecf6..2a07b15 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -516,9 +516,7 @@ static void amdgpu_user_framebuffer_destroy(struct drm_framebuffer *fb) { struct amdgpu_framebuffer *amdgpu_fb = to_amdgpu_framebuffer(fb);
- if (amdgpu_fb->obj) { - drm_gem_object_unreference_unlocked(amdgpu_fb->obj); - } + drm_gem_object_unreference_unlocked(amdgpu_fb->obj); drm_framebuffer_cleanup(fb); kfree(amdgpu_fb); }
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 12:38:12 +0200
The kfree() function tests whether its argument is NULL and then returns immediately. Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 3 +-- drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c | 5 +---- 2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c index cf6f49f..6f11bc1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c @@ -1053,8 +1053,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, }
error: - if (obj != NULL) - kfree(obj); + kfree(obj); kfree((void *)input.pointer); return result; } diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c index 7a705ce..024e22e 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c @@ -76,10 +76,7 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr, }
result = phm_run_table(hwmgr, rt_table, input, output, temp_storage); - - if (NULL != temp_storage) - kfree(temp_storage); - + kfree(temp_storage); return result; }
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 13:43:44 +0200
The kfree() function was called in one case by the amdgpu_cgs_acpi_eval_object() function during error handling even if the passed variable "obj" contained a null pointer.
* Adjust jump targets according to the Linux coding style convention.
* Delete unnecessary initialisations for the variables "obj" and "params" then.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c index 6f11bc1..705bfa2 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c @@ -903,8 +903,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, acpi_handle handle; struct acpi_object_list input; struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; - union acpi_object *params = NULL; - union acpi_object *obj = NULL; + union acpi_object *params, *obj; uint8_t name[5] = {'\0'}; struct cgs_acpi_method_argument *argument = NULL; uint32_t i, count; @@ -996,7 +995,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device,
if (ACPI_FAILURE(status)) { result = -EIO; - goto error; + goto free_input; }
/* return the output info */ @@ -1006,7 +1005,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, if ((obj->type != ACPI_TYPE_PACKAGE) || (obj->package.count != count)) { result = -EIO; - goto error; + goto free_obj; } params = obj->package.elements; } else @@ -1014,13 +1013,13 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device,
if (params == NULL) { result = -EIO; - goto error; + goto free_obj; }
for (i = 0; i < count; i++) { if (argument->type != params->type) { result = -EIO; - goto error; + goto free_obj; } switch (params->type) { case ACPI_TYPE_INTEGER: @@ -1030,7 +1029,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, if ((params->string.length != argument->data_length) || (params->string.pointer == NULL)) { result = -EIO; - goto error; + goto free_obj; } strncpy(argument->pointer, params->string.pointer, @@ -1039,7 +1038,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, case ACPI_TYPE_BUFFER: if (params->buffer.pointer == NULL) { result = -EIO; - goto error; + goto free_obj; } memcpy(argument->pointer, params->buffer.pointer, @@ -1052,8 +1051,9 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, params++; }
-error: +free_obj: kfree(obj); +free_input: kfree((void *)input.pointer); return result; }
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 14:00:28 +0200
The local variable "func_no" was assigned a value at two places. But it was not read within this function. Thus delete it.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c index 705bfa2..f5de510 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c @@ -909,7 +909,6 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, uint32_t i, count; acpi_status status; int result = 0; - uint32_t func_no = 0xFFFFFFFF;
handle = ACPI_HANDLE(&adev->pdev->dev); if (!handle) @@ -926,7 +925,6 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, if (info->pinput_argument == NULL) return -EINVAL; argument = info->pinput_argument; - func_no = argument->value; for (i = 0; i < info->input_count; i++) { if (((argument->type == ACPI_TYPE_STRING) || (argument->type == ACPI_TYPE_BUFFER)) &&
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 14:54:12 +0200
The variable "argument" will be set to an appropriate value a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c index f5de510..47f2a43 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c @@ -905,7 +905,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *params, *obj; uint8_t name[5] = {'\0'}; - struct cgs_acpi_method_argument *argument = NULL; + struct cgs_acpi_method_argument *argument; uint32_t i, count; acpi_status status; int result = 0;
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 14:54:12 +0200
The variable "argument" will be set to an appropriate value a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c index f5de510..47f2a43 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c @@ -905,7 +905,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *params, *obj; uint8_t name[5] = {'\0'}; - struct cgs_acpi_method_argument *argument = NULL; + struct cgs_acpi_method_argument *argument; uint32_t i, count; acpi_status status; int result = 0;
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 15:05:45 +0200
Indicate successful function execution only at the end. Thus omit initialisation for the variable "result" at the beginning.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c index 47f2a43..57859bd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c @@ -908,7 +908,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, struct cgs_acpi_method_argument *argument; uint32_t i, count; acpi_status status; - int result = 0; + int result;
handle = ACPI_HANDLE(&adev->pdev->dev); if (!handle) @@ -1049,6 +1049,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device *cgs_device, params++; }
+ result = 0; free_obj: kfree(obj); free_input:
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 15:36:36 +0200
The variable "temp_storage" was eventually reassigned with a pointer. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c index 024e22e..735aeb0 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c @@ -60,7 +60,7 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr, void *input, void *output) { int result = 0; - void *temp_storage = NULL; + void *temp_storage;
if (hwmgr == NULL || rt_table == NULL) { printk(KERN_ERR "[ powerplay ] Invalid Parameter!\n"); @@ -73,7 +73,8 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr, printk(KERN_ERR "[ powerplay ] Could not allocate table temporary storage\n"); return -ENOMEM; } - } + } else + temp_storage = NULL;
result = phm_run_table(hwmgr, rt_table, input, output, temp_storage); kfree(temp_storage);
Am 16.07.2016 17:08, schrieb SF Markus Elfring:
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 15:36:36 +0200
The variable "temp_storage" was eventually reassigned with a pointer. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c index 024e22e..735aeb0 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c @@ -60,7 +60,7 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr, void *input, void *output) { int result = 0;
- void *temp_storage = NULL;
void *temp_storage;
if (hwmgr == NULL || rt_table == NULL) { printk(KERN_ERR "[ powerplay ] Invalid Parameter!\n");
@@ -73,7 +73,8 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr, printk(KERN_ERR "[ powerplay ] Could not allocate table temporary storage\n"); return -ENOMEM; }
- }
} else
temp_storage = NULL;result = phm_run_table(hwmgr, rt_table, input, output, temp_storage); kfree(temp_storage);
the handling of rt_table->storage_size == 0 is so better visible.
IMHO in this case the function could return directly either with -EINVAL; or with 0; -> more direct more obvious.
if (rt_table->storage_size == 0 ) return 0;
re, wh
From 0bf6f3c40786e12d3d42672f1d56296b30e17ac9 Mon Sep 17 00:00:00 2001
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 15:50:18 +0200 Subject: [PATCH 8/8] drm/amd/powerplay: Delete an unnecessary variable initialisation in phm_dispatch_table()
The variable "result" will be set to an appropriate value a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c index 735aeb0..fdfed63 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c @@ -59,7 +59,7 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr, struct phm_runtime_table_header *rt_table, void *input, void *output) { - int result = 0; + int result; void *temp_storage;
if (hwmgr == NULL || rt_table == NULL) {
Am 16.07.2016 um 16:33 schrieb SF Markus Elfring:
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 16:23:21 +0200
Further update suggestions were taken into account after patches were applied from static source code analysis.
Small coding style nit pick on patch #7:
- }
- } else
temp_storage = NULL;
When an "if" has "{" and "}" the else should also use them even when it is only one line.
With that fixed the whole series is Reviewed-by: Christian König christian.koenig@amd.com, but as Walter Harms pointed out as well there are a couple of other things we could make more as well.
Regards, Christian.
Markus Elfring (8): Delete an unnecessary check before drm_gem_object_unreference_unlocked() Delete unnecessary checks before the function call "kfree" One function call less in amdgpu_cgs_acpi_eval_object() after error detection Delete a variable in amdgpu_cgs_acpi_eval_object() Delete an unnecessary variable initialisation in amdgpu_cgs_acpi_eval_object() Change assignment for a variable in amdgpu_cgs_acpi_eval_object() Change assignment for a buffer variable in phm_dispatch_table() Delete an unnecessary variable initialisation in phm_dispatch_table()
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 28 ++++++++++------------ drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 +--- .../gpu/drm/amd/powerplay/hwmgr/functiontables.c | 12 ++++------ 3 files changed, 19 insertions(+), 25 deletions(-)
On Mon, Jul 18, 2016 at 3:41 AM, Christian König christian.koenig@amd.com wrote:
Am 16.07.2016 um 16:33 schrieb SF Markus Elfring:
From: Markus Elfring elfring@users.sourceforge.net Date: Sat, 16 Jul 2016 16:23:21 +0200
Further update suggestions were taken into account after patches were applied from static source code analysis.
Small coding style nit pick on patch #7:
}
} elsetemp_storage = NULL;When an "if" has "{" and "}" the else should also use them even when it is only one line.
With that fixed the whole series is Reviewed-by: Christian König christian.koenig@amd.com, but as Walter Harms pointed out as well there are a couple of other things we could make more as well.
Applied the series except patch 2 was which already fixed by an earlier patch.
Thanks!
Alex
Regards, Christian.
Markus Elfring (8): Delete an unnecessary check before drm_gem_object_unreference_unlocked() Delete unnecessary checks before the function call "kfree" One function call less in amdgpu_cgs_acpi_eval_object() after error detection Delete a variable in amdgpu_cgs_acpi_eval_object() Delete an unnecessary variable initialisation in amdgpu_cgs_acpi_eval_object() Change assignment for a variable in amdgpu_cgs_acpi_eval_object() Change assignment for a buffer variable in phm_dispatch_table() Delete an unnecessary variable initialisation in phm_dispatch_table()
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 28 ++++++++++------------ drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4 +--- .../gpu/drm/amd/powerplay/hwmgr/functiontables.c | 12 ++++------ 3 files changed, 19 insertions(+), 25 deletions(-)
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
SF Markus Elfring -
walter harms