Patchwork [Maverick,pull,request] Input: i8042 - reset keyboard controller wehen resuming from S2R

login
register
mail settings
Submitter Jerome Lacoste
Date Sept. 8, 2010, 7:56 a.m.
Message ID <AANLkTimVyoQVm6GFq7H4i8sRhSZp99N7X6h09+3rA+vk@mail.gmail.com>
Download mbox | patch
Permalink /patch/64119/
State Accepted
Delegated to: Leann Ogasawara
Headers show

Comments

Jerome Lacoste - Sept. 8, 2010, 7:56 a.m.
This patches fixes:

BugLink: http://bugs.launchpad.net/bugs/86820

(cherry-pick from commit 1ca56e513a9fd356d5a9e0de45dbe0e189e00386
inux/kernel/git/torvalds/linux-2.6.git)

(upstream patch in 2.6.36, tested and backported on 2.6.32's Lucid for
a month. Patches applies cleanly on 2.6.35-20.29)

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

From 1ca56e513a9fd356d5a9e0de45dbe0e189e00386 Mon Sep 17 00:00:00 2001
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Tue, 20 Jul 2010 20:25:34 -0700
Subject: [PATCH] Input: i8042 - reset keyboard controller wehen
resuming from S2R

Some laptops, such as Lenovo 3000 N100, require keyboard controller reset
in order to have touchpad operable after suspend to RAM. Even if box does
not need the reset it should be safe to do so, so instead of chasing
after misbehaving boxes and grow DMI tables, let's reset the controller
unconditionally.

Reported-and-tested-by: Jerome Lacoste <jerome.lacoste@gmail.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/serio/i8042.c |   65 +++++++++++++++++++++++++++----------------
 1 files changed, 41 insertions(+), 24 deletions(-)

 	if (error)
Leann Ogasawara - Sept. 10, 2010, 3:40 a.m.
Applied to Maverick linux master.  Jerome, this would also seem to be an
appropriate candidate for the upstream stable kernels.  Care to forward
this along there as well?  For information on submitting patches to
upstream stable, refer to:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=Documentation/stable_kernel_rules.txt;hb=HEAD

Thanks,
Leann

On Wed, 2010-09-08 at 09:56 +0200, Jerome Lacoste wrote:
> This patches fixes:
> 
> BugLink: http://bugs.launchpad.net/bugs/86820
> 
> (cherry-pick from commit 1ca56e513a9fd356d5a9e0de45dbe0e189e00386
> inux/kernel/git/torvalds/linux-2.6.git)
> 
> (upstream patch in 2.6.36, tested and backported on 2.6.32's Lucid for
> a month. Patches applies cleanly on 2.6.35-20.29)
> 
> -------------------------------------------------------------------
> 
> From 1ca56e513a9fd356d5a9e0de45dbe0e189e00386 Mon Sep 17 00:00:00 2001
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date: Tue, 20 Jul 2010 20:25:34 -0700
> Subject: [PATCH] Input: i8042 - reset keyboard controller wehen
> resuming from S2R
> 
> Some laptops, such as Lenovo 3000 N100, require keyboard controller reset
> in order to have touchpad operable after suspend to RAM. Even if box does
> not need the reset it should be safe to do so, so instead of chasing
> after misbehaving boxes and grow DMI tables, let's reset the controller
> unconditionally.
> 
> Reported-and-tested-by: Jerome Lacoste <jerome.lacoste@gmail.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>  drivers/input/serio/i8042.c |   65 +++++++++++++++++++++++++++----------------
>  1 files changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 6440a8f..258b98b 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -861,9 +861,6 @@ static int i8042_controller_selftest(void)
>  	unsigned char param;
>  	int i = 0;
> 
> -	if (!i8042_reset)
> -		return 0;
> -
>  	/*
>  	 * We try this 5 times; on some really fragile systems this does not
>  	 * take the first time...
> @@ -1020,7 +1017,8 @@ static void i8042_controller_reset(void)
>   * Reset the controller if requested.
>   */
> 
> -	i8042_controller_selftest();
> +	if (i8042_reset)
> +		i8042_controller_selftest();
> 
>  /*
>   * Restore the original control register setting.
> @@ -1094,23 +1092,11 @@ static void i8042_dritek_enable(void)
>  #ifdef CONFIG_PM
> 
>  /*
> - * Here we try to restore the original BIOS settings to avoid
> - * upsetting it.
> - */
> -
> -static int i8042_pm_reset(struct device *dev)
> -{
> -	i8042_controller_reset();
> -
> -	return 0;
> -}
> -
> -/*
>   * Here we try to reset everything back to a state we had
>   * before suspending.
>   */
> 
> -static int i8042_pm_restore(struct device *dev)
> +static int i8042_controller_resume(bool force_reset)
>  {
>  	int error;
> 
> @@ -1118,9 +1104,11 @@ static int i8042_pm_restore(struct device *dev)
>  	if (error)
>  		return error;
> 
> -	error = i8042_controller_selftest();
> -	if (error)
> -		return error;
> +	if (i8042_reset || force_reset) {
> +		error = i8042_controller_selftest();
> +		if (error)
> +			return error;
> +	}
> 
>  /*
>   * Restore original CTR value and disable all ports
> @@ -1162,6 +1150,28 @@ static int i8042_pm_restore(struct device *dev)
>  	return 0;
>  }
> 
> +/*
> + * Here we try to restore the original BIOS settings to avoid
> + * upsetting it.
> + */
> +
> +static int i8042_pm_reset(struct device *dev)
> +{
> +	i8042_controller_reset();
> +
> +	return 0;
> +}
> +
> +static int i8042_pm_resume(struct device *dev)
> +{
> +	/*
> +	 * On resume from S2R we always try to reset the controller
> +	 * to bring it in a sane state. (In case of S2D we expect
> +	 * BIOS to reset the controller for us.)
> +	 */
> +	return i8042_controller_resume(true);
> +}
> +
>  static int i8042_pm_thaw(struct device *dev)
>  {
>  	i8042_interrupt(0, NULL);
> @@ -1169,9 +1179,14 @@ static int i8042_pm_thaw(struct device *dev)
>  	return 0;
>  }
> 
> +static int i8042_pm_restore(struct device *dev)
> +{
> +	return i8042_controller_resume(false);
> +}
> +
>  static const struct dev_pm_ops i8042_pm_ops = {
>  	.suspend	= i8042_pm_reset,
> -	.resume		= i8042_pm_restore,
> +	.resume		= i8042_pm_resume,
>  	.thaw		= i8042_pm_thaw,
>  	.poweroff	= i8042_pm_reset,
>  	.restore	= i8042_pm_restore,
> @@ -1389,9 +1404,11 @@ static int __init i8042_probe(struct
> platform_device *dev)
> 
>  	i8042_platform_device = dev;
> 
> -	error = i8042_controller_selftest();
> -	if (error)
> -		return error;
> +	if (i8042_reset) {
> +		error = i8042_controller_selftest();
> +		if (error)
> +			return error;
> +	}
> 
>  	error = i8042_controller_init();
>  	if (error)
> -- 
> 1.7.0.4
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Jerome Lacoste - Sept. 10, 2010, 5:12 a.m.
On Fri, Sep 10, 2010 at 5:40 AM, Leann Ogasawara
<leann.ogasawara@canonical.com> wrote:
> Applied to Maverick linux master.  Jerome, this would also seem to be an
> appropriate candidate for the upstream stable kernels.  Care to forward
> this along there as well?  For information on submitting patches to
> upstream stable, refer to:
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=Documentation/stable_kernel_rules.txt;hb=HEAD

I can try to get it merged into the stable tree. Do you really think
it passes the 'that's not good' rule ?

  "It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical."

Is the inability to resume properly a critical issue from a kernel POV ?

Jerome
Leann Ogasawara - Sept. 10, 2010, 2:33 p.m.
On Fri, 2010-09-10 at 07:12 +0200, Jerome Lacoste wrote:
> On Fri, Sep 10, 2010 at 5:40 AM, Leann Ogasawara
> <leann.ogasawara@canonical.com> wrote:
> > Applied to Maverick linux master.  Jerome, this would also seem to be an
> > appropriate candidate for the upstream stable kernels.  Care to forward
> > this along there as well?  For information on submitting patches to
> > upstream stable, refer to:
> >
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=Documentation/stable_kernel_rules.txt;hb=HEAD
> 
> I can try to get it merged into the stable tree. Do you really think
> it passes the 'that's not good' rule ?
> 
>   "It must fix a problem that causes a build error (but not for things
>    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>    security issue, or some "oh, that's not good" issue.  In short, something
>    critical."
> 
> Is the inability to resume properly a critical issue from a kernel POV ?

In my opinion a patch to resolve inoperable touchpads after suspend to
RAM qualifies.  The worst that could happen is it doesn't get accepted,
at which point at least we tried.

Thanks,
Leann

Patch

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 6440a8f..258b98b 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -861,9 +861,6 @@  static int i8042_controller_selftest(void)
 	unsigned char param;
 	int i = 0;

-	if (!i8042_reset)
-		return 0;
-
 	/*
 	 * We try this 5 times; on some really fragile systems this does not
 	 * take the first time...
@@ -1020,7 +1017,8 @@  static void i8042_controller_reset(void)
  * Reset the controller if requested.
  */

-	i8042_controller_selftest();
+	if (i8042_reset)
+		i8042_controller_selftest();

 /*
  * Restore the original control register setting.
@@ -1094,23 +1092,11 @@  static void i8042_dritek_enable(void)
 #ifdef CONFIG_PM

 /*
- * Here we try to restore the original BIOS settings to avoid
- * upsetting it.
- */
-
-static int i8042_pm_reset(struct device *dev)
-{
-	i8042_controller_reset();
-
-	return 0;
-}
-
-/*
  * Here we try to reset everything back to a state we had
  * before suspending.
  */

-static int i8042_pm_restore(struct device *dev)
+static int i8042_controller_resume(bool force_reset)
 {
 	int error;

@@ -1118,9 +1104,11 @@  static int i8042_pm_restore(struct device *dev)
 	if (error)
 		return error;

-	error = i8042_controller_selftest();
-	if (error)
-		return error;
+	if (i8042_reset || force_reset) {
+		error = i8042_controller_selftest();
+		if (error)
+			return error;
+	}

 /*
  * Restore original CTR value and disable all ports
@@ -1162,6 +1150,28 @@  static int i8042_pm_restore(struct device *dev)
 	return 0;
 }

+/*
+ * Here we try to restore the original BIOS settings to avoid
+ * upsetting it.
+ */
+
+static int i8042_pm_reset(struct device *dev)
+{
+	i8042_controller_reset();
+
+	return 0;
+}
+
+static int i8042_pm_resume(struct device *dev)
+{
+	/*
+	 * On resume from S2R we always try to reset the controller
+	 * to bring it in a sane state. (In case of S2D we expect
+	 * BIOS to reset the controller for us.)
+	 */
+	return i8042_controller_resume(true);
+}
+
 static int i8042_pm_thaw(struct device *dev)
 {
 	i8042_interrupt(0, NULL);
@@ -1169,9 +1179,14 @@  static int i8042_pm_thaw(struct device *dev)
 	return 0;
 }

+static int i8042_pm_restore(struct device *dev)
+{
+	return i8042_controller_resume(false);
+}
+
 static const struct dev_pm_ops i8042_pm_ops = {
 	.suspend	= i8042_pm_reset,
-	.resume		= i8042_pm_restore,
+	.resume		= i8042_pm_resume,
 	.thaw		= i8042_pm_thaw,
 	.poweroff	= i8042_pm_reset,
 	.restore	= i8042_pm_restore,
@@ -1389,9 +1404,11 @@  static int __init i8042_probe(struct
platform_device *dev)

 	i8042_platform_device = dev;

-	error = i8042_controller_selftest();
-	if (error)
-		return error;
+	if (i8042_reset) {
+		error = i8042_controller_selftest();
+		if (error)
+			return error;
+	}

 	error = i8042_controller_init();