[PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

[PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

This takes care of the following entries from Dan's list:

drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu'
drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'

Reported-by: Dan Carpenter <error27@gmail.com>
Cc: corbet@lwn.net
Cc: eteo@redhat.com
Cc: Julia Lawall <julia@diku.dk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/platform/x86/fujitsu-laptop.c |    6 ------
 1 file changed, 6 deletions(-)

Index: b/drivers/platform/x86/fujitsu-laptop.c
===================================================================
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -324,9 +324,6 @@ static int set_lcd_level(int level)
 	if (level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
-	if (!fujitsu)
-		return -EINVAL;
-
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n");
@@ -355,9 +352,6 @@ static int set_lcd_level_alt(int level)
 	if (level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
-	if (!fujitsu)
-		return -EINVAL;
-
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
 	if (ACPI_FAILURE(status)) {
 		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n");

Re: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

[Jonathan Woithe]

Hi Bart

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks
> 
> This takes care of the following entries from Dan's list:
> 
> drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu'
> drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'

I'd rather keep the test for a non-null fujitsu in there, but obviously it's
kind of pointless doing it after the first dereference.  Since this fixup
overlaps with the one previously discussed with Julia I've taken the liberty
of consolidating these - I'll send the result to the list as a separate
email.

Regards
  jonathan

> Reported-by: Dan Carpenter <error27@gmail.com>
> Cc: corbet@lwn.net
> Cc: eteo@redhat.com
> Cc: Julia Lawall <julia@diku.dk>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/platform/x86/fujitsu-laptop.c |    6 ------
>  1 file changed, 6 deletions(-)
> 
> Index: b/drivers/platform/x86/fujitsu-laptop.c
> ===================================================================
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -324,9 +324,6 @@ static int set_lcd_level(int level)
>  	if (level < 0 || level >= fujitsu->max_brightness)
>  		return -EINVAL;
>  
> -	if (!fujitsu)
> -		return -EINVAL;
> -
>  	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
>  	if (ACPI_FAILURE(status)) {
>  		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n");
> @@ -355,9 +352,6 @@ static int set_lcd_level_alt(int level)
>  	if (level < 0 || level >= fujitsu->max_brightness)
>  		return -EINVAL;
>  
> -	if (!fujitsu)
> -		return -EINVAL;
> -
>  	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
>  	if (ACPI_FAILURE(status)) {
>  		vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n");

Re: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 30 July 2009 09:43:49 Jonathan Woithe wrote:
> Hi Bart
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks
> > 
> > This takes care of the following entries from Dan's list:
> > 
> > drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu'
> > drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'
> 
> I'd rather keep the test for a non-null fujitsu in there, but obviously it's
> kind of pointless doing it after the first dereference.  Since this fixup
> overlaps with the one previously discussed with Julia I've taken the liberty

They are on top of Julia's patch (sorry I forgot to mention it)..

Re: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks

[Jonathan Woithe]

Hi

> > > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > > Subject: [PATCH] fujitsu-laptop: remove superfluous NULL pointer checks
> > > 
> > > This takes care of the following entries from Dan's list:
> > > 
> > > drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning: variable derefenced before check 'fujitsu'
> > > drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning: variable derefenced before check 'fujitsu'
> > 
> > I'd rather keep the test for a non-null fujitsu in there, but obviously it's
> > kind of pointless doing it after the first dereference.  Since this fixup
> > overlaps with the one previously discussed with Julia I've taken the liberty
> 
> They are on top of Julia's patch (sorry I forgot to mention it)..

Ah, right.  I was trying to work out where it fitted (that was probably the
one preceeding patch I *didn't* end up trying).

Regards
  jonathan

Re: [PATCH] fujitsu-laptop: consolidated fixes (NULL pointer checks, [un]registration fixes, etc)

[Jonathan Woithe]

Hi all

This patch consolidates the two patches posted by Bartlomiej Zolnierkiewicz
and the NULL pointer patch which came out of a discussion with Julia Lawall.
Some additional NULL pointer check tidy-ups have also been done.

To summarise:

* Take care of the following entries from Dan's list:

  drivers/platform/x86/fujitsu-laptop.c +327 set_lcd_level(13) warning:       
    variable derefenced before check 'fujitsu'
  drivers/platform/x86/fujitsu-laptop.c +358 set_lcd_level_alt(13) warning:   
    variable derefenced before check 'fujitsu'

* Move led_classdev_unregister() calls from fujitsu_cleanup() to
  acpi_fujitsu_hotkey_remove().

* Fix ordering in fujitsu_cleanup().

* Fix backlight_device_register() failure handling in fujitsu_init().

* Add missing sysfs group removal on failure to fujitsu_init().

* Add input device unregistering on failure to acpi_fujitsu_add()
  and acpi_fujitsu_hotkey_add().

* Add input device unregistering/freeing to acpi_fujitsu_remove()   
  and acpi_fujitsu_hotkey_remove() (also remove superfluous 'device'
  and 'acpi_driver_data(device)' checks while at it).

* Do few minor cleanups.

* Fix some additional NULL pointer checks which should occur before the 
  initial dereference.

Patch is relative to linux-acpi-2.6.git head.


Reported-by: Dan Carpenter <error27@gmail.com>
Cc: corbet@lwn.net
Cc: eteo@redhat.com
Cc: Julia Lawall <julia@diku.dk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Signed-off-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>

--- a/drivers/platform/x86/fujitsu-laptop.c	2009-07-30 16:30:55.455795000 +0930
+++ b/drivers/platform/x86/fujitsu-laptop.c	2009-07-30 16:48:45.542784340 +0930
@@ -70,7 +70,7 @@
 #include <linux/leds.h>
 #endif
 
-#define FUJITSU_DRIVER_VERSION "0.5.0"
+#define FUJITSU_DRIVER_VERSION "0.6.0"
 
 #define FUJITSU_LCD_N_LEVELS 8
 
@@ -321,10 +321,7 @@ static int set_lcd_level(int level)
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
-		return -EINVAL;
-
-	if (!fujitsu)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
@@ -352,10 +349,7 @@ static int set_lcd_level_alt(int level)
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
-		return -EINVAL;
-
-	if (!fujitsu)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
@@ -697,7 +691,7 @@ static int acpi_fujitsu_add(struct acpi_
 	result = acpi_bus_get_power(fujitsu->acpi_handle, &state);
 	if (result) {
 		printk(KERN_ERR "Error reading power state\n");
-		goto end;
+		goto err_unregister_input_dev;
 	}
 
 	printk(KERN_INFO PREFIX "%s [%s] (%s)\n",
@@ -728,25 +722,31 @@ static int acpi_fujitsu_add(struct acpi_
 
 	return result;
 
-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_stop:
-
 	return result;
 }
 
 static int acpi_fujitsu_remove(struct acpi_device *device, int type)
 {
 	struct fujitsu_t *fujitsu = NULL;
+	struct input_dev *input = NULL;
 
-	if (!device || !acpi_driver_data(device))
+	if (!device)
 		return -EINVAL;
 
-	fujitsu = acpi_driver_data(device);
-
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
+        fujitsu = acpi_driver_data(device);
+        if (!fujitsu)
+        	return -EINVAL;
+        input = fujitsu->input;
+
+        if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	fujitsu->acpi_handle = NULL;
 
@@ -871,7 +871,7 @@ static int acpi_fujitsu_hotkey_add(struc
 	result = acpi_bus_get_power(fujitsu_hotkey->acpi_handle, &state);
 	if (result) {
 		printk(KERN_ERR "Error reading power state\n");
-		goto end;
+		goto err_unregister_input_dev;
 	}
 
 	printk(KERN_INFO PREFIX "%s [%s] (%s)\n",
@@ -911,7 +911,7 @@ static int acpi_fujitsu_hotkey_add(struc
 	printk(KERN_INFO "fujitsu-laptop: BTNI: [0x%x]\n",
 		call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0));
 
-	#ifdef CONFIG_LEDS_CLASS
+#ifdef CONFIG_LEDS_CLASS
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
 		result = led_classdev_register(&fujitsu->pf_device->dev,
 						&logolamp_led);
@@ -934,33 +934,50 @@ static int acpi_fujitsu_hotkey_add(struc
 			"LED handler for keyboard lamps, error %i\n", result);
 		}
 	}
-	#endif
+#endif
 
 	return result;
 
-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_free_fifo:
 	kfifo_free(fujitsu_hotkey->fifo);
 err_stop:
-
 	return result;
 }
 
 static int acpi_fujitsu_hotkey_remove(struct acpi_device *device, int type)
 {
 	struct fujitsu_hotkey_t *fujitsu_hotkey = NULL;
+	struct input_dev *input = NULL;
 
-	if (!device || !acpi_driver_data(device))
+	if (!device)
 		return -EINVAL;
-
 	fujitsu_hotkey = acpi_driver_data(device);
+	if (!fujitsu_hotkey)
+		return -EINVAL;
 
-	fujitsu_hotkey->acpi_handle = NULL;
+	input = fujitsu_hotkey->input;
+
+#ifdef CONFIG_LEDS_CLASS
+	if (fujitsu_hotkey->logolamp_registered)
+		led_classdev_unregister(&logolamp_led);
+
+	if (fujitsu_hotkey->kblamps_registered)
+		led_classdev_unregister(&kblamps_led);
+#endif
+
+	if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	kfifo_free(fujitsu_hotkey->fifo);
 
+	fujitsu_hotkey->acpi_handle = NULL;
+
 	return 0;
 }
 
@@ -1130,8 +1147,11 @@ static int __init fujitsu_init(void)
 		fujitsu->bl_device =
 			backlight_device_register("fujitsu-laptop", NULL, NULL,
 						  &fujitsubl_ops);
-		if (IS_ERR(fujitsu->bl_device))
-			return PTR_ERR(fujitsu->bl_device);
+		if (IS_ERR(fujitsu->bl_device)) {
+			ret = PTR_ERR(fujitsu->bl_device);
+			fujitsu->bl_device = NULL;
+			goto fail_sysfs_group;
+		}
 		max_brightness = fujitsu->max_brightness;
 		fujitsu->bl_device->props.max_brightness = max_brightness - 1;
 		fujitsu->bl_device->props.brightness = fujitsu->brightness_level;
@@ -1171,32 +1191,22 @@ static int __init fujitsu_init(void)
 	return 0;
 
 fail_hotkey1:
-
 	kfree(fujitsu_hotkey);
-
 fail_hotkey:
-
 	platform_driver_unregister(&fujitsupf_driver);
-
 fail_backlight:
-
 	if (fujitsu->bl_device)
 		backlight_device_unregister(fujitsu->bl_device);
-
+fail_sysfs_group:
+	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
+			   &fujitsupf_attribute_group);
 fail_platform_device2:
-
 	platform_device_del(fujitsu->pf_device);
-
 fail_platform_device1:
-
 	platform_device_put(fujitsu->pf_device);
-
 fail_platform_driver:
-
 	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
-
 fail_acpi:
-
 	kfree(fujitsu);
 
 	return ret;
@@ -1204,28 +1214,23 @@ fail_acpi:
 
 static void __exit fujitsu_cleanup(void)
 {
-	#ifdef CONFIG_LEDS_CLASS
-	if (fujitsu_hotkey->logolamp_registered != 0)
-		led_classdev_unregister(&logolamp_led);
+	acpi_bus_unregister_driver(&acpi_fujitsu_hotkey_driver);
 
-	if (fujitsu_hotkey->kblamps_registered != 0)
-		led_classdev_unregister(&kblamps_led);
-	#endif
+	kfree(fujitsu_hotkey);
 
-	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
-			   &fujitsupf_attribute_group);
-	platform_device_unregister(fujitsu->pf_device);
 	platform_driver_unregister(&fujitsupf_driver);
+
 	if (fujitsu->bl_device)
 		backlight_device_unregister(fujitsu->bl_device);
 
-	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
+	sysfs_remove_group(&fujitsu->pf_device->dev.kobj,
+			   &fujitsupf_attribute_group);
 
-	kfree(fujitsu);
+	platform_device_unregister(fujitsu->pf_device);
 
-	acpi_bus_unregister_driver(&acpi_fujitsu_hotkey_driver);
+	acpi_bus_unregister_driver(&acpi_fujitsu_driver);
 
-	kfree(fujitsu_hotkey);
+	kfree(fujitsu);
 
 	printk(KERN_INFO "fujitsu-laptop: driver unloaded.\n");
 }

Re: [PATCH] fujitsu-laptop: consolidated fixes (NULL pointer checks, [un]registration fixes, etc)

[Bartlomiej Zolnierkiewicz]

On Thursday 30 July 2009 09:55:22 Jonathan Woithe wrote:
> Hi all
> 
> This patch consolidates the two patches posted by Bartlomiej Zolnierkiewicz
> and the NULL pointer patch which came out of a discussion with Julia Lawall.
> Some additional NULL pointer check tidy-ups have also been done.
> 
> To summarise:

I think that for the increased readability and to preserve proper credits it
would be better to keep separate patches (updating the driver version can be
as well handled in an incremental patch).

I would also strongly insist on removing the following needless NULL pointer
checks introduced by this version.  They may hide real bugs in the ACPI driver
code (which should make sure that we always have valid 'device' in the ACPI
driver methods and that it doesn't go away while the method executes) or in
the fujitsu-laptop itself (which has to always provide valid 'fujitsu_hotkey',
'fujitsu' and 'input' pointers and not free them while there are still some
users left).

If this is the case we should fix the underlying problems and not hide them
with the "defensive coding".  While it could (sometimes) help in avoiding
the underlying issue it may also easily result in the whole new bag of issues
resulting from introducing the unexpected code paths and system states, i.e.
ACPI driver code assumes that the ACPI device is no longer used/referenced by
higher-layers (like input layer) and unregistered from the driver after
->remove method finishes -- this assumption will no longer be true in case of
premature return caused by 'device == NULL' check if 'device' is NULL (it will
never be NULL actually, this is just an example to explain the problem with
the "defensive coding" principle).

I understand the sentiment for the "defensive coding" methods (often caused by
the real need for them when working on components for the closed code software)
but they should be avoided in the open source software.

Thanks.

diff -u b/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
--- b/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c	2009-07-30 16:48:45.542784340 +0930
@@ -70,7 +70,7 @@
 #include <linux/leds.h>
 #endif
 
-#define FUJITSU_DRIVER_VERSION "0.5.0"
+#define FUJITSU_DRIVER_VERSION "0.6.0"
 
 #define FUJITSU_LCD_N_LEVELS 8
 
@@ -321,7 +321,7 @@ 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBLL", &handle);
@@ -349,7 +349,7 @@
 	vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n",
 		    level);
 
-	if (level < 0 || level >= fujitsu->max_brightness)
+	if (!fujitsu || level < 0 || level >= fujitsu->max_brightness)
 		return -EINVAL;
 
 	status = acpi_get_handle(fujitsu->acpi_handle, "SBL2", &handle);
@@ -732,12 +732,21 @@
 
 static int acpi_fujitsu_remove(struct acpi_device *device, int type)
 {
-	struct fujitsu_t *fujitsu = acpi_driver_data(device);
-	struct input_dev *input = fujitsu->input;
+	struct fujitsu_t *fujitsu = NULL;
+	struct input_dev *input = NULL;
 
-	input_unregister_device(input);
+	if (!device)
+		return -EINVAL;
 
-	input_free_device(input);
+        fujitsu = acpi_driver_data(device);
+        if (!fujitsu)
+        	return -EINVAL;
+        input = fujitsu->input;
+
+        if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	fujitsu->acpi_handle = NULL;
 
@@ -941,8 +950,16 @@
 
 static int acpi_fujitsu_hotkey_remove(struct acpi_device *device, int type)
 {
-	struct fujitsu_hotkey_t *fujitsu_hotkey = acpi_driver_data(device);
-	struct input_dev *input = fujitsu_hotkey->input;
+	struct fujitsu_hotkey_t *fujitsu_hotkey = NULL;
+	struct input_dev *input = NULL;
+
+	if (!device)
+		return -EINVAL;
+	fujitsu_hotkey = acpi_driver_data(device);
+	if (!fujitsu_hotkey)
+		return -EINVAL;
+
+	input = fujitsu_hotkey->input;
 
 #ifdef CONFIG_LEDS_CLASS
 	if (fujitsu_hotkey->logolamp_registered)
@@ -952,9 +969,10 @@
 		led_classdev_unregister(&kblamps_led);
 #endif
 
-	input_unregister_device(input);
-
-	input_free_device(input);
+	if (input) {
+		input_unregister_device(input);
+		input_free_device(input);
+	}
 
 	kfifo_free(fujitsu_hotkey->fifo);
 

Re: [PATCH] fujitsu-laptop: consolidated fixes (NULL pointer checks, [un]registration fixes, etc)

[Jonathan Woithe]

Hi all

> > This patch consolidates the two patches posted by Bartlomiej Zolnierkiewicz
> > and the NULL pointer patch which came out of a discussion with Julia Lawall.
> > Some additional NULL pointer check tidy-ups have also been done.
> > 
> > To summarise:
> 
> I think that for the increased readability and to preserve proper credits it
> would be better to keep separate patches (updating the driver version can be
> as well handled in an incremental patch).

Ok, leave it with me and I'll re-split along these lines.

> I would also strongly insist on removing the following needless NULL pointer
> checks introduced by this version.  They may hide real bugs in the ACPI driver
> code (which should make sure that we always have valid 'device' in the ACPI
> driver methods and that it doesn't go away while the method executes) or in
> the fujitsu-laptop itself (which has to always provide valid 'fujitsu_hotkey',
> 'fujitsu' and 'input' pointers and not free them while there are still some
> users left).

I'll take another look at these.  What I was concerned about was whether the
hotkey pointer in particular was valid in all cases (since some of the
relevant laptops don't in fact have hotkeys) - and I was very short of time
yesterday.  I've had a very quick look through the code just now and it
seems that this may not be a problem after all.  In any case I'll confirm
this over the next day or so and redo/reisssue the patches accordingly.

Regards
  jonathan