[2/4,RFC,v2] fujitsu-laptop: fujitsu-laptop: restructure initialisation

Commit Message

Jonathan Woithe Jan. 31, 2017, 1:09 p.m. UTC

Don't register backlight and platform devices on non-fujitsu hardware
 - bail out instead.

Register the backlight driver _after_ the main acpi driver.  The
backlight device needs the main device in order to get and set its power
state. This also allows us to move the keycodes and platform device from
"struct fujitsu_bl" to the main "struct fujitsu".

Similarly, the platform device interface is now exported to userspace
_after_ the acpi devices have been initialized.  This closes a gap
during initialization where userspace would read undefined values,
and where writes from userspace might provoke undefined behaviour.

While we're at it, make sure the platform driver is registered before
the platform device.

Also replace kmalloc() + memset() with kzalloc().

Signed-off-by: Alan Jenkins <[e-mail]>
Signed-off-by: Jonathan Woithe <[e-mail]>
---
 drivers/platform/x86/fujitsu-laptop.c | 222 +++++++++++++++-------------------
 1 file changed, 98 insertions(+), 124 deletions(-)

Comments

Michał Kępień Feb. 3, 2017, 2:46 p.m. UTC | #1

> Don't register backlight and platform devices on non-fujitsu hardware
>  - bail out instead.
> 
> Register the backlight driver _after_ the main acpi driver.

Something went wrong (during the rebase?) as after this patch is
applied, acpi_bus_register_driver() is never called for
acpi_fujitsu_bl_driver, rendering the latter useless.

Jonathan Woithe Feb. 3, 2017, 11:30 p.m. UTC | #2

On Fri, Feb 03, 2017 at 03:46:19PM +0100, Micha?? K??pie?? wrote:
> > Don't register backlight and platform devices on non-fujitsu hardware
> >  - bail out instead.
> > 
> > Register the backlight driver _after_ the main acpi driver.
> 
> Something went wrong (during the rebase?) as after this patch is
> applied, acpi_bus_register_driver() is never called for
> acpi_fujitsu_bl_driver, rendering the latter useless.

Obviously that needs to be addressed.  Did you want me to look into it?

After looking at the original patches it appears that contrary to my
original assumptions, I don't thimk they were CCed to the list.  Would it
help you to see the originals?  It is probably not good to post them to the
list so as to avoid confusion, but I could forward them to you if you think
they might be of use.

Regards
  jonathan

PS: thanks for the git primer you sent the other day.

Michał Kępień Feb. 4, 2017, 6:26 a.m. UTC | #3

> On Fri, Feb 03, 2017 at 03:46:19PM +0100, Micha?? K??pie?? wrote:
> > > Don't register backlight and platform devices on non-fujitsu hardware
> > >  - bail out instead.
> > > 
> > > Register the backlight driver _after_ the main acpi driver.
> > 
> > Something went wrong (during the rebase?) as after this patch is
> > applied, acpi_bus_register_driver() is never called for
> > acpi_fujitsu_bl_driver, rendering the latter useless.
> 
> Obviously that needs to be addressed.  Did you want me to look into it?
> 
> After looking at the original patches it appears that contrary to my
> original assumptions, I don't thimk they were CCed to the list.  Would it
> help you to see the originals?  It is probably not good to post them to the
> list so as to avoid confusion, but I could forward them to you if you think
> they might be of use.

Yes, please send them to me off-list, in their original form.  That way
I will not have to assume anything and I should be able to tell what
Alan's intention was regarding driver registration order.  Thanks!

> PS: thanks for the git primer you sent the other day.

You are welcome, I hope it was useful.

Patch

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index a3752bb..c410047 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -146,9 +146,6 @@  struct fujitsu_bl {
 	struct input_dev *input;
 	char phys[32];
 	struct backlight_device *bl_device;
-	struct platform_device *pf_device;
-	int keycode1, keycode2, keycode3, keycode4, keycode5;
-
 	unsigned int max_brightness;
 	unsigned int brightness_changed;
 	unsigned int brightness_level;
@@ -167,7 +164,8 @@  struct fujitsu_laptop {
 	struct platform_device *pf_device;
 	struct kfifo fifo;
 	spinlock_t fifo_lock;
-        int flags_supported;
+	int keycode1, keycode2, keycode3, keycode4, keycode5;
+	int flags_supported;
         int flags_state;
 	int logolamp_registered;
 	int kblamps_registered;
@@ -634,25 +632,25 @@  static void __init dmi_check_cb_common(const struct dmi_system_id *id)
 static int __init dmi_check_cb_s6410(const struct dmi_system_id *id)
 {
 	dmi_check_cb_common(id);
-	fujitsu_bl->keycode1 = KEY_SCREENLOCK;	/* "Lock" */
-	fujitsu_bl->keycode2 = KEY_HELP;	/* "Mobility Center" */
+	fujitsu_laptop->keycode1 = KEY_SCREENLOCK;	/* "Lock" */
+	fujitsu_laptop->keycode2 = KEY_HELP;		/* "Mobility Center" */
 	return 1;
 }
 
 static int __init dmi_check_cb_s6420(const struct dmi_system_id *id)
 {
 	dmi_check_cb_common(id);
-	fujitsu_bl->keycode1 = KEY_SCREENLOCK;	/* "Lock" */
-	fujitsu_bl->keycode2 = KEY_HELP;	/* "Mobility Center" */
+	fujitsu_laptop->keycode1 = KEY_SCREENLOCK;	/* "Lock" */
+	fujitsu_laptop->keycode2 = KEY_HELP;		/* "Mobility Center" */
 	return 1;
 }
 
 static int __init dmi_check_cb_p8010(const struct dmi_system_id *id)
 {
 	dmi_check_cb_common(id);
-	fujitsu_bl->keycode1 = KEY_HELP;	/* "Support" */
-	fujitsu_bl->keycode3 = KEY_SWITCHVIDEOMODE;	/* "Presentation" */
-	fujitsu_bl->keycode4 = KEY_WWW;		/* "Internet" */
+	fujitsu_laptop->keycode1 = KEY_HELP;		/* "Support" */
+	fujitsu_laptop->keycode3 = KEY_SWITCHVIDEOMODE;	/* "Presentation" */
+	fujitsu_laptop->keycode4 = KEY_WWW;		/* "Internet" */
 	return 1;
 }
 
@@ -685,6 +683,7 @@  static const struct dmi_system_id fujitsu_laptop_dmi_table[] __initconst = {
 
 static int acpi_fujitsu_bl_add(struct acpi_device *device)
 {
+	int result = 0;
 	int state = 0;
 	struct input_dev *input;
 	int error;
@@ -692,6 +691,9 @@  static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	fujitsu_bl = kzalloc(sizeof(struct fujitsu_bl), GFP_KERNEL);
+	if (!fujitsu_bl)
+		return -ENOMEM;
 	fujitsu_bl->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s", ACPI_FUJITSU_BL_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
@@ -700,7 +702,7 @@  static int acpi_fujitsu_bl_add(struct acpi_device *device)
 	fujitsu_bl->input = input = input_allocate_device();
 	if (!input) {
 		error = -ENOMEM;
-		goto err_stop;
+		goto err_free;
 	}
 
 	snprintf(fujitsu_bl->phys, sizeof(fujitsu_bl->phys),
@@ -751,14 +753,35 @@  static int acpi_fujitsu_bl_add(struct acpi_device *device)
 		fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS;
 	get_lcd_level();
 
-	return 0;
+	/* Register backlight stuff */
+
+	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
+		struct backlight_properties props;
+
+		memset(&props, 0, sizeof(struct backlight_properties));
+		props.type = BACKLIGHT_PLATFORM;
+		props.max_brightness = fujitsu_bl->max_brightness - 1;
+		fujitsu_bl->bl_device = backlight_device_register("fujitsu-laptop",
+							         NULL, NULL,
+							         &fujitsubl_ops,
+							         &props);
+		if (IS_ERR(fujitsu_bl->bl_device)) {
+			result = PTR_ERR(fujitsu_bl->bl_device);
+			fujitsu_bl->bl_device = NULL;
+			goto err_unregister_input_dev;
+		}
+		fujitsu_bl->bl_device->props.brightness = fujitsu_bl->brightness_level;
+	}
+
+	return result;
 
 err_unregister_input_dev:
 	input_unregister_device(input);
 	input = NULL;
 err_free_input_dev:
 	input_free_device(input);
-err_stop:
+err_free:
+	kfree(fujitsu_bl);
 	return error;
 }
 
@@ -767,9 +790,13 @@  static int acpi_fujitsu_bl_remove(struct acpi_device *device)
 	struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device);
 	struct input_dev *input = fujitsu_bl->input;
 
+	if (fujitsu_bl->bl_device)
+		backlight_device_unregister(fujitsu_bl->bl_device);
+
 	input_unregister_device(input);
 
-	fujitsu_bl->acpi_handle = NULL;
+	kfree(fujitsu_bl);
+	fujitsu_bl = NULL;
 
 	return 0;
 }
@@ -841,19 +868,28 @@  static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
+	fujitsu_laptop = kzalloc(sizeof(struct fujitsu_laptop), GFP_KERNEL);
+	if (!fujitsu_laptop)
+		return -ENOMEM;
 	fujitsu_laptop->acpi_handle = device->handle;
 	sprintf(acpi_device_name(device), "%s",
 		ACPI_FUJITSU_LAPTOP_DEVICE_NAME);
 	sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
 	device->driver_data = fujitsu_laptop;
 
+	fujitsu_laptop->keycode1 = KEY_PROG1;
+	fujitsu_laptop->keycode2 = KEY_PROG2;
+	fujitsu_laptop->keycode3 = KEY_PROG3;
+	fujitsu_laptop->keycode4 = KEY_PROG4;
+	dmi_check_system(fujitsu_laptop_dmi_table);
+
 	/* kfifo */
 	spin_lock_init(&fujitsu_laptop->fifo_lock);
 	error = kfifo_alloc(&fujitsu_laptop->fifo, RINGBUFFERSIZE * sizeof(int),
 			GFP_KERNEL);
 	if (error) {
 		pr_err("kfifo_alloc failed\n");
-		goto err_stop;
+		goto err_free;
 	}
 
 	fujitsu_laptop->input = input = input_allocate_device();
@@ -872,11 +908,11 @@  static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	input->dev.parent = &device->dev;
 
 	set_bit(EV_KEY, input->evbit);
-	set_bit(fujitsu_bl->keycode1, input->keybit);
-	set_bit(fujitsu_bl->keycode2, input->keybit);
-	set_bit(fujitsu_bl->keycode3, input->keybit);
-	set_bit(fujitsu_bl->keycode4, input->keybit);
-	set_bit(fujitsu_bl->keycode5, input->keybit);
+	set_bit(fujitsu_laptop->keycode1, input->keybit);
+	set_bit(fujitsu_laptop->keycode2, input->keybit);
+	set_bit(fujitsu_laptop->keycode3, input->keybit);
+	set_bit(fujitsu_laptop->keycode4, input->keybit);
+	set_bit(fujitsu_laptop->keycode5, input->keybit);
 	set_bit(KEY_TOUCHPAD_TOGGLE, input->keybit);
 	set_bit(KEY_UNKNOWN, input->keybit);
 
@@ -927,7 +963,7 @@  static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 #if IS_ENABLED(CONFIG_LEDS_CLASS)
 	if (call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & LOGOLAMP_POWERON) {
-		result = led_classdev_register(&fujitsu_bl->pf_device->dev,
+		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
 						&logolamp_led);
 		if (result == 0) {
 			fujitsu_laptop->logolamp_registered = 1;
@@ -939,7 +975,7 @@  static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 
 	if ((call_fext_func(FUNC_LEDS, 0x0, 0x0, 0x0) & KEYBOARD_LAMPS) &&
 	   (call_fext_func(FUNC_BUTTONS, 0x0, 0x0, 0x0) == 0x0)) {
-		result = led_classdev_register(&fujitsu_bl->pf_device->dev,
+		result = led_classdev_register(&fujitsu_laptop->pf_device->dev,
 						&kblamps_led);
 		if (result == 0) {
 			fujitsu_laptop->kblamps_registered = 1;
@@ -993,7 +1029,8 @@  static int acpi_fujitsu_laptop_add(struct acpi_device *device)
 	input_free_device(input);
 err_free_fifo:
 	kfifo_free(&fujitsu_laptop->fifo);
-err_stop:
+err_free:
+	kfree(fujitsu_laptop);
 	return error;
 }
 
@@ -1020,7 +1057,8 @@  static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 
 	kfifo_free(&fujitsu_laptop->fifo);
 
-	fujitsu_laptop->acpi_handle = NULL;
+	kfree(fujitsu_laptop);
+	fujitsu_laptop = NULL;
 
 	return 0;
 }
@@ -1046,19 +1084,19 @@  static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
 				&& (i++) < MAX_HOTKEY_RINGBUFFER_SIZE) {
 			switch (irb & 0x4ff) {
 			case KEY1_CODE:
-				keycode = fujitsu_bl->keycode1;
+				keycode = fujitsu_laptop->keycode1;
 				break;
 			case KEY2_CODE:
-				keycode = fujitsu_bl->keycode2;
+				keycode = fujitsu_laptop->keycode2;
 				break;
 			case KEY3_CODE:
-				keycode = fujitsu_bl->keycode3;
+				keycode = fujitsu_laptop->keycode3;
 				break;
 			case KEY4_CODE:
-				keycode = fujitsu_bl->keycode4;
+				keycode = fujitsu_laptop->keycode4;
 				break;
 			case KEY5_CODE:
-				keycode = fujitsu_bl->keycode5;
+				keycode = fujitsu_laptop->keycode5;
 				break;
 			case 0:
 				keycode = 0;
@@ -1171,134 +1209,70 @@  MODULE_DEVICE_TABLE(acpi, fujitsu_ids);
 
 static int __init fujitsu_init(void)
 {
-	int ret, result;
+	int ret;
 
 	if (acpi_disabled)
 		return -ENODEV;
 
-	fujitsu_bl = kzalloc(sizeof(struct fujitsu_bl), GFP_KERNEL);
-	if (!fujitsu_bl)
-		return -ENOMEM;
-	fujitsu_bl->keycode1 = KEY_PROG1;
-	fujitsu_bl->keycode2 = KEY_PROG2;
-	fujitsu_bl->keycode3 = KEY_PROG3;
-	fujitsu_bl->keycode4 = KEY_PROG4;
-	fujitsu_bl->keycode5 = KEY_RFKILL;
-	dmi_check_system(fujitsu_laptop_dmi_table);
-
-	result = acpi_bus_register_driver(&acpi_fujitsu_bl_driver);
-	if (result < 0) {
+	ret = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
+	if (ret < 0)
+		goto err_stop;
+	if (!fujitsu_laptop) {
 		ret = -ENODEV;
-		goto fail_acpi;
+		goto err_unregister_acpi1;
 	}
 
 	/* Register platform stuff */
 
-	fujitsu_bl->pf_device = platform_device_alloc("fujitsu-laptop", -1);
-	if (!fujitsu_bl->pf_device) {
-		ret = -ENOMEM;
-		goto fail_platform_driver;
-	}
-
-	ret = platform_device_add(fujitsu_bl->pf_device);
-	if (ret)
-		goto fail_platform_device1;
-
-	ret =
-	    sysfs_create_group(&fujitsu_bl->pf_device->dev.kobj,
-			       &fujitsu_pf_attribute_group);
-	if (ret)
-		goto fail_platform_device2;
-
-	/* Register backlight stuff */
-
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		struct backlight_properties props;
-
-		memset(&props, 0, sizeof(struct backlight_properties));
-		props.type = BACKLIGHT_PLATFORM;
-		props.max_brightness = fujitsu_bl->max_brightness - 1;
-		fujitsu_bl->bl_device = backlight_device_register("fujitsu-laptop",
-							         NULL, NULL,
-							         &fujitsubl_ops,
-							         &props);
-		if (IS_ERR(fujitsu_bl->bl_device)) {
-			ret = PTR_ERR(fujitsu_bl->bl_device);
-			fujitsu_bl->bl_device = NULL;
-			goto fail_sysfs_group;
-		}
-		fujitsu_bl->bl_device->props.brightness = fujitsu_bl->brightness_level;
-	}
-
 	ret = platform_driver_register(&fujitsu_pf_driver);
 	if (ret)
-		goto fail_backlight;
+		goto err_unregister_acpi2;
 
-	/* Register laptop driver */
-
-	fujitsu_laptop = kzalloc(sizeof(struct fujitsu_laptop), GFP_KERNEL);
-	if (!fujitsu_laptop) {
+	fujitsu_laptop->pf_device = platform_device_alloc("fujitsu-laptop", -1);
+	if (!fujitsu_laptop->pf_device) {
 		ret = -ENOMEM;
-		goto fail_laptop;
-	}
-
-	result = acpi_bus_register_driver(&acpi_fujitsu_laptop_driver);
-	if (result < 0) {
-		ret = -ENODEV;
-		goto fail_laptop1;
+		goto err_free_platform_driver;
 	}
+	ret = platform_device_add(fujitsu_laptop->pf_device);
+	if (ret)
+		goto err_free_platform_device1;
 
-	/* Sync backlight power status (needs FUJ02E3 device, hence deferred) */
-	if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
-		if (call_fext_func(FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
-			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
-		else
-			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
-	}
+	ret = sysfs_create_group(&fujitsu_laptop->pf_device->dev.kobj,
+				 &fujitsu_pf_attribute_group);
+	if (ret)
+		goto err_free_platform_device2;
 
 	pr_info("driver " FUJITSU_DRIVER_VERSION " successfully loaded\n");
 
 	return 0;
 
-fail_laptop1:
-	kfree(fujitsu_laptop);
-fail_laptop:
+err_free_platform_device2:
+	platform_device_del(fujitsu_laptop->pf_device);
+err_free_platform_device1:
+	platform_device_put(fujitsu_laptop->pf_device);
+err_free_platform_driver:
 	platform_driver_unregister(&fujitsu_pf_driver);
-fail_backlight:
-	backlight_device_unregister(fujitsu_bl->bl_device);
-fail_sysfs_group:
-	sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
-			   &fujitsu_pf_attribute_group);
-fail_platform_device2:
-	platform_device_del(fujitsu_bl->pf_device);
-fail_platform_device1:
-	platform_device_put(fujitsu_bl->pf_device);
-fail_platform_driver:
+err_unregister_acpi2:
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
-fail_acpi:
-	kfree(fujitsu_bl);
+err_unregister_acpi1:
+	acpi_bus_unregister_driver(&acpi_fujitsu_laptop_driver);
+err_stop:
 
 	return ret;
 }
 
 static void __exit fujitsu_cleanup(void)
 {
-	acpi_bus_unregister_driver(&acpi_fujitsu_laptop_driver);
-
-	kfree(fujitsu_laptop);
+	platform_device_unregister(fujitsu_laptop->pf_device);
 
-	platform_driver_unregister(&fujitsu_pf_driver);
-
-	backlight_device_unregister(fujitsu_bl->bl_device);
-
-	sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj,
+	sysfs_remove_group(&fujitsu_laptop->pf_device->dev.kobj,
 			   &fujitsu_pf_attribute_group);
 
-	platform_device_unregister(fujitsu_bl->pf_device);
+	platform_driver_unregister(&fujitsu_pf_driver);
 
 	acpi_bus_unregister_driver(&acpi_fujitsu_bl_driver);
 
-	kfree(fujitsu_bl);
+	acpi_bus_unregister_driver(&acpi_fujitsu_laptop_driver);
 
 	pr_info("driver unloaded\n");
 }