[PATCH 0/5] dell-laptop improvements

[PATCH 0/5] dell-laptop improvements

[Alan Jenkins]

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

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

[Alan Jenkins]

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 <alan-jenkins@tuffmail.co.uk>
---
 drivers/platform/x86/dell-laptop.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 74909c4..12b6f33 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -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;
 }
-- 
1.6.3.2

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

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

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

[Alan Jenkins]

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

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 drivers/platform/x86/dell-laptop.c |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 12b6f33..8fbff38 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -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);
-- 
1.6.3.2

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

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

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

[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 <alan-jenkins@tuffmail.co.uk>
---
 drivers/platform/x86/dell-laptop.c |   38 ++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 8fbff38..a13a9f7 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -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;
 }
-- 
1.6.3.2

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

[Alan Jenkins]

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 drivers/platform/x86/dell-laptop.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index a13a9f7..250c4b1 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -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;
-- 
1.6.3.2

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

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

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

[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 <alan-jenkins@tuffmail.co.uk>
---
 drivers/platform/x86/dell-laptop.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 250c4b1..349cf12 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -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)
-- 
1.6.3.2

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

[Matthew Garrett]

On Wed, Aug 19, 2009 at 03:06:51PM +0100, Alan Jenkins wrote:
> 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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Alan Jenkins]

Matthew Garrett wrote:
> On Wed, Aug 19, 2009 at 03:06:51PM +0100, Alan Jenkins wrote:
>   
>> 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.

----------------------------------------------------------------------
>From 7efa12d3ea8bb70897358b4a87f8dedce9b78cca Mon Sep 17 00:00:00 2001
From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
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 <alan-jenkins@tuffmail.co.uk>
---
 drivers/misc/Kconfig |   13 -------------
 1 files changed, 0 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 68ab39d..22414c5 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -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
-- 
1.6.3.2

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

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/5] dell-laptop improvements

[Matthew Garrett]

On Wed, Aug 19, 2009 at 03:03:15PM +0100, Alan Jenkins wrote:

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

-- 
Matthew Garrett | mjg59@srcf.ucam.org