[PATCH] fujitsu-laptop: driver [un]registration fixes

[PATCH] fujitsu-laptop: driver [un]registration fixes

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
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 <bzolnier@gmail.com>
---
 drivers/platform/x86/fujitsu-laptop.c |   86 ++++++++++++++++------------------
 1 file changed, 41 insertions(+), 45 deletions(-)

Index: b/drivers/platform/x86/fujitsu-laptop.c
===================================================================
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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");
 }

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Jonathan Woithe]

Hi Bartlomiej

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 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.

Thanks for these - most of these look ok and a few fix some embarrassing
omissions, but a couple of them clash with the other patches being discussed
in other threads.  A consolidated patch will follow in a separate thread for
further discussion.

Regards
  jonathan


> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/platform/x86/fujitsu-laptop.c |   86 ++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 45 deletions(-)
> 
> Index: b/drivers/platform/x86/fujitsu-laptop.c
> ===================================================================
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -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");
>  }

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Dmitry Torokhov]

Hi Bartlomiej,

On Wed, Jul 29, 2009 at 10:15:33PM +0200, Bartlomiej Zolnierkiewicz wrote:
> @@ -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:

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

Thanks.

-- 
Dmitry

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Jonathan Woithe]

Hi Dmitry

> On Wed, Jul 29, 2009 at 10:15:33PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > @@ -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:
> 
> 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

-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 :-) ).

> 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?

Regards
  jonathan

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Dmitry Torokhov]

Hi Jonathan,

On Monday 21 December 2009 02:32:40 pm Jonathan Woithe wrote:
> Hi Dmitry
> 
> > On Wed, Jul 29, 2009 at 10:15:33PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > @@ -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:
> >
> > 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
> 
> -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 :-) ).

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.

> 
> > 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?

Right, it was more of a general statement so I don't get bunch of patches 
removing input_free_polled_device() after calls to
input_unregister_polled_device() ;)

-- 
Dmitry

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 21 December 2009 11:46:32 pm Dmitry Torokhov wrote:
> Hi Jonathan,
> 
> On Monday 21 December 2009 02:32:40 pm Jonathan Woithe wrote:
> > Hi Dmitry
> > 
> > > On Wed, Jul 29, 2009 at 10:15:33PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > @@ -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:
> > >
> > > 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
> > 
> > -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 :-) ).
> 
> 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).

[ Jonathan, please apply.  Thanks! ]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
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 <dmitry.torokhov@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
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
===================================================================
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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;

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Dmitry Torokhov]

On Tue, Jan 05, 2010 at 05:46:46PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday 21 December 2009 11:46:32 pm Dmitry Torokhov wrote:
> > Hi Jonathan,
> > 
> > On Monday 21 December 2009 02:32:40 pm Jonathan Woithe wrote:
> > > Hi Dmitry
> > > 
> > > > On Wed, Jul 29, 2009 at 10:15:33PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > > @@ -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:
> > > >
> > > > 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
> > > 
> > > -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 :-) ).
> > 
> > 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).
> 

Yep, the patch looks good.

> [ Jonathan, please apply.  Thanks! ]
> 
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 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 <dmitry.torokhov@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> 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
> ===================================================================
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -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;
> 

-- 
Dmitry

Re: [PATCH] fujitsu-laptop: driver [un]registration fixes

[Jonathan Woithe]

Hi

> On Tue, Jan 05, 2010 at 05:46:46PM +0100, Bartlomiej Zolnierkiewicz wrote:
> :
> > > > So what's the correct way to deal with that in this case?  Something like
> > > > 
> > > > -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 :-) ).
> > > 
> > > 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).

Yeah, sorry about that.  I was going to do up a patch but got caught up on
other things over the past two weeks.  It got close to the top of my TODO
list yesterday, but it seems you beat me to it. :-)  Thanks.

Acked-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>

Len: please apply.

Regards
  jonathan


From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
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 <dmitry.torokhov@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Acked-by: Jonathan Woithe <jwoithe@physics.adelaide.edu.au>
---
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
===================================================================
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -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;