ACPI-fan: Another source code review around null pointer handling?

ACPI-fan: Another source code review around null pointer handling?

[SF Markus Elfring]

Hello,

I have looked at the source file for the ACPI fan driver once more.
I would appreciate if a specific implementation detail can be clarified there.

Static source code analysis can point out that functions like the following
share an approach for error detection and corresponding exception handling.
* acpi_fan_get_fif
* acpi_fan_get_fps
* fan_get_state_acpi4
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/acpi/fan.c?id=80c75a0f1d81922bf322c0634d1e1a15825a89e6#n107

Can it matter eventually to handle a detected null pointer differently from
further checks for an attribute like "obj->type" or "obj->package"?

Regards,
Markus

[PATCH 00/21] ACPI-video: Fine-tuning for several function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 18:22:11 +0200

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (21):
  Use kmalloc_array() in acpi_video_get_levels()
  Return directly after a failed device query
  Delete an error message for a failed kzalloc() call
  Rename jump labels in acpi_video_get_levels()
  Delete an unnecessary initialisation in acpi_video_get_levels()
  Move four assignments in acpi_video_get_levels()
  Rename jump labels in acpi_video_bus_add()
  Improve a size determination in acpi_video_bus_add()
  Rename jump labels in acpi_video_register()
  Return directly after a failed input_allocate_device()
  Rename jump labels in acpi_video_bus_add_notify_handler()
  Delete unnecessary if statement in acpi_video_switch_brightness()
  Improve a jump target in acpi_video_switch_brightness()
  Improve a size determination in acpi_video_device_enumerate()
  Delete an unnecessary initialisation in acpi_video_device_enumerate()
  Rename jump labels in acpi_video_device_enumerate()
  Delete an unnecessary initialisation in acpi_video_init_brightness()
  Rename jump labels in acpi_video_init_brightness()
  Rename a jump label in acpi_video_device_lcd_query_levels()
  Improve a size determination in acpi_video_dev_register_backlight()
  Improve a size determination in acpi_video_bus_get_one_device()

 drivers/acpi/acpi_video.c | 114 +++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 62 deletions(-)

-- 
2.10.0

[PATCH 01/21] ACPI-video: Use kmalloc_array() in acpi_video_get_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 11:33:30 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index c5557d0..3d39b53 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -784,8 +784,9 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out;
 	}
 
-	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
-				GFP_KERNEL);
+	br->levels = kmalloc_array(obj->package.count + 2,
+				   sizeof(*br->levels),
+				   GFP_KERNEL);
 	if (!br->levels) {
 		result = -ENOMEM;
 		goto out_free;
-- 
2.10.0

[PATCH 02/21] ACPI-video: Return directly after a failed device query

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 13:25:01 +0200

Return directly after a function call "acpi_video_device_lcd_query_levels"
failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 3d39b53..8f0807a 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -768,8 +768,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 								&obj))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
 						"LCD brightness level\n"));
-		result = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
 	if (obj->package.count < 2) {
-- 
2.10.0

[PATCH 03/21] ACPI-video: Delete an error message for a failed kzalloc() call

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 13:57:33 +0200

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 8f0807a..420d125 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -778,7 +778,6 @@ int acpi_video_get_levels(struct acpi_device *device,
 
 	br = kzalloc(sizeof(*br), GFP_KERNEL);
 	if (!br) {
-		printk(KERN_ERR "can't allocate memory\n");
 		result = -ENOMEM;
 		goto out;
 	}
-- 
2.10.0

[PATCH 04/21] ACPI-video: Rename jump labels in acpi_video_get_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 14:11:43 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 420d125..82987db 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -773,13 +773,13 @@ int acpi_video_get_levels(struct acpi_device *device,
 
 	if (obj->package.count < 2) {
 		result = -EINVAL;
-		goto out;
+		goto free_object;
 	}
 
 	br = kzalloc(sizeof(*br), GFP_KERNEL);
 	if (!br) {
 		result = -ENOMEM;
-		goto out;
+		goto free_object;
 	}
 
 	br->levels = kmalloc_array(obj->package.count + 2,
@@ -787,7 +787,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 				   GFP_KERNEL);
 	if (!br->levels) {
 		result = -ENOMEM;
-		goto out_free;
+		goto free_brightness;
 	}
 
 	for (i = 0; i < obj->package.count; i++) {
@@ -843,13 +843,12 @@ int acpi_video_get_levels(struct acpi_device *device,
 	*dev_br = br;
 	if (pmax_level)
 		*pmax_level = max_level;
-
-out:
+ free_object:
 	kfree(obj);
 	return result;
-out_free:
+ free_brightness:
 	kfree(br);
-	goto out;
+	goto free_object;
 }
 EXPORT_SYMBOL(acpi_video_get_levels);
 
-- 
2.10.0

[PATCH 05/21] ACPI-video: Delete an unnecessary initialisation in acpi_video_get_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 14:23:55 +0200

The local variable "br" 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/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 82987db..0799865 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -760,7 +760,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 	union acpi_object *obj = NULL;
 	int i, max_level = 0, count = 0, level_ac_battery = 0;
 	union acpi_object *o;
-	struct acpi_video_device_brightness *br = NULL;
+	struct acpi_video_device_brightness *br;
 	int result = 0;
 	u32 value;
 
-- 
2.10.0

[PATCH 06/21] ACPI-video: Move four assignments in acpi_video_get_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 14:40:46 +0200

Move the assignments for four local variables so that they will only
be performed if the corresponding data processing succeeded
by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 0799865..0fca196 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -758,10 +758,10 @@ int acpi_video_get_levels(struct acpi_device *device,
 			  int *pmax_level)
 {
 	union acpi_object *obj = NULL;
-	int i, max_level = 0, count = 0, level_ac_battery = 0;
+	int i, max_level, count, level_ac_battery;
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br;
-	int result = 0;
+	int result;
 	u32 value;
 
 	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->handle,
@@ -790,6 +790,8 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto free_brightness;
 	}
 
+	max_level = 0;
+	count = 0;
 	for (i = 0; i < obj->package.count; i++) {
 		o = (union acpi_object *)&obj->package.elements[i];
 		if (o->type != ACPI_TYPE_INTEGER) {
@@ -814,6 +816,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 	 * In this case, the first two elements in _BCL packages
 	 * are also supported brightness levels that OS should take care of.
 	 */
+	level_ac_battery = 0;
 	for (i = 2; i < count; i++) {
 		if (br->levels[i] == br->levels[0])
 			level_ac_battery++;
@@ -843,6 +846,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 	*dev_br = br;
 	if (pmax_level)
 		*pmax_level = max_level;
+	result = 0;
  free_object:
 	kfree(obj);
 	return result;
-- 
2.10.0

[PATCH 07/21] ACPI-video: Rename jump labels in acpi_video_bus_add()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 15:01:23 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 0fca196..8003c90 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1983,14 +1983,14 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	acpi_video_bus_find_cap(video);
 	error = acpi_video_bus_check(video);
 	if (error)
-		goto err_free_video;
+		goto free_video;
 
 	mutex_init(&video->device_list_lock);
 	INIT_LIST_HEAD(&video->video_device_list);
 
 	error = acpi_video_bus_get_devices(video, device);
 	if (error)
-		goto err_put_video;
+		goto put_devices;
 
 	printk(KERN_INFO PREFIX "%s [%s] (multi-head: %s  rom: %s  post: %s)\n",
 	       ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device),
@@ -2005,11 +2005,10 @@ static int acpi_video_bus_add(struct acpi_device *device)
 	acpi_video_bus_add_notify_handler(video);
 
 	return 0;
-
-err_put_video:
+ put_devices:
 	acpi_video_bus_put_devices(video);
 	kfree(video->attached_array);
-err_free_video:
+ free_video:
 	kfree(video);
 	device->driver_data = NULL;
 
-- 
2.10.0

[PATCH 08/21] ACPI-video: Improve a size determination in acpi_video_bus_add()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 15:16:26 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 8003c90..c9fbc6e 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1958,7 +1958,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
 			return -ENODEV;
 	}
 
-	video = kzalloc(sizeof(struct acpi_video_bus), GFP_KERNEL);
+	video = kzalloc(sizeof(*video), GFP_KERNEL);
 	if (!video)
 		return -ENOMEM;
 
-- 
2.10.0

[PATCH 09/21] ACPI-video: Rename jump labels in acpi_video_register()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 15:28:12 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index c9fbc6e..74e5a40 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -2080,22 +2080,21 @@ int acpi_video_register(void)
 		 * if the function of acpi_video_register is already called,
 		 * don't register the acpi_vide_bus again and return no error.
 		 */
-		goto leave;
+		goto unlock;
 	}
 
 	dmi_check_system(video_dmi_table);
 
 	ret = acpi_bus_register_driver(&acpi_video_bus);
 	if (ret)
-		goto leave;
+		goto unlock;
 
 	/*
 	 * When the acpi_video_bus is loaded successfully, increase
 	 * the counter reference.
 	 */
 	register_count = 1;
-
-leave:
+ unlock:
 	mutex_unlock(&register_count_mutex);
 	return ret;
 }
-- 
2.10.0

[PATCH 10/21] ACPI-video: Return directly after a failed input_allocate_device()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 15:46:19 +0200

* Return directly after a call of the function "input_allocate_device"
  failed at the beginning.

* Delete the jump label "out" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 74e5a40..6c871dd 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1852,10 +1852,8 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
 	int error;
 
 	video->input = input = input_allocate_device();
-	if (!input) {
-		error = -ENOMEM;
-		goto out;
-	}
+	if (!input)
+		return -ENOMEM;
 
 	error = acpi_video_bus_start_devices(video);
 	if (error)
@@ -1895,7 +1893,6 @@ err_stop_dev:
 err_free_input:
 	input_free_device(input);
 	video->input = NULL;
-out:
 	return error;
 }
 
-- 
2.10.0

[PATCH 11/21] ACPI-video: Rename jump labels in acpi_video_bus_add_notify_handler()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 15:50:12 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 6c871dd..fe9b40e 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1857,7 +1857,7 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
 
 	error = acpi_video_bus_start_devices(video);
 	if (error)
-		goto err_free_input;
+		goto free_device;
 
 	snprintf(video->phys, sizeof(video->phys),
 			"%s/video/input0", acpi_device_hid(video->device));
@@ -1879,7 +1879,7 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
 
 	error = input_register_device(input);
 	if (error)
-		goto err_stop_dev;
+		goto stop_devices;
 
 	mutex_lock(&video->device_list_lock);
 	list_for_each_entry(dev, &video->video_device_list, entry)
@@ -1887,10 +1887,9 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
 	mutex_unlock(&video->device_list_lock);
 
 	return 0;
-
-err_stop_dev:
+ stop_devices:
 	acpi_video_bus_stop_devices(video);
-err_free_input:
+ free_device:
 	input_free_device(input);
 	video->input = NULL;
 	return error;
-- 
2.10.0

[PATCH 12/21] ACPI-video: Delete unnecessary if statement in acpi_video_switch_brightness()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 16:10:40 +0200

Move a function call into an else branch for successful function execution.
Omit a duplicate check for the local variable "result" then at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index fe9b40e..0362a43 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1372,14 +1372,12 @@ acpi_video_switch_brightness(struct work_struct *work)
 	level_next = acpi_video_get_next_level(device, level_current, event);
 
 	result = acpi_video_device_lcd_set_level(device, level_next);
-
-	if (!result)
-		backlight_force_update(device->backlight,
-				       BACKLIGHT_UPDATE_HOTKEY);
-
 out:
 	if (result)
 		printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
+	else
+		backlight_force_update(device->backlight,
+				       BACKLIGHT_UPDATE_HOTKEY);
 }
 
 int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
-- 
2.10.0

[PATCH 13/21] ACPI-video: Improve a jump target in acpi_video_switch_brightness()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 16:34:08 +0200

* Avoid another duplicate check for the local variable "result"
  then at the end.

* Jump directly to an error message in the case that the desired brightness
  can not be switched.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 0362a43..fe10d3f 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1361,19 +1361,19 @@ acpi_video_switch_brightness(struct work_struct *work)
 		return;
 
 	if (!device->brightness)
-		goto out;
+		goto report_failure;
 
 	result = acpi_video_device_lcd_get_level_current(device,
 							 &level_current,
 							 false);
 	if (result)
-		goto out;
+		goto report_failure;
 
 	level_next = acpi_video_get_next_level(device, level_current, event);
 
 	result = acpi_video_device_lcd_set_level(device, level_next);
-out:
 	if (result)
+ report_failure:
 		printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
 	else
 		backlight_force_update(device->backlight,
-- 
2.10.0

[PATCH 14/21] ACPI-video: Improve a size determination in acpi_video_device_enumerate()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 16:45:41 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index fe10d3f..df06390 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1268,7 +1268,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 			  dod->package.count));
 
 	active_list = kcalloc(1 + dod->package.count,
-			      sizeof(struct acpi_video_enumerated_device),
+			      sizeof(*active_list),
 			      GFP_KERNEL);
 	if (!active_list) {
 		status = -ENOMEM;
-- 
2.10.0

[PATCH 15/21] ACPI-video: Delete an unnecessary initialisation in acpi_video_device_enumerate()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 16:54:55 +0200

The local variable "dod" 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/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index df06390..fcf74e7 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1245,7 +1245,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 	int i;
 	struct acpi_video_enumerated_device *active_list;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *dod = NULL;
+	union acpi_object *dod;
 	union acpi_object *obj;
 
 	if (!video->cap._DOD)
-- 
2.10.0

[PATCH 16/21] ACPI-video: Rename jump labels in acpi_video_device_enumerate()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 17:01:30 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index fcf74e7..2fc775a 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1261,7 +1261,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 	if (!dod || (dod->type != ACPI_TYPE_PACKAGE)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Invalid _DOD data"));
 		status = -EFAULT;
-		goto out;
+		goto free_buffer;
 	}
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d video heads in _DOD\n",
@@ -1272,7 +1272,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 			      GFP_KERNEL);
 	if (!active_list) {
 		status = -ENOMEM;
-		goto out;
+		goto free_buffer;
 	}
 
 	count = 0;
@@ -1296,8 +1296,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 
 	video->attached_array = active_list;
 	video->attached_count = count;
-
-out:
+ free_buffer:
 	kfree(buffer.pointer);
 	return status;
 }
-- 
2.10.0

[PATCH 17/21] ACPI-video: Delete an unnecessary initialisation in acpi_video_init_brightness()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 17:20:22 +0200

The local variable "result" will be assigned with a statement that follows
it directly. Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 2fc775a..5f1ef6e 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -872,7 +872,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	int i, max_level = 0;
 	unsigned long long level, level_old;
 	struct acpi_video_device_brightness *br = NULL;
-	int result = -EINVAL;
+	int result;
 
 	result = acpi_video_get_levels(device->dev, &br, &max_level);
 	if (result)
-- 
2.10.0

[PATCH 18/21] ACPI-video: Rename jump labels in acpi_video_init_brightness()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 17:28:45 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 5f1ef6e..5566f68 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -888,11 +888,11 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	result = acpi_video_device_lcd_get_level_current(device,
 							 &level_old, true);
 	if (result)
-		goto out_free_levels;
+		goto free_levels;
 
 	result = acpi_video_bqc_quirk(device, max_level, level_old);
 	if (result)
-		goto out_free_levels;
+		goto free_levels;
 	/*
 	 * cap._BQC may get cleared due to _BQC is found to be broken
 	 * in acpi_video_bqc_quirk, so check again here.
@@ -912,17 +912,15 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 			break;
 	if (i == br->count || !level)
 		level = max_level;
-
-set_level:
+ set_level:
 	result = acpi_video_device_lcd_set_level(device, level);
 	if (result)
-		goto out_free_levels;
+		goto free_levels;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			  "found %d brightness levels\n", br->count - 2));
 	return 0;
-
-out_free_levels:
+ free_levels:
 	kfree(br->levels);
 	kfree(br);
 	device->brightness = NULL;
-- 
2.10.0

[PATCH 19/21] ACPI-video: Rename a jump label in acpi_video_device_lcd_query_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 17:34:40 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 5566f68..231fab3 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -329,14 +329,13 @@ acpi_video_device_lcd_query_levels(acpi_handle handle,
 	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
 		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
 		status = -EFAULT;
-		goto err;
+		goto free_buffer;
 	}
 
 	*levels = obj;
 
 	return 0;
-
-err:
+ free_buffer:
 	kfree(buffer.pointer);
 
 	return status;
-- 
2.10.0

[PATCH 20/21] ACPI-video: Improve a size determination in acpi_video_dev_register_backlight()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 17:45:40 +0200

Replace the specification of a data structure by a reference for
a local variable as the parameter for the operator "sizeof" to make
the corresponding size determination a bit safer according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 231fab3..9685725 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1685,7 +1685,7 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 		pci_dev_put(pdev);
 	}
 
-	memset(&props, 0, sizeof(struct backlight_properties));
+	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_FIRMWARE;
 	props.max_brightness = device->brightness->count - 3;
 	device->backlight = backlight_device_register(name,
-- 
2.10.0

[PATCH 21/21] ACPI-video: Improve a size determination in acpi_video_bus_get_one_device()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 17:50:04 +0200

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 9685725..4cd693e 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1084,7 +1084,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	if (ACPI_FAILURE(status))
 		return 0;
 
-	data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-- 
2.10.0

[PATCH 0/7] ACPI-APEI-HEST: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 22:05:05 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (7):
  Fix a typo in a comment line
  Use kmalloc_array() in hest_ghes_dev_register()
  Move an assignment in hest_ghes_dev_register()
  Rename jump labels in hest_ghes_dev_register()
  Rename jump labels in acpi_hest_init()
  Reduce the scope for a variable in acpi_hest_init()
  Rename jump labels in hest_parse_ghes()

 drivers/acpi/apei/hest.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

-- 
2.10.0

[PATCH 1/7] ACPI-APEI-HEST: Fix a typo in a comment line

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 20:37:38 +0200

Add a missing character to the fourth word at the beginning of this file.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 20b3fcf..e170885 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -1,5 +1,5 @@
 /*
- * APEI Hardware Error Souce Table support
+ * APEI Hardware Error Source Table support
  *
  * HEST describes error sources in detail; communicates operational
  * parameters (i.e. severity levels, masking bits, and threshold
-- 
2.10.0

[PATCH 2/7] ACPI-APEI-HEST: Use kmalloc_array() in hest_ghes_dev_register()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 20:55:33 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index e170885..a852237 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -186,7 +186,9 @@ static int __init hest_ghes_dev_register(unsigned int ghes_count)
 	struct ghes_arr ghes_arr;
 
 	ghes_arr.count = 0;
-	ghes_arr.ghes_devs = kmalloc(sizeof(void *) * ghes_count, GFP_KERNEL);
+	ghes_arr.ghes_devs = kmalloc_array(ghes_count,
+					   sizeof(*ghes_arr.ghes_devs),
+					   GFP_KERNEL);
 	if (!ghes_arr.ghes_devs)
 		return -ENOMEM;
 
-- 
2.10.0

[PATCH 3/7] ACPI-APEI-HEST: Move an assignment in hest_ghes_dev_register()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:15:22 +0200

Move one assignment for a data structure member in one local variable
so that its setting will only be performed after a corresponding memory
allocation succeeded by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index a852237..26f5e78 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -185,13 +185,13 @@ static int __init hest_ghes_dev_register(unsigned int ghes_count)
 	int rc, i;
 	struct ghes_arr ghes_arr;
 
-	ghes_arr.count = 0;
 	ghes_arr.ghes_devs = kmalloc_array(ghes_count,
 					   sizeof(*ghes_arr.ghes_devs),
 					   GFP_KERNEL);
 	if (!ghes_arr.ghes_devs)
 		return -ENOMEM;
 
+	ghes_arr.count = 0;
 	rc = apei_hest_parse(hest_parse_ghes, &ghes_arr);
 	if (rc)
 		goto err;
-- 
2.10.0

[PATCH 4/7] ACPI-APEI-HEST: Rename jump labels in hest_ghes_dev_register()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:22:55 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 26f5e78..03dd7d3 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -194,14 +194,14 @@ static int __init hest_ghes_dev_register(unsigned int ghes_count)
 	ghes_arr.count = 0;
 	rc = apei_hest_parse(hest_parse_ghes, &ghes_arr);
 	if (rc)
-		goto err;
-out:
+		goto unregister;
+ free_array:
 	kfree(ghes_arr.ghes_devs);
 	return rc;
-err:
+ unregister:
 	for (i = 0; i < ghes_arr.count; i++)
 		platform_device_unregister(ghes_arr.ghes_devs[i]);
-	goto out;
+	goto free_array;
 }
 
 static int __init setup_hest_disable(char *str)
-- 
2.10.0

[PATCH 5/7] ACPI-APEI-HEST: Rename jump labels in acpi_hest_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:30:06 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 03dd7d3..ddff1b1 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -226,12 +226,12 @@ void __init acpi_hest_init(void)
 	status = acpi_get_table(ACPI_SIG_HEST, 0,
 				(struct acpi_table_header **)&hest_tab);
 	if (status == AE_NOT_FOUND)
-		goto err;
+		goto disable_hest;
 	else if (ACPI_FAILURE(status)) {
 		const char *msg = acpi_format_exception(status);
 		pr_err(HEST_PFX "Failed to get table, %s\n", msg);
 		rc = -EINVAL;
-		goto err;
+		goto disable_hest;
 	}
 
 	if (!acpi_disable_cmcff)
@@ -240,14 +240,14 @@ void __init acpi_hest_init(void)
 	if (!ghes_disable) {
 		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
 		if (rc)
-			goto err;
+			goto disable_hest;
 		rc = hest_ghes_dev_register(ghes_count);
 		if (rc)
-			goto err;
+			goto disable_hest;
 	}
 
 	pr_info(HEST_PFX "Table parsing has been initialized.\n");
 	return;
-err:
+ disable_hest:
 	hest_disable = 1;
 }
-- 
2.10.0

[PATCH 6/7] ACPI-APEI-HEST: Reduce the scope for a variable in acpi_hest_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:50:34 +0200

Move the definition for the local variable "ghes_count" into an if branch
so that the corresponding setting will only be performed if GHES could be
enabled by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index ddff1b1..0e629c0 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -216,7 +216,6 @@ void __init acpi_hest_init(void)
 {
 	acpi_status status;
 	int rc = -ENODEV;
-	unsigned int ghes_count = 0;
 
 	if (hest_disable) {
 		pr_info(HEST_PFX "Table parsing disabled.\n");
@@ -238,6 +237,8 @@ void __init acpi_hest_init(void)
 		apei_hest_parse(hest_parse_cmc, NULL);
 
 	if (!ghes_disable) {
+		unsigned int ghes_count = 0;
+
 		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
 		if (rc)
 			goto disable_hest;
-- 
2.10.0

[PATCH 7/7] ACPI-APEI-HEST: Rename jump labels in hest_parse_ghes()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:56:02 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 0e629c0..d5b75fe 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -167,15 +167,15 @@ static int __init hest_parse_ghes(struct acpi_hest_header *hest_hdr, void *data)
 
 	rc = platform_device_add_data(ghes_dev, &hest_hdr, sizeof(void *));
 	if (rc)
-		goto err;
+		goto put_device;
 
 	rc = platform_device_add(ghes_dev);
 	if (rc)
-		goto err;
+		goto put_device;
 	ghes_arr->ghes_devs[ghes_arr->count++] = ghes_dev;
 
 	return 0;
-err:
+ put_device:
 	platform_device_put(ghes_dev);
 	return rc;
 }
-- 
2.10.0

Re: [PATCH 00/21] ACPI-video: Fine-tuning for several function implementations

[Rafael J. Wysocki]

On Mon, Sep 5, 2016 at 6:42 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 5 Sep 2016 18:22:11 +0200
>
> Several update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (21):
>   Use kmalloc_array() in acpi_video_get_levels()
>   Return directly after a failed device query
>   Delete an error message for a failed kzalloc() call
>   Rename jump labels in acpi_video_get_levels()
>   Delete an unnecessary initialisation in acpi_video_get_levels()
>   Move four assignments in acpi_video_get_levels()
>   Rename jump labels in acpi_video_bus_add()
>   Improve a size determination in acpi_video_bus_add()
>   Rename jump labels in acpi_video_register()
>   Return directly after a failed input_allocate_device()
>   Rename jump labels in acpi_video_bus_add_notify_handler()
>   Delete unnecessary if statement in acpi_video_switch_brightness()
>   Improve a jump target in acpi_video_switch_brightness()
>   Improve a size determination in acpi_video_device_enumerate()
>   Delete an unnecessary initialisation in acpi_video_device_enumerate()
>   Rename jump labels in acpi_video_device_enumerate()
>   Delete an unnecessary initialisation in acpi_video_init_brightness()
>   Rename jump labels in acpi_video_init_brightness()
>   Rename a jump label in acpi_video_device_lcd_query_levels()
>   Improve a size determination in acpi_video_dev_register_backlight()
>   Improve a size determination in acpi_video_bus_get_one_device()

I'd prefer this to be combined into fewer patches that each will
address several issues of one type, ie. put all label renames into one
patch, all size determination improvements into another one and so on.

Thanks,
Rafael

Re: [PATCH 0/7] ACPI-APEI-HEST: Fine-tuning for three function implementations

[Rafael J. Wysocki]

On Mon, Sep 5, 2016 at 10:12 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 5 Sep 2016 22:05:05 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (7):
>   Fix a typo in a comment line
>   Use kmalloc_array() in hest_ghes_dev_register()
>   Move an assignment in hest_ghes_dev_register()
>   Rename jump labels in hest_ghes_dev_register()
>   Rename jump labels in acpi_hest_init()
>   Reduce the scope for a variable in acpi_hest_init()
>   Rename jump labels in hest_parse_ghes()

Like in the other patch series I've just commented, please put all
label renames into one patch.

Thanks,
Rafael

Re: ACPI-video: Fine-tuning for several function implementations

[SF Markus Elfring]

> I'd prefer this to be combined into fewer patches
> that each will address several issues of one type,

I understand your concern a bit in principle.


> ie. put all label renames into one patch,

Are any of my update suggestions controversial here?


> all size determination improvements into another one and so on.

I am unsure about the acceptance for the selected software change opportunities.
So I chose a very specific patch granularity intentionally.

I tend to provide some change ideas for each affected function
implementation individually. I imagine that this way should support
the recombination of update steps to some degree already, shouldn't it?

Regards,
Markus

Re: ACPI-APEI-HEST: Fine-tuning for three function implementations

[SF Markus Elfring]

> Like in the other patch series I've just commented,

Thanks for your quick response.


> please put all label renames into one patch.

Could you accept these update suggestions generally?


I would prefer to avoid squashing special changes together by default.

Regards,
Markus

Re: ACPI-video: Fine-tuning for several function implementations

[Rafael J. Wysocki]

On Tue, Sep 6, 2016 at 5:28 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> I'd prefer this to be combined into fewer patches
>> that each will address several issues of one type,
>
> I understand your concern a bit in principle.
>
>
>> ie. put all label renames into one patch,
>
> Are any of my update suggestions controversial here?

Well, the label renames have a little value in general IMO, but that
depends on a particular case.

Anyway, if there's something I don't like in particular, I'll let you know.

>> all size determination improvements into another one and so on.
>
> I am unsure about the acceptance for the selected software change opportunities.
> So I chose a very specific patch granularity intentionally.
>
> I tend to provide some change ideas for each affected function
> implementation individually. I imagine that this way should support
> the recombination of update steps to some degree already, shouldn't it?

However, it's a pain to review 20 patches if you could review 4 instead.

Please take the reviewers' time into consideration too.

Thanks,
Rafael

Re: ACPI-APEI-HEST: Fine-tuning for three function implementations

[Rafael J. Wysocki]

On Tue, Sep 6, 2016 at 5:38 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Like in the other patch series I've just commented,
>
> Thanks for your quick response.
>
>
>> please put all label renames into one patch.
>
> Could you accept these update suggestions generally?
>
>
> I would prefer to avoid squashing special changes together by default.

Well, as I said elsewhere, if you make the same type of change in
multiple places in one piece of code (or code maintained by the same
maintainer), you make it easier to review those changes if they go in
one patch.

Thanks,
Rafael

Re: ACPI-video: Fine-tuning for several function implementations

[SF Markus Elfring]

> Anyway, if there's something I don't like in particular, I'll let you know.

Thanks for your general interest.

I hope that occasional disagreements can be resolved in constructive ways.


> However, it's a pain to review 20 patches if you could review 4 instead.

Are there any more possibilities to improve the convenience for this
change review process with advanced tools?


> Please take the reviewers' time into consideration too.

I am trying this to some degree.

But I guess that it is hard to do something about corresponding efforts
when various contributors can easily spot many software update opportunities
in the discussed source files, isn't it?

Regards,
Markus

Re: ACPI-APEI-HEST: Fine-tuning for three function implementations

[SF Markus Elfring]

> Well, as I said elsewhere, if you make the same type of change in
> multiple places in one piece of code (or code maintained by the same maintainer),

I can imagine that my update suggestions will also trigger some additional
development efforts. I assume that some contributors appreciate
fine-grained patch series, don't they?


> you make it easier to review those changes if they go in one patch.

It might look convenient. But I find that there are more aspects to consider
for a better patch granularity.

Regards,
Markus

Re: ACPI-video: Fine-tuning for several function implementations

[Rafael J. Wysocki]

On Tue, Sep 6, 2016 at 4:10 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Anyway, if there's something I don't like in particular, I'll let you know.
>
> Thanks for your general interest.
>
> I hope that occasional disagreements can be resolved in constructive ways.
>
>
>> However, it's a pain to review 20 patches if you could review 4 instead.
>
> Are there any more possibilities to improve the convenience for this
> change review process with advanced tools?
>
>
>> Please take the reviewers' time into consideration too.
>
> I am trying this to some degree.
>
> But I guess that it is hard to do something about corresponding efforts
> when various contributors can easily spot many software update opportunities
> in the discussed source files, isn't it?

OK, look.

Your patches happen to modify code maintained by me.  From my
perspective the value of the changes made by them is marginal.
Nevertheless, I might take them if you made my life somewhat easier,
so I've tried to tell you politely how to do that.

If you're not willing to do it, though, this is where it ends.  And
attempts to convince me that I may not want my life to be easier after
all are not likely to succeed.

Thanks,
Rafael

Re: ACPI-video: Fine-tuning for several function implementations

[SF Markus Elfring]

> Your patches happen to modify code maintained by me.  From my
> perspective the value of the changes made by them is marginal.

Thanks for another bit of interesting information.


> Nevertheless, I might take them if you made my life somewhat easier,

I am also looking for further approaches to help you there.


> so I've tried to tell you politely how to do that.

This feedback is generally fine.


> If you're not willing to do it,

My willingness is depending on also some factors.


> though, this is where it ends.

I hope that a bit more clarification can improve the situation.


> And attempts to convince me that I may not want my life to be easier
> after all are not likely to succeed.

We usually want that life will become more comfortable.

I chose to contribute something to Linux source files for this purpose.
My knowledge evolved in the way that I am using some tools for
static source code analysis. Such advanced tools can point various
change opportunities out. I picked a few special search patterns up.
It happened then that hundreds of source files were found which contain
update candidates. I am trying to inform the corresponding developers
about improvement possibilities in affected systems.


Further challenges are relevant then as usual.

* Handling of the search process and their results

* Communication between contributors


Search patterns can occasionally be categorised as "too special".
The software technology contains also the risk for showing "false positives".

The reactions of code reviewers are varying between rejection and acceptance.
Now I would like to determine again which details of the proposed changes
have got a higher chance for acceptance.

The discussed concrete patch series is just another example for usual
difficulties or more interesting software development challenges.
I hope that they can be resolved in a systematic way.
I sent analysis results as a series of small software updates. I find
it important to understand them also in the way that they belong to
software design patterns. I can imagine that it is harder to recognise
the involved patterns from the presented combination of update steps.

Would you like to check and clarify these patterns once more
before the desired improvements will happen (in a software area you maintain)?


So there are further constraints to consider. My software development experience
leaded me to a very specific kind of patch granularity here.
My software development interest evolved also in the way that I dared
to fiddle with the source files "drivers/acpi/processor_perflib.c"
and "drivers/acpi/processor_throttling.c" yesterday.
The consequence is that I would to publish a corresponding series
of 30 update steps for integration into another source code repository.
It seems that I need to wait a bit more for the next contribution attempt
before the change acceptance will fit to such an approach.

Regards,
Markus

[PATCH v2 0/9] ACPI-video: Fine-tuning for several function implementations

[SF Markus Elfring]

>From 2a7010a6555091b685ed51d91fea23ff9058047c Mon Sep 17 00:00:00 2001
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 17:16:15 +0200
Subject: [PATCH v2 0/9] ACPI-video: Fine-tuning for several function implementations

Several update suggestions were taken into account
from static source code analysis.

Markus Elfring (9):
  Use kmalloc_array() in acpi_video_get_levels()
  Return directly after a failed device query
  Delete an error message for a failed kzalloc() call
  Return directly after a failed input_allocate_device()
  Delete unnecessary if statement in acpi_video_switch_brightness()
  Improve a jump target in acpi_video_switch_brightness()
  Delete an unnecessary initialisation in three functions
  Move four assignments in acpi_video_get_levels()
  Improve a size determination in four functions

---

v2:
* Desired changes were rebased on source files from Linux next-20170803.

* Some update steps were recombined.

 drivers/acpi/acpi_video.c | 53 ++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

-- 
2.13.4

[PATCH v2 1/9] ACPI-video: Use kmalloc_array() in acpi_video_get_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 14:52:25 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 0972ec0e2eb8..1aaf2b591ca8 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -832,8 +832,9 @@ int acpi_video_get_levels(struct acpi_device *device,
 	 * in order to account for buggy BIOS which don't export the first two
 	 * special levels (see below)
 	 */
-	br->levels = kmalloc((obj->package.count + ACPI_VIDEO_FIRST_LEVEL) *
-	                     sizeof(*br->levels), GFP_KERNEL);
+	br->levels = kmalloc_array(obj->package.count + ACPI_VIDEO_FIRST_LEVEL,
+				   sizeof(*br->levels),
+				   GFP_KERNEL);
 	if (!br->levels) {
 		result = -ENOMEM;
 		goto out_free;
-- 
2.13.4

[PATCH v2 2/9] ACPI-video: Return directly after a failed device query

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 15:00:45 +0200

Return directly after a function call "acpi_video_device_lcd_query_levels"
failed at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 1aaf2b591ca8..7a9cda6c0a6b 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -811,8 +811,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 								&obj))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
 						"LCD brightness level\n"));
-		result = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
 	if (obj->package.count < ACPI_VIDEO_FIRST_LEVEL) {
-- 
2.13.4

[PATCH v2 3/9] ACPI-video: Delete an error message for a failed kzalloc() call

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 15:07:53 +0200

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 7a9cda6c0a6b..f83ece9c830c 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -821,7 +821,6 @@ int acpi_video_get_levels(struct acpi_device *device,
 
 	br = kzalloc(sizeof(*br), GFP_KERNEL);
 	if (!br) {
-		printk(KERN_ERR "can't allocate memory\n");
 		result = -ENOMEM;
 		goto out;
 	}
-- 
2.13.4

[PATCH v2 4/9] ACPI-video: Return directly after a failed input_allocate_device()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 15:15:16 +0200

* Return directly after a call of the function "input_allocate_device"
  failed at the beginning.

* Delete the jump label "out" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index f83ece9c830c..09ea1f115f55 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1903,10 +1903,8 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
 	int error;
 
 	video->input = input = input_allocate_device();
-	if (!input) {
-		error = -ENOMEM;
-		goto out;
-	}
+	if (!input)
+		return -ENOMEM;
 
 	error = acpi_video_bus_start_devices(video);
 	if (error)
@@ -1946,7 +1944,6 @@ static int acpi_video_bus_add_notify_handler(struct acpi_video_bus *video)
 err_free_input:
 	input_free_device(input);
 	video->input = NULL;
-out:
 	return error;
 }
 
-- 
2.13.4

[PATCH v2 5/9] ACPI-video: Delete unnecessary if statement in acpi_video_switch_brightness()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 15:43:17 +0200

Move a function call into an else branch for successful function execution.
Omit a duplicate check for the local variable "result" then at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 09ea1f115f55..8295ae1deab9 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1422,14 +1422,12 @@ acpi_video_switch_brightness(struct work_struct *work)
 	level_next = acpi_video_get_next_level(device, level_current, event);
 
 	result = acpi_video_device_lcd_set_level(device, level_next);
-
-	if (!result)
-		backlight_force_update(device->backlight,
-				       BACKLIGHT_UPDATE_HOTKEY);
-
 out:
 	if (result)
 		printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
+	else
+		backlight_force_update(device->backlight,
+				       BACKLIGHT_UPDATE_HOTKEY);
 }
 
 int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
-- 
2.13.4

[PATCH v2 6/9] ACPI-video: Improve a jump target in acpi_video_switch_brightness()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 15:45:35 +0200

* Avoid another duplicate check for the local variable "result"
  then at the end.

* Jump directly to an error message in the case that the desired brightness
  can not be switched.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 8295ae1deab9..e279ed221961 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1411,19 +1411,19 @@ acpi_video_switch_brightness(struct work_struct *work)
 		return;
 
 	if (!device->brightness)
-		goto out;
+		goto report_failure;
 
 	result = acpi_video_device_lcd_get_level_current(device,
 							 &level_current,
 							 false);
 	if (result)
-		goto out;
+		goto report_failure;
 
 	level_next = acpi_video_get_next_level(device, level_current, event);
 
 	result = acpi_video_device_lcd_set_level(device, level_next);
-out:
 	if (result)
+ report_failure:
 		printk(KERN_ERR PREFIX "Failed to switch the brightness\n");
 	else
 		backlight_force_update(device->backlight,
-- 
2.13.4

[PATCH v2 7/9] ACPI-video: Delete an unnecessary initialisation in three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 15:55:35 +0200

Three local variables will be set to appropriate values a bit later.
Thus omit the explicit initialisation at the beginning of these functions.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index e279ed221961..a899faf6ff84 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -803,7 +803,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 	union acpi_object *obj = NULL;
 	int i, max_level = 0, count = 0, level_ac_battery = 0;
 	union acpi_object *o;
-	struct acpi_video_device_brightness *br = NULL;
+	struct acpi_video_device_brightness *br;
 	int result = 0;
 	u32 value;
 
@@ -921,7 +921,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	int i, max_level = 0;
 	unsigned long long level, level_old;
 	struct acpi_video_device_brightness *br = NULL;
-	int result = -EINVAL;
+	int result;
 
 	result = acpi_video_get_levels(device->dev, &br, &max_level);
 	if (result)
@@ -1295,7 +1295,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 	int i;
 	struct acpi_video_enumerated_device *active_list;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *dod = NULL;
+	union acpi_object *dod;
 	union acpi_object *obj;
 
 	if (!video->cap._DOD)
-- 
2.13.4

[PATCH v2 8/9] ACPI-video: Move four assignments in acpi_video_get_levels()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 16:21:17 +0200

Move the assignments for four local variables so that they will only
be performed if the corresponding data processing succeeded
by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index a899faf6ff84..3cc1798676ab 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -801,10 +801,10 @@ int acpi_video_get_levels(struct acpi_device *device,
 			  int *pmax_level)
 {
 	union acpi_object *obj = NULL;
-	int i, max_level = 0, count = 0, level_ac_battery = 0;
+	int i, max_level, count, level_ac_battery;
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br;
-	int result = 0;
+	int result;
 	u32 value;
 
 	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->handle,
@@ -838,6 +838,8 @@ int acpi_video_get_levels(struct acpi_device *device,
 		goto out_free;
 	}
 
+	max_level = 0;
+	count = 0;
 	for (i = 0; i < obj->package.count; i++) {
 		o = (union acpi_object *)&obj->package.elements[i];
 		if (o->type != ACPI_TYPE_INTEGER) {
@@ -863,6 +865,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 	 * In this case, the first two elements in _BCL packages
 	 * are also supported brightness levels that OS should take care of.
 	 */
+	level_ac_battery = 0;
 	for (i = ACPI_VIDEO_FIRST_LEVEL; i < count; i++) {
 		if (br->levels[i] == br->levels[ACPI_VIDEO_AC_LEVEL])
 			level_ac_battery++;
@@ -895,7 +898,7 @@ int acpi_video_get_levels(struct acpi_device *device,
 	*dev_br = br;
 	if (pmax_level)
 		*pmax_level = max_level;
-
+	result = 0;
 out:
 	kfree(obj);
 	return result;
-- 
2.13.4

[PATCH v2 9/9] ACPI-video: Improve a size determination in four functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 16:45:29 +0200

Replace the specification of a data structures by variable references
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/acpi_video.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 3cc1798676ab..d999f4e49b98 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1140,7 +1140,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	if (ACPI_FAILURE(status))
 		return 0;
 
-	data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -1321,7 +1321,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 			  dod->package.count));
 
 	active_list = kcalloc(1 + dod->package.count,
-			      sizeof(struct acpi_video_enumerated_device),
+			      sizeof(*active_list),
 			      GFP_KERNEL);
 	if (!active_list) {
 		status = -ENOMEM;
@@ -1742,7 +1742,7 @@ static void acpi_video_dev_register_backlight(struct acpi_video_device *device)
 		pci_dev_put(pdev);
 	}
 
-	memset(&props, 0, sizeof(struct backlight_properties));
+	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_FIRMWARE;
 	props.max_brightness =
 		device->brightness->count - ACPI_VIDEO_FIRST_LEVEL - 1;
@@ -2007,7 +2007,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
 			return -ENODEV;
 	}
 
-	video = kzalloc(sizeof(struct acpi_video_bus), GFP_KERNEL);
+	video = kzalloc(sizeof(*video), GFP_KERNEL);
 	if (!video)
 		return -ENOMEM;
 
-- 
2.13.4

[PATCH v2 0/4] ACPI-APEI-HEST: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 9 Aug 2017 19:45:43 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Fix a typo in a comment line
  Use kmalloc_array() in hest_ghes_dev_register()
  Move an assignment in hest_ghes_dev_register()
  Reduce the scope for a variable in acpi_hest_init()

---

v2:
* Desired changes were rebased on source files from Linux next-20170803.

* The number of update steps was reduced.

 drivers/acpi/apei/hest.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.13.4

[PATCH v2 1/4] ACPI-APEI-HEST: Fix a typo in a comment line

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 20:37:38 +0200

Add a missing character to the fourth word at the beginning of this file.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 456b488eb1df..832a930e98f2 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -1,5 +1,5 @@
 /*
- * APEI Hardware Error Souce Table support
+ * APEI Hardware Error Source Table support
  *
  * HEST describes error sources in detail; communicates operational
  * parameters (i.e. severity levels, masking bits, and threshold
-- 
2.13.4

[PATCH v2 2/4] ACPI-APEI-HEST: Use kmalloc_array() in hest_ghes_dev_register()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 20:55:33 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 832a930e98f2..24d1d3cd3371 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -195,7 +195,9 @@ static int __init hest_ghes_dev_register(unsigned int ghes_count)
 	struct ghes_arr ghes_arr;
 
 	ghes_arr.count = 0;
-	ghes_arr.ghes_devs = kmalloc(sizeof(void *) * ghes_count, GFP_KERNEL);
+	ghes_arr.ghes_devs = kmalloc_array(ghes_count,
+					   sizeof(*ghes_arr.ghes_devs),
+					   GFP_KERNEL);
 	if (!ghes_arr.ghes_devs)
 		return -ENOMEM;
 
-- 
2.13.4

[PATCH v2 3/4] ACPI-APEI-HEST: Move an assignment in hest_ghes_dev_register()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:15:22 +0200

Move one assignment for a data structure member in one local variable
so that its setting will only be performed after a corresponding memory
allocation succeeded by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 24d1d3cd3371..94b9bd494c20 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -194,13 +194,13 @@ static int __init hest_ghes_dev_register(unsigned int ghes_count)
 	int rc, i;
 	struct ghes_arr ghes_arr;
 
-	ghes_arr.count = 0;
 	ghes_arr.ghes_devs = kmalloc_array(ghes_count,
 					   sizeof(*ghes_arr.ghes_devs),
 					   GFP_KERNEL);
 	if (!ghes_arr.ghes_devs)
 		return -ENOMEM;
 
+	ghes_arr.count = 0;
 	rc = apei_hest_parse(hest_parse_ghes, &ghes_arr);
 	if (rc)
 		goto err;
-- 
2.13.4

[PATCH v2 4/4] ACPI-APEI-HEST: Reduce the scope for a variable in acpi_hest_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 5 Sep 2016 21:50:34 +0200

Move the definition for the local variable "ghes_count" into an if branch
so that the corresponding setting will only be performed if GHES could be
enabled by this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/acpi/apei/hest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 94b9bd494c20..c0d81387ce18 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -225,7 +225,6 @@ void __init acpi_hest_init(void)
 {
 	acpi_status status;
 	int rc = -ENODEV;
-	unsigned int ghes_count = 0;
 
 	if (hest_disable) {
 		pr_info(HEST_PFX "Table parsing disabled.\n");
@@ -248,6 +247,8 @@ void __init acpi_hest_init(void)
 		goto err;
 
 	if (!ghes_disable) {
+		unsigned int ghes_count = 0;
+
 		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
 		if (rc)
 			goto err;
-- 
2.13.4