fujitsu-laptop: driver [un]registration fixes

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] fujitsu-laptop: driver [un]registration fixes

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

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/platform/x86/fujitsu-laptop.c |   86 ++++++++++++++++------------------
 1 file changed, 41 insertions(+), 45 deletions(-)

Index: b/drivers/platform/x86/fujitsu-laptop.c
===================================================================

drivers/platform/x86/fujitsu-laptop.c +41 -45

@@ -691,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",

@@ -722,22 +722,22 @@ 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 fujitsu_t *fujitsu = acpi_driver_data(device);
+	struct input_dev *input = fujitsu->input;
 
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
+	input_unregister_device(input);
 
-	fujitsu = acpi_driver_data(device);
+	input_free_device(input);
 
 	fujitsu->acpi_handle = NULL;
 

@@ -862,7 +862,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",

@@ -902,7 +902,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);

@@ -925,33 +925,41 @@ 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 fujitsu_hotkey_t *fujitsu_hotkey = acpi_driver_data(device);
+	struct input_dev *input = fujitsu_hotkey->input;
 
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
+#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
 
-	fujitsu_hotkey = acpi_driver_data(device);
+	input_unregister_device(input);
 
-	fujitsu_hotkey->acpi_handle = NULL;
+	input_free_device(input);
 
 	kfifo_free(fujitsu_hotkey->fifo);
 
+	fujitsu_hotkey->acpi_handle = NULL;
+
 	return 0;
 }
 

@@ -1121,8 +1129,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;

@@ -1162,32 +1173,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;

@@ -1195,28 +1196,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");
 }

Quoted 4 lines

drivers/platform/x86/fujitsu-laptop.c +0 -0

@@ -722,22 +722,22 @@ static int acpi_fujitsu_add(struct acpi_
 
 	return result;
 

drivers/platform/x86/fujitsu-laptop.c static int acpi_fujitsu_add(struct acpi_ · line 729

Quoted 6 lines

drivers/platform/x86/fujitsu-laptop.c +2 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Just noticed it scanning ACPI list. You must not use input_free_device() after calling input_unregister_device() since unregister likely drops the last reference to the device and it will get freed by input core.

For polled input devices you need to use both unregister and free though because polled device structure is not refcounted (but underlying input device is).

Hi Dmitry

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines

drivers/platform/x86/fujitsu-laptop.c +0 -0

@@ -722,22 +722,22 @@ static int acpi_fujitsu_add(struct acpi_
 
 	return result;
 

Quoted 10 lines

drivers/platform/x86/fujitsu-laptop.c static int acpi_fujitsu_add(struct acpi_ · line 729

Quoted 6 lines

drivers/platform/x86/fujitsu-laptop.c +2 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Just noticed it scanning ACPI list. You must not use input_free_device() after calling input_unregister_device() since unregister likely drops the last reference to the device and it will get freed by input core.

So what's the correct way to deal with that in this case? Something like

drivers/platform/x86/fujitsu-laptop.c +3 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
+	goto err_stop;
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

(with a short comment to explain the goto) would circumvent the problem but it looks ugly (at least to my eyes - I've never really liked "goto"s :-) ).

Quoted 4 lines

For polled input devices you need to use both unregister and free though because polled device structure is not refcounted (but underlying input device is).

This isn't a polled input device AFAIK so this doesn't apply here, right?

Hi Jonathan,

Quoted 28 lines · Jonathan Woithe

Hi Dmitry

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines

(quoted diff) +0 -0

@@ -722,22 +722,22 @@ static int acpi_fujitsu_add(struct acpi_
 
 	return result;
 

Quoted 10 lines

Review static int acpi_fujitsu_add(struct acpi_ · line 729

Quoted 6 lines

(quoted diff) +2 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Just noticed it scanning ACPI list. You must not use input_free_device() after calling input_unregister_device() since unregister likely drops the last reference to the device and it will get freed by input core.

So what's the correct way to deal with that in this case? Something like

(quoted diff) +3 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
+	goto err_stop;
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Quoted 2 lines

(with a short comment to explain the goto) would circumvent the problem but it looks ugly (at least to my eyes - I've never really liked "goto"s :-) ).

Just do "input = NULL;" after calling input_unregister_device() - input_free_device() is like kfree() and will happily ignore passed NULL pointers.

Or rearrange the code to register device last, when everything is ready.

Hi,

Quoted 38 lines · Dmitry Torokhov

Hi Jonathan,

Quoted 28 lines · Jonathan Woithe

Hi Dmitry

Quoted 5 lines · Bartlomiej Zolnierkiewicz

Quoted 4 lines

(quoted diff) +0 -0

@@ -722,22 +722,22 @@ static int acpi_fujitsu_add(struct acpi_
 
 	return result;
 

Quoted 10 lines

drivers/platform/x86/fujitsu-laptop.c static int acpi_fujitsu_add(struct acpi_ · line 730

Quoted 6 lines

drivers/platform/x86/fujitsu-laptop.c +2 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Just noticed it scanning ACPI list. You must not use input_free_device() after calling input_unregister_device() since unregister likely drops the last reference to the device and it will get freed by input core.

So what's the correct way to deal with that in this case? Something like

(quoted diff) +3 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
+	goto err_stop;
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Quoted 2 lines

(with a short comment to explain the goto) would circumvent the problem but it looks ugly (at least to my eyes - I've never really liked "goto"s :-) ).

Just do "input = NULL;" after calling input_unregister_device() - input_free_device() is like kfree() and will happily ignore passed NULL pointers.

Quoted 1 line

Or rearrange the code to register device last, when everything is ready.

I don't see it fixed in Linus' tree or linux-next yet so here is a patch:
(thanks for noticing the issue and sorry for the delayed reply).

[ Jonathan, please apply. Thanks! ]

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] fujitsu-laptop: fix input_free_device() usage

input_free_device() must not be used after calling input_unregister_device() since unregister likely drops the last reference to the device and it will get freed by input core.

Fix all input_unregister_device()+input_free_device() occurences accordingly.

Noticed-by Dmitry Torokhov <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
against current Linus' tree

 drivers/platform/x86/fujitsu-laptop.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Index: b/drivers/platform/x86/fujitsu-laptop.c
===================================================================

drivers/platform/x86/fujitsu-laptop.c +2 -4

@@ -724,6 +724,7 @@ static int acpi_fujitsu_add(struct acpi_
 
 err_unregister_input_dev:
 	input_unregister_device(input);
+	input = NULL;
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

@@ -737,8 +738,6 @@ static int acpi_fujitsu_remove(struct ac
 
 	input_unregister_device(input);
 
-	input_free_device(input);
-
 	fujitsu->acpi_handle = NULL;
 
 	return 0;

@@ -929,6 +928,7 @@ static int acpi_fujitsu_hotkey_add(struc
 
 err_unregister_input_dev:
 	input_unregister_device(input);
+	input = NULL;
 err_free_input_dev:
 	input_free_device(input);
 err_free_fifo:

@@ -952,8 +952,6 @@ static int acpi_fujitsu_hotkey_remove(st
 
 	input_unregister_device(input);
 
-	input_free_device(input);
-
 	kfifo_free(&fujitsu_hotkey->fifo);
 
 	fujitsu_hotkey->acpi_handle = NULL;

:

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

So what's the correct way to deal with that in this case? Something like

(quoted diff) +3 -1

-end:
+err_unregister_input_dev:
+	input_unregister_device(input);
+	goto err_stop;
 err_free_input_dev:
 	input_free_device(input);
 err_stop:

Quoted 8 lines

Quoted 2 lines

(with a short comment to explain the goto) would circumvent the problem but it looks ugly (at least to my eyes - I've never really liked "goto"s :-) ).

Just do "input = NULL;" after calling input_unregister_device() - input_free_device() is like kfree() and will happily ignore passed NULL pointers.

Or rearrange the code to register device last, when everything is ready.

I don't see it fixed in Linus' tree or linux-next yet so here is a patch:
(thanks for noticing the issue and sorry for the delayed reply).