Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Joey Lee]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

Hi all, 

Thank's for Dmitry and Garg's review and point our my driver's problem.

Please kindly review my attached additional patch for clean up all
objects used by MSI scm model when driver initial fail or exit.




Thank's
Joey Lee

於 四，2010-05-13 於 15:54 -0700，Greg KH 提到：
> On Thu, May 13, 2010 at 05:53:04AM -0600, Joey Lee wrote:
> > Hi Dmitry, 
> > 
> > Thank's for your review.
> > 
> > 於 四，2010-05-13 於 00:52 -0700，Dmitry Torokhov 提到：
> > > On Wed, May 12, 2010 at 11:56:47AM -0700, Greg KH wrote:
> > > >  static int __init msi_init(void)
> > > > @@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
> > > >  	platform_driver_unregister(&msipf_driver);
> > > >  	backlight_device_unregister(msibl_device);
> > > >  
> > > > +	i8042_remove_filter(msi_laptop_i8042_filter);
> > > 
> > > You also need cancel_delayed_work_sync() here. Sorry for not noticing
> > > this before.
> > > 
> > 
> > Yes, you are right, need add cancel_delayed_work_sync().
> 
> Want to make up an additional patch to add on top of this that does it?
> 
> thanks,
> 
> greg k-h


[-- Attachment #2: 0011-Clean-up-all-objects-used-by-scm-model-when-driver-i.patch --]
[-- Type: text/plain, Size: 1670 bytes --]

>From 6dd1bf2049496e932e681534c0e1f0ffc0b7a47d Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee@novell.com>
Date: Sat, 15 May 2010 06:18:54 +0800
Subject: [PATCH] Clean up all objects used by scm model when driver initial fail or exit

Clean up i8042 filter, rfkill and cancel delayed work when msi-laptop driver initial fail or exit on MSI scm model.

Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
---
 drivers/platform/x86/msi-laptop.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index cd2c15e..e536232 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -811,6 +811,11 @@ static int __init msi_init(void)
 
 fail_platform_device2:
 
+	if (load_scm_model) {
+		i8042_remove_filter(msi_laptop_i8042_filter);
+		cancel_delayed_work_sync(&msi_rfkill_work);
+		rfkill_cleanup();
+	}
 	platform_device_del(msipf_device);
 
 fail_platform_device1:
@@ -830,15 +835,17 @@ fail_backlight:
 
 static void __exit msi_cleanup(void)
 {
+	if (load_scm_model) {
+		i8042_remove_filter(msi_laptop_i8042_filter);
+		cancel_delayed_work_sync(&msi_rfkill_work);
+		rfkill_cleanup();
+	}
 
 	sysfs_remove_group(&msipf_device->dev.kobj, &msipf_attribute_group);
 	platform_device_unregister(msipf_device);
 	platform_driver_unregister(&msipf_driver);
 	backlight_device_unregister(msibl_device);
 
-	i8042_remove_filter(msi_laptop_i8042_filter);
-	rfkill_cleanup();
-
 	/* Enable automatic brightness control again */
 	if (auto_brightness != 2)
 		set_auto_brightness(1);
-- 
1.6.0.2

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Matthew Garrett]

Hi Joey,

This patch didn't apply cleanly, but I've fixed it up by hand. I've also 
applied the rest of the patchset.

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

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Joey Lee]

Hi Mattew, 

於 四，2010-05-20 於 14:32 +0100，Matthew Garrett 提到：
> Hi Joey,
> 
> This patch didn't apply cleanly, but I've fixed it up by hand. I've also 
> applied the rest of the patchset.
> 

Thank's for your help! :-)


Joey Lee

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Joey Lee]

Hi Greg, 

於 四，2010-05-13 於 15:54 -0700，Greg KH 提到：
> On Thu, May 13, 2010 at 05:53:04AM -0600, Joey Lee wrote:
> > Hi Dmitry, 
> > 
> > Thank's for your review.
> > 
> > 於 四，2010-05-13 於 00:52 -0700，Dmitry Torokhov 提到：
> > > On Wed, May 12, 2010 at 11:56:47AM -0700, Greg KH wrote:
> > > >  static int __init msi_init(void)
> > > > @@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
> > > >  	platform_driver_unregister(&msipf_driver);
> > > >  	backlight_device_unregister(msibl_device);
> > > >  
> > > > +	i8042_remove_filter(msi_laptop_i8042_filter);
> > > 
> > > You also need cancel_delayed_work_sync() here. Sorry for not noticing
> > > this before.
> > > 
> > 
> > Yes, you are right, need add cancel_delayed_work_sync().
> 
> Want to make up an additional patch to add on top of this that does it?
> 

OK! I am doing it, and will submit another patch to fix this issue.


Thank's
Joey Lee

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Joey Lee]

Hi Dmitry, 

Thank's for your review.

於 四，2010-05-13 於 00:52 -0700，Dmitry Torokhov 提到：
> On Wed, May 12, 2010 at 11:56:47AM -0700, Greg KH wrote:
> >  static int __init msi_init(void)
> > @@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
> >  	platform_driver_unregister(&msipf_driver);
> >  	backlight_device_unregister(msibl_device);
> >  
> > +	i8042_remove_filter(msi_laptop_i8042_filter);
> 
> You also need cancel_delayed_work_sync() here. Sorry for not noticing
> this before.
> 

Yes, you are right, need add cancel_delayed_work_sync().

Joey Lee

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Greg KH]

On Thu, May 13, 2010 at 05:53:04AM -0600, Joey Lee wrote:
> Hi Dmitry, 
> 
> Thank's for your review.
> 
> 於 四，2010-05-13 於 00:52 -0700，Dmitry Torokhov 提到：
> > On Wed, May 12, 2010 at 11:56:47AM -0700, Greg KH wrote:
> > >  static int __init msi_init(void)
> > > @@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
> > >  	platform_driver_unregister(&msipf_driver);
> > >  	backlight_device_unregister(msibl_device);
> > >  
> > > +	i8042_remove_filter(msi_laptop_i8042_filter);
> > 
> > You also need cancel_delayed_work_sync() here. Sorry for not noticing
> > this before.
> > 
> 
> Yes, you are right, need add cancel_delayed_work_sync().

Want to make up an additional patch to add on top of this that does it?

thanks,

greg k-h

[PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Greg Kroah-Hartman]

From: Lee, Chun-Yi <jlee@novell.com>

There have some MSI netbook change devices state by EC when user press
wlan/bluetooth/wwan function keys. So, add a i8042 filter to sync sw
state with BIOS when function keys pressed.

Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/platform/x86/msi-laptop.c |   59 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 34bec2e..3c6e160 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -59,6 +59,7 @@
 #include <linux/backlight.h>
 #include <linux/platform_device.h>
 #include <linux/rfkill.h>
+#include <linux/i8042.h>
 
 #define MSI_DRIVER_VERSION "0.5"
 
@@ -581,6 +582,46 @@ static void rfkill_cleanup(void)
 	}
 }
 
+static void msi_update_rfkill(struct work_struct *ignored)
+{
+	get_wireless_state_ec_standard();
+
+	if (rfk_wlan)
+		rfkill_set_sw_state(rfk_wlan, !wlan_s);
+	if (rfk_bluetooth)
+		rfkill_set_sw_state(rfk_bluetooth, !bluetooth_s);
+	if (rfk_threeg)
+		rfkill_set_sw_state(rfk_threeg, !threeg_s);
+}
+static DECLARE_DELAYED_WORK(msi_rfkill_work, msi_update_rfkill);
+
+bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
+				struct serio *port)
+{
+	static bool extended;
+
+	if (str & 0x20)
+		return false;
+
+	/* 0x54 wwan, 0x62 bluetooth, 0x76 wlan*/
+	if (unlikely(data == 0xe0)) {
+		extended = true;
+		return false;
+	} else if (unlikely(extended)) {
+		switch (data) {
+		case 0x54:
+		case 0x62:
+		case 0x76:
+			schedule_delayed_work(&msi_rfkill_work,
+				round_jiffies_relative(0.5 * HZ));
+			break;
+		}
+		extended = false;
+	}
+
+	return false;
+}
+
 static void msi_init_rfkill(struct work_struct *ignored)
 {
 	if (rfk_wlan) {
@@ -706,9 +747,24 @@ static int load_scm_model_init(struct platform_device *sdev)
 	/* initial rfkill */
 	result = rfkill_init(sdev);
 	if (result < 0)
-		return result;
+		goto fail_rfkill;
+
+	result = i8042_install_filter(msi_laptop_i8042_filter);
+	if (result) {
+		printk(KERN_WARNING
+			"msi-laptop: Unable to install key filter\n");
+		goto fail_filter;
+	}
 
 	return 0;
+
+fail_filter:
+	rfkill_cleanup();
+
+fail_rfkill:
+
+	return result;
+
 }
 
 static int __init msi_init(void)
@@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
 	platform_driver_unregister(&msipf_driver);
 	backlight_device_unregister(msibl_device);
 
+	i8042_remove_filter(msi_laptop_i8042_filter);
 	rfkill_cleanup();
 
 	/* Enable automatic brightness control again */
-- 
1.7.1

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Dmitry Torokhov]

On Wed, May 12, 2010 at 09:58:10AM -0700, Greg Kroah-Hartman wrote:
> From: Lee, Chun-Yi <jlee@novell.com>
> 
> There have some MSI netbook change devices state by EC when user press
> wlan/bluetooth/wwan function keys. So, add a i8042 filter to sync sw
> state with BIOS when function keys pressed.
> 
> Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> ---
>  drivers/platform/x86/msi-laptop.c |   59 ++++++++++++++++++++++++++++++++++++-
>  1 files changed, 58 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
> index 34bec2e..3c6e160 100644
> --- a/drivers/platform/x86/msi-laptop.c
> +++ b/drivers/platform/x86/msi-laptop.c
> @@ -59,6 +59,7 @@
>  #include <linux/backlight.h>
>  #include <linux/platform_device.h>
>  #include <linux/rfkill.h>
> +#include <linux/i8042.h>
>  
>  #define MSI_DRIVER_VERSION "0.5"
>  
> @@ -581,6 +582,46 @@ static void rfkill_cleanup(void)
>  	}
>  }
>  
> +static void msi_update_rfkill(struct work_struct *ignored)
> +{
> +	get_wireless_state_ec_standard();
> +
> +	if (rfk_wlan)
> +		rfkill_set_sw_state(rfk_wlan, !wlan_s);
> +	if (rfk_bluetooth)
> +		rfkill_set_sw_state(rfk_bluetooth, !bluetooth_s);
> +	if (rfk_threeg)
> +		rfkill_set_sw_state(rfk_threeg, !threeg_s);
> +}
> +static DECLARE_DELAYED_WORK(msi_rfkill_work, msi_update_rfkill);
> +
> +bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
> +				struct serio *port)

Make it static bool...

> +{
> +	static bool extended;
> +
> +	if (str & 0x20)
> +		return false;
> +
> +	/* 0x54 wwan, 0x62 bluetooth, 0x76 wlan*/
> +	if (unlikely(data == 0xe0)) {
> +		extended = true;
> +		return false;
> +	} else if (unlikely(extended)) {
> +		switch (data) {
> +		case 0x54:
> +		case 0x62:
> +		case 0x76:
> +			schedule_delayed_work(&msi_rfkill_work,
> +				round_jiffies_relative(0.5 * HZ));
> +			break;
> +		}
> +		extended = false;
> +	}
> +
> +	return false;
> +}
> +
>  static void msi_init_rfkill(struct work_struct *ignored)
>  {
>  	if (rfk_wlan) {
> @@ -706,9 +747,24 @@ static int load_scm_model_init(struct platform_device *sdev)
>  	/* initial rfkill */
>  	result = rfkill_init(sdev);
>  	if (result < 0)
> -		return result;
> +		goto fail_rfkill;
> +
> +	result = i8042_install_filter(msi_laptop_i8042_filter);
> +	if (result) {
> +		printk(KERN_WARNING

Since we fail driver load this should be error, not warning message.

> +			"msi-laptop: Unable to install key filter\n");
> +		goto fail_filter;
> +	}
>  
>  	return 0;
> +
> +fail_filter:
> +	rfkill_cleanup();
> +
> +fail_rfkill:
> +
> +	return result;
> +
>  }
>  
>  static int __init msi_init(void)
> @@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
>  	platform_driver_unregister(&msipf_driver);
>  	backlight_device_unregister(msibl_device);
>  
> +	i8042_remove_filter(msi_laptop_i8042_filter);
>  	rfkill_cleanup();
>  
>  	/* Enable automatic brightness control again */

-- 
Dmitry

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Greg KH]

On Wed, May 12, 2010 at 10:33:59AM -0700, Dmitry Torokhov wrote:
> On Wed, May 12, 2010 at 09:58:10AM -0700, Greg Kroah-Hartman wrote:
> > From: Lee, Chun-Yi <jlee@novell.com>
> > 
> > There have some MSI netbook change devices state by EC when user press
> > wlan/bluetooth/wwan function keys. So, add a i8042 filter to sync sw
> > state with BIOS when function keys pressed.
> > 
> > Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> > ---
> >  drivers/platform/x86/msi-laptop.c |   59 ++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 58 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
> > index 34bec2e..3c6e160 100644
> > --- a/drivers/platform/x86/msi-laptop.c
> > +++ b/drivers/platform/x86/msi-laptop.c
> > @@ -59,6 +59,7 @@
> >  #include <linux/backlight.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/rfkill.h>
> > +#include <linux/i8042.h>
> >  
> >  #define MSI_DRIVER_VERSION "0.5"
> >  
> > @@ -581,6 +582,46 @@ static void rfkill_cleanup(void)
> >  	}
> >  }
> >  
> > +static void msi_update_rfkill(struct work_struct *ignored)
> > +{
> > +	get_wireless_state_ec_standard();
> > +
> > +	if (rfk_wlan)
> > +		rfkill_set_sw_state(rfk_wlan, !wlan_s);
> > +	if (rfk_bluetooth)
> > +		rfkill_set_sw_state(rfk_bluetooth, !bluetooth_s);
> > +	if (rfk_threeg)
> > +		rfkill_set_sw_state(rfk_threeg, !threeg_s);
> > +}
> > +static DECLARE_DELAYED_WORK(msi_rfkill_work, msi_update_rfkill);
> > +
> > +bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
> > +				struct serio *port)
> 
> Make it static bool...

Will do.

> >  static void msi_init_rfkill(struct work_struct *ignored)
> >  {
> >  	if (rfk_wlan) {
> > @@ -706,9 +747,24 @@ static int load_scm_model_init(struct platform_device *sdev)
> >  	/* initial rfkill */
> >  	result = rfkill_init(sdev);
> >  	if (result < 0)
> > -		return result;
> > +		goto fail_rfkill;
> > +
> > +	result = i8042_install_filter(msi_laptop_i8042_filter);
> > +	if (result) {
> > +		printk(KERN_WARNING
> 
> Since we fail driver load this should be error, not warning message.

Good point, I'll fix that up as well.

I'll respin this now.

thanks,

greg k-h

[PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Greg KH]

From: Lee, Chun-Yi <jlee@novell.com>

There have some MSI netbook change devices state by EC when user press
wlan/bluetooth/wwan function keys. So, add a i8042 filter to sync sw
state with BIOS when function keys pressed.

Signed-off-by: Lee, Chun-Yi <jlee@novell.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 v2 - fixed some things Dmitry pointed out - gregkh

 drivers/platform/x86/msi-laptop.c |   59 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 34bec2e..0b17590 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -59,6 +59,7 @@
 #include <linux/backlight.h>
 #include <linux/platform_device.h>
 #include <linux/rfkill.h>
+#include <linux/i8042.h>
 
 #define MSI_DRIVER_VERSION "0.5"
 
@@ -581,6 +582,46 @@ static void rfkill_cleanup(void)
 	}
 }
 
+static void msi_update_rfkill(struct work_struct *ignored)
+{
+	get_wireless_state_ec_standard();
+
+	if (rfk_wlan)
+		rfkill_set_sw_state(rfk_wlan, !wlan_s);
+	if (rfk_bluetooth)
+		rfkill_set_sw_state(rfk_bluetooth, !bluetooth_s);
+	if (rfk_threeg)
+		rfkill_set_sw_state(rfk_threeg, !threeg_s);
+}
+static DECLARE_DELAYED_WORK(msi_rfkill_work, msi_update_rfkill);
+
+static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
+				    struct serio *port)
+{
+	static bool extended;
+
+	if (str & 0x20)
+		return false;
+
+	/* 0x54 wwan, 0x62 bluetooth, 0x76 wlan*/
+	if (unlikely(data == 0xe0)) {
+		extended = true;
+		return false;
+	} else if (unlikely(extended)) {
+		switch (data) {
+		case 0x54:
+		case 0x62:
+		case 0x76:
+			schedule_delayed_work(&msi_rfkill_work,
+				round_jiffies_relative(0.5 * HZ));
+			break;
+		}
+		extended = false;
+	}
+
+	return false;
+}
+
 static void msi_init_rfkill(struct work_struct *ignored)
 {
 	if (rfk_wlan) {
@@ -706,9 +747,24 @@ static int load_scm_model_init(struct platform_device *sdev)
 	/* initial rfkill */
 	result = rfkill_init(sdev);
 	if (result < 0)
-		return result;
+		goto fail_rfkill;
+
+	result = i8042_install_filter(msi_laptop_i8042_filter);
+	if (result) {
+		printk(KERN_ERR
+			"msi-laptop: Unable to install key filter\n");
+		goto fail_filter;
+	}
 
 	return 0;
+
+fail_filter:
+	rfkill_cleanup();
+
+fail_rfkill:
+
+	return result;
+
 }
 
 static int __init msi_init(void)
@@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
 	platform_driver_unregister(&msipf_driver);
 	backlight_device_unregister(msibl_device);
 
+	i8042_remove_filter(msi_laptop_i8042_filter);
 	rfkill_cleanup();
 
 	/* Enable automatic brightness control again */

Re: [PATCH 4/4] msi-laptop: Add i8042 filter to sync sw state with BIOS when function key pressed

[Dmitry Torokhov]

On Wed, May 12, 2010 at 11:56:47AM -0700, Greg KH wrote:
>  static int __init msi_init(void)
> @@ -819,6 +875,7 @@ static void __exit msi_cleanup(void)
>  	platform_driver_unregister(&msipf_driver);
>  	backlight_device_unregister(msibl_device);
>  
> +	i8042_remove_filter(msi_laptop_i8042_filter);

You also need cancel_delayed_work_sync() here. Sorry for not noticing
this before.

>  	rfkill_cleanup();
>  
>  	/* Enable automatic brightness control again */

-- 
Dmitry