Skip to contentSkip to conversation

dell-laptop improvements

PATCH Applied Alan Jenkins 3 participants 13 messages Aug 19, 2009 – Dec 10, 2009

Subsystems (3)
ACPI Char and misc drivers x86 platform drivers
lore
Share & tools
lore

Alan Jenkins

author
#0 lore raw

cover dell-laptop improvements

Mario Limonciello's compal-laptop changes were partly based on a reading of dell-laptop. Unfortunately dell-laptop set a few bad examples; let's fix them.

I don't have the hardware to test this, but the first four patches should be nice and low risk.

dell-laptop: fix a use-after-free error on the failure path
dell-laptop: fix rfkill memory leak on unload and failure paths
dell-laptop: create a platform device as a parent for the rfkill devices etc.
dell-laptop: add __init to init functions

The last patch adds polling for the hardware switch which blocks all radios. This exercises the hardware a little more than before; it would benefit from testing. It should be possible to see events generated by the hardware switch using "udevadm monitor --kernel --environment".

dell-laptop: poll the rfkill hard-block

author
#1 lore raw

PATCH 1/5 dell-laptop: fix a use-after-free error on the failure path

dell_setup_rfkill() already cleans up the rfkill devices on failure. So if it returns an error, we should not try to unregister the rfkill devices.

Signed-off-by Alan Jenkins <[email protected]>
---
 drivers/platform/x86/dell-laptop.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/platform/x86/dell-laptop.c +5 -3
index 74909c4..12b6f33 100644
@@ -330,7 +330,7 @@ static int __init dell_init(void)
 
 	if (ret) {
 		printk(KERN_WARNING "dell-laptop: Unable to setup rfkill\n");
-		goto out;
+		goto fail_rfkill;
 	}
 
 #ifdef CONFIG_ACPI

@@ -358,7 +358,7 @@ static int __init dell_init(void)
 		if (IS_ERR(dell_backlight_device)) {
 			ret = PTR_ERR(dell_backlight_device);
 			dell_backlight_device = NULL;
-			goto out;
+			goto fail_backlight;
 		}
 
 		dell_backlight_device->props.max_brightness = max_intensity;

@@ -368,13 +368,15 @@ static int __init dell_init(void)
 	}
 
 	return 0;
-out:
+
+fail_backlight:
 	if (wifi_rfkill)
 		rfkill_unregister(wifi_rfkill);
 	if (bluetooth_rfkill)
 		rfkill_unregister(bluetooth_rfkill);
 	if (wwan_rfkill)
 		rfkill_unregister(wwan_rfkill);
+fail_rfkill:
 	kfree(da_tokens);
 	return ret;
 }
Signature · 1 line
1.6.3.2
reviewer Applied
#2 lore raw

Re: [PATCH 1/5] dell-laptop: fix a use-after-free error on the failure path

applied

thanks,
Len Brown, Intel Open Source Technology Center

author
#3 lore raw

PATCH 2/5 dell-laptop: fix rfkill memory leak on unload and failure paths

In reply to Alan Jenkins

rfkill_unregister() should always be followed by rfkill_destroy().

Signed-off-by Alan Jenkins <[email protected]>
---
 drivers/platform/x86/dell-laptop.c |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)
drivers/platform/x86/dell-laptop.c +18 -12
index 12b6f33..8fbff38 100644
@@ -268,6 +268,22 @@ err_wifi:
 	return ret;
 }
 
+static void dell_cleanup_rfkill(void)
+{
+	if (wifi_rfkill) {
+		rfkill_unregister(wifi_rfkill);
+		rfkill_destroy(wifi_rfkill);
+	}
+	if (bluetooth_rfkill) {
+		rfkill_unregister(bluetooth_rfkill);
+		rfkill_destroy(bluetooth_rfkill);
+	}
+	if (wwan_rfkill) {
+		rfkill_unregister(wwan_rfkill);
+		rfkill_destroy(wwan_rfkill);
+	}
+}
+
 static int dell_send_intensity(struct backlight_device *bd)
 {
 	struct calling_interface_buffer buffer;

@@ -370,12 +386,7 @@ static int __init dell_init(void)
 	return 0;
 
 fail_backlight:
-	if (wifi_rfkill)
-		rfkill_unregister(wifi_rfkill);
-	if (bluetooth_rfkill)
-		rfkill_unregister(bluetooth_rfkill);
-	if (wwan_rfkill)
-		rfkill_unregister(wwan_rfkill);
+	dell_cleanup_rfkill();
 fail_rfkill:
 	kfree(da_tokens);
 	return ret;

@@ -384,12 +395,7 @@ fail_rfkill:
 static void __exit dell_exit(void)
 {
 	backlight_device_unregister(dell_backlight_device);
-	if (wifi_rfkill)
-		rfkill_unregister(wifi_rfkill);
-	if (bluetooth_rfkill)
-		rfkill_unregister(bluetooth_rfkill);
-	if (wwan_rfkill)
-		rfkill_unregister(wwan_rfkill);
+	dell_cleanup_rfkill();
 }
 
 module_init(dell_init);
Signature · 1 line
1.6.3.2
reviewer Applied
#4 lore raw

Re: [PATCH 2/5] dell-laptop: fix rfkill memory leak on unload and failure paths

applied

thanks,
Len Brown, Intel Open Source Technology Center

#5 lore raw
In reply to Alan Jenkins
Quoted 4 lines · Alan Jenkins

The last patch adds polling for the hardware switch which blocks all radios. This exercises the hardware a little more than before; it would benefit from testing. It should be possible to see events generated by the hardware switch using "udevadm monitor --kernel --environment".

We get a hardware event (via the keyboard controller...) when the switch is set. I need to fix up my i8042 filtering patch to match feedback from the maintainer, but there's no real need to poll.

Signature · 5 lines
Matthew Garrett | [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#6 lore raw

PATCH 3/5 dell-laptop: create a platform device as a parent for the rfkill devices etc.

In reply to Alan Jenkins

dell-laptop may not need to export any sysfs files, but it should still create a platform device as a parent for the rfkill and backlight devices. Otherwise sysfs will display these as "virtual" devices, with no connection to either physical hardware or the dell-laptop module.

Apparently this is useful for hardware detection.

Signed-off-by Alan Jenkins <[email protected]>
---
 drivers/platform/x86/dell-laptop.c |   38 ++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 4 deletions(-)
drivers/platform/x86/dell-laptop.c +34 -4
index 8fbff38..a13a9f7 100644
@@ -58,6 +58,14 @@ static int da_command_code;
 static int da_num_tokens;
 static struct calling_interface_token *da_tokens;
 
+static struct platform_driver platform_driver = {
+	.driver = {
+		.name = "dell-laptop",
+		.owner = THIS_MODULE,
+	}
+};
+
+static struct platform_device *platform_device;
 static struct backlight_device *dell_backlight_device;
 static struct rfkill *wifi_rfkill;
 static struct rfkill *bluetooth_rfkill;

@@ -217,7 +225,8 @@ static int dell_setup_rfkill(void)
 	status = buffer.output[1];
 
 	if ((status & (1<<2|1<<8)) == (1<<2|1<<8)) {
-		wifi_rfkill = rfkill_alloc("dell-wifi", NULL, RFKILL_TYPE_WLAN,
+		wifi_rfkill = rfkill_alloc("dell-wifi", &platform_device->dev,
+					   RFKILL_TYPE_WLAN,
 					   &dell_rfkill_ops, (void *) 1);
 		if (!wifi_rfkill) {
 			ret = -ENOMEM;

@@ -229,7 +238,8 @@ static int dell_setup_rfkill(void)
 	}
 
 	if ((status & (1<<3|1<<9)) == (1<<3|1<<9)) {
-		bluetooth_rfkill = rfkill_alloc("dell-bluetooth", NULL,
+		bluetooth_rfkill = rfkill_alloc("dell-bluetooth",
+						&platform_device->dev,
 						RFKILL_TYPE_BLUETOOTH,
 						&dell_rfkill_ops, (void *) 2);
 		if (!bluetooth_rfkill) {

@@ -242,7 +252,9 @@ static int dell_setup_rfkill(void)
 	}
 
 	if ((status & (1<<4|1<<10)) == (1<<4|1<<10)) {
-		wwan_rfkill = rfkill_alloc("dell-wwan", NULL, RFKILL_TYPE_WWAN,
+		wwan_rfkill = rfkill_alloc("dell-wwan",
+					   &platform_device->dev,
+					   RFKILL_TYPE_WWAN,
 					   &dell_rfkill_ops, (void *) 3);
 		if (!wwan_rfkill) {
 			ret = -ENOMEM;

@@ -342,6 +354,18 @@ static int __init dell_init(void)
 		return -ENODEV;
 	}
 
+	ret = platform_driver_register(&platform_driver);
+	if (ret)
+		goto fail_platform_driver;
+	platform_device = platform_device_alloc("dell-laptop", -1);
+	if (!platform_device) {
+		ret = -ENOMEM;
+		goto fail_platform_device1;
+	}
+	ret = platform_device_add(platform_device);
+	if (ret)
+		goto fail_platform_device2;
+
 	ret = dell_setup_rfkill();
 
 	if (ret) {

@@ -368,7 +392,7 @@ static int __init dell_init(void)
 	if (max_intensity) {
 		dell_backlight_device = backlight_device_register(
 			"dell_backlight",
-			NULL, NULL,
+			&platform_device->dev, NULL,
 			&dell_ops);
 
 		if (IS_ERR(dell_backlight_device)) {

@@ -388,6 +412,12 @@ static int __init dell_init(void)
 fail_backlight:
 	dell_cleanup_rfkill();
 fail_rfkill:
+	platform_device_del(platform_device);
+fail_platform_device2:
+	platform_device_put(platform_device);
+fail_platform_device1:
+	platform_driver_unregister(&platform_driver);
+fail_platform_driver:
 	kfree(da_tokens);
 	return ret;
 }
Signature · 6 lines
1.6.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#7 lore raw

PATCH 4/5 dell-laptop: add __init to init functions

In reply to Alan Jenkins
Signed-off-by Alan Jenkins <[email protected]>
---
 drivers/platform/x86/dell-laptop.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/platform/x86/dell-laptop.c +3 -3
index a13a9f7..250c4b1 100644
@@ -82,7 +82,7 @@ static const struct dmi_system_id __initdata dell_device_table[] = {
 	{ }
 };
 
-static void parse_da_table(const struct dmi_header *dm)
+static void __init parse_da_table(const struct dmi_header *dm)
 {
 	/* Final token is a terminator, so we don't want to copy it */
 	int tokens = (dm->length-11)/sizeof(struct calling_interface_token)-1;

@@ -111,7 +111,7 @@ static void parse_da_table(const struct dmi_header *dm)
 	da_num_tokens += tokens;
 }
 
-static void find_tokens(const struct dmi_header *dm, void *dummy)
+static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 {
 	switch (dm->type) {
 	case 0xd4: /* Indexed IO */

@@ -214,7 +214,7 @@ static const struct rfkill_ops dell_rfkill_ops = {
 	.query = dell_rfkill_query,
 };
 
-static int dell_setup_rfkill(void)
+static int __init dell_setup_rfkill(void)
 {
 	struct calling_interface_buffer buffer;
 	int status;
Signature · 1 line
1.6.3.2
reviewer Applied
#8 lore raw

Re: [PATCH 4/5] dell-laptop: add __init to init functions

applied

thanks,
Len Brown, Intel Open Source Technology Center

author
#9 lore raw

PATCH 5/5 dell-laptop: poll the rfkill hard-block

In reply to Alan Jenkins

This is controlled by a hardware switch, so we should poll it in order to pick up changes. (There does not appear to be an interrupt or any other notification mechanism).

Signed-off-by Alan Jenkins <[email protected]>
---
 drivers/platform/x86/dell-laptop.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/platform/x86/dell-laptop.c +2 -2
index 250c4b1..349cf12 100644
@@ -195,7 +195,7 @@ static int dell_rfkill_set(void *data, bool blocked)
 	return 0;
 }
 
-static void dell_rfkill_query(struct rfkill *rfkill, void *data)
+static void dell_rfkill_poll(struct rfkill *rfkill, void *data)
 {
 	struct calling_interface_buffer buffer;
 	int status;

@@ -211,7 +211,7 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 
 static const struct rfkill_ops dell_rfkill_ops = {
 	.set_block = dell_rfkill_set,
-	.query = dell_rfkill_query,
+	.poll = dell_rfkill_poll,
 };
 
 static int __init dell_setup_rfkill(void)
Signature · 1 line
1.6.3.2
#10 lore raw

Re: [PATCH 5/5] dell-laptop: poll the rfkill hard-block

Quoted 3 lines · Alan Jenkins

This is controlled by a hardware switch, so we should poll it in order to pick up changes. (There does not appear to be an interrupt or any other notification mechanism).

With the exception of this one they all look fine - I'm moving house next week and won't have access to any Dell hardware for over a month, so if anyone else could confirm that these work that would be excellent.

Signature · 1 line
Matthew Garrett | [email protected]
author
#11 lore raw

[PATCH 6/5] dell-laptop: remove duplicate Kconfig entry under drivers/misc

Quoted 12 lines · Matthew Garrett
Quoted 4 lines · Alan Jenkins

This is controlled by a hardware switch, so we should poll it in order to pick up changes. (There does not appear to be an interrupt or any other notification mechanism).

With the exception of this one they all look fine - I'm moving house next week and won't have access to any Dell hardware for over a month, so if anyone else could confirm that these work that would be excellent.

Ok, thanks!

Here's something else I just noticed. I was able to test this one myself.

----------------------------------------------------------------------

Quoted 1 line

From 7efa12d3ea8bb70897358b4a87f8dedce9b78cca Mon Sep 17 00:00:00 2001

From: Alan Jenkins <[email protected]>
Date: Wed, 19 Aug 2009 15:32:17 +0100
Subject: [PATCH] dell-laptop: remove duplicate Kconfig entry under drivers/misc

This showed up as an unselectable option when using xconfig and searching for "dell". It must been overlooked when dell-laptop was moved to drivers/platform/x86.

Signed-off-by Alan Jenkins <[email protected]>
---
 drivers/misc/Kconfig |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)
drivers/misc/Kconfig +0 -13
index 68ab39d..22414c5 100644
@@ -210,19 +210,6 @@ config SGI_GRU_DEBUG
 	This option enables addition debugging code for the SGI GRU driver. If
 	you are unsure, say N.
 
-config DELL_LAPTOP
-	tristate "Dell Laptop Extras (EXPERIMENTAL)"
-	depends on X86
-	depends on DCDBAS
-	depends on EXPERIMENTAL
-	depends on BACKLIGHT_CLASS_DEVICE
-	depends on RFKILL
-	depends on POWER_SUPPLY
-	default n
-	---help---
-	This driver adds support for rfkill and backlight control to Dell
-	laptops.
-
 config ISL29003
 	tristate "Intersil ISL29003 ambient light sensor"
 	depends on I2C && SYSFS
Signature · 1 line
1.6.3.2