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
===================================================================
--- 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");
}
Hi Bartlomiej
> 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.
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 <[email protected]>
> ---
> 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");
> }
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
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
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
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 <[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
===================================================================
--- 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;
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 <[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
> ===================================================================
> --- 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
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 <[email protected]>
Len: please apply.
Regards
jonathan
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]>
Acked-by: Jonathan Woithe <[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
===================================================================
--- 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;