[PATCH v4 0/2] i8042: Add forcenorestore quirk to leave controller

[PATCH v4 0/2] i8042: Add forcenorestore quirk to leave controller

[Werner Sembach]

v2: Just a typo fix in commit message.
v3: Another typo fix in commit message.
v4: Spotted another typo. Note to self: Don't write commit messages when tired.

[PATCH v4 1/2] i8042: Add forcenorestore quirk to leave controller untouched even on s3

[Werner Sembach]

On s3 resume the i8042 driver tries to restore the controller to a known
state by reinitializing things, however this can confuse the controller
with different effects. Mostly occasionally unresponsive keyboards after
resume.

These issues do not rise on s0ix resume as here the controller is assumed
to preserved its state from before suspend.

This patch adds a quirk for devices where the reinitialization on s3 resume
is not needed and might be harmful as described above. It does this by
using the s0ix resume code path at selected locations.

This new quirk goes beyond what the preexisting reset=never quirk does,
which only skips some reinitialization steps.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Cc: stable@vger.kernel.org
---
 drivers/input/serio/i8042-acpipnpio.h | 10 +++++++---
 drivers/input/serio/i8042.c           | 10 +++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index b585b1dab870e..10ec4534e5e14 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -83,6 +83,7 @@ static inline void i8042_write_command(int val)
 #define SERIO_QUIRK_KBDRESET		BIT(12)
 #define SERIO_QUIRK_DRITEK		BIT(13)
 #define SERIO_QUIRK_NOPNP		BIT(14)
+#define SERIO_QUIRK_FORCENORESTORE	BIT(15)
 
 /* Quirk table for different mainboards. Options similar or identical to i8042
  * module parameters.
@@ -1657,6 +1658,8 @@ static void __init i8042_check_quirks(void)
 	if (quirks & SERIO_QUIRK_NOPNP)
 		i8042_nopnp = true;
 #endif
+	if (quirks & SERIO_QUIRK_FORCENORESTORE)
+		i8042_forcenorestore = true;
 }
 #else
 static inline void i8042_check_quirks(void) {}
@@ -1690,7 +1693,7 @@ static int __init i8042_platform_init(void)
 
 	i8042_check_quirks();
 
-	pr_debug("Active quirks (empty means none):%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
+	pr_debug("Active quirks (empty means none):%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
 		i8042_nokbd ? " nokbd" : "",
 		i8042_noaux ? " noaux" : "",
 		i8042_nomux ? " nomux" : "",
@@ -1710,10 +1713,11 @@ static int __init i8042_platform_init(void)
 		"",
 #endif
 #ifdef CONFIG_PNP
-		i8042_nopnp ? " nopnp" : "");
+		i8042_nopnp ? " nopnp" : "",
 #else
-		"");
+		"",
 #endif
+		i8042_forcenorestore ? " forcenorestore" : "");
 
 	retval = i8042_pnp_init();
 	if (retval)
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 9fbb8d31575ae..2233d93f90e81 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -115,6 +115,10 @@ module_param_named(nopnp, i8042_nopnp, bool, 0);
 MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
 #endif
 
+static bool i8042_forcenorestore;
+module_param_named(forcenorestore, i8042_forcenorestore, bool, 0);
+MODULE_PARM_DESC(forcenorestore, "Force no restore on s3 resume, copying s2idle behaviour");
+
 #define DEBUG
 #ifdef DEBUG
 static bool i8042_debug;
@@ -1232,7 +1236,7 @@ static int i8042_pm_suspend(struct device *dev)
 {
 	int i;
 
-	if (pm_suspend_via_firmware())
+	if (!i8042_forcenorestore && pm_suspend_via_firmware())
 		i8042_controller_reset(true);
 
 	/* Set up serio interrupts for system wakeup. */
@@ -1248,7 +1252,7 @@ static int i8042_pm_suspend(struct device *dev)
 
 static int i8042_pm_resume_noirq(struct device *dev)
 {
-	if (!pm_resume_via_firmware())
+	if (i8042_forcenorestore || !pm_resume_via_firmware())
 		i8042_interrupt(0, NULL);
 
 	return 0;
@@ -1271,7 +1275,7 @@ static int i8042_pm_resume(struct device *dev)
 	 * not restore the controller state to whatever it had been at boot
 	 * time, so we do not need to do anything.
 	 */
-	if (!pm_suspend_via_firmware())
+	if (i8042_forcenorestore || !pm_suspend_via_firmware())
 		return 0;
 
 	/*
-- 
2.34.1

Re: [PATCH v4 1/2] i8042: Add forcenorestore quirk to leave controller untouched even on s3

[Hans de Goede]

Hi,

On 1/4/24 19:31, Werner Sembach wrote:
> On s3 resume the i8042 driver tries to restore the controller to a known
> state by reinitializing things, however this can confuse the controller
> with different effects. Mostly occasionally unresponsive keyboards after
> resume.
> 
> These issues do not rise on s0ix resume as here the controller is assumed
> to preserved its state from before suspend.
> 
> This patch adds a quirk for devices where the reinitialization on s3 resume
> is not needed and might be harmful as described above. It does this by
> using the s0ix resume code path at selected locations.
> 
> This new quirk goes beyond what the preexisting reset=never quirk does,
> which only skips some reinitialization steps.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: stable@vger.kernel.org

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/input/serio/i8042-acpipnpio.h | 10 +++++++---
>  drivers/input/serio/i8042.c           | 10 +++++++---
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
> index b585b1dab870e..10ec4534e5e14 100644
> --- a/drivers/input/serio/i8042-acpipnpio.h
> +++ b/drivers/input/serio/i8042-acpipnpio.h
> @@ -83,6 +83,7 @@ static inline void i8042_write_command(int val)
>  #define SERIO_QUIRK_KBDRESET		BIT(12)
>  #define SERIO_QUIRK_DRITEK		BIT(13)
>  #define SERIO_QUIRK_NOPNP		BIT(14)
> +#define SERIO_QUIRK_FORCENORESTORE	BIT(15)
>  
>  /* Quirk table for different mainboards. Options similar or identical to i8042
>   * module parameters.
> @@ -1657,6 +1658,8 @@ static void __init i8042_check_quirks(void)
>  	if (quirks & SERIO_QUIRK_NOPNP)
>  		i8042_nopnp = true;
>  #endif
> +	if (quirks & SERIO_QUIRK_FORCENORESTORE)
> +		i8042_forcenorestore = true;
>  }
>  #else
>  static inline void i8042_check_quirks(void) {}
> @@ -1690,7 +1693,7 @@ static int __init i8042_platform_init(void)
>  
>  	i8042_check_quirks();
>  
> -	pr_debug("Active quirks (empty means none):%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
> +	pr_debug("Active quirks (empty means none):%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>  		i8042_nokbd ? " nokbd" : "",
>  		i8042_noaux ? " noaux" : "",
>  		i8042_nomux ? " nomux" : "",
> @@ -1710,10 +1713,11 @@ static int __init i8042_platform_init(void)
>  		"",
>  #endif
>  #ifdef CONFIG_PNP
> -		i8042_nopnp ? " nopnp" : "");
> +		i8042_nopnp ? " nopnp" : "",
>  #else
> -		"");
> +		"",
>  #endif
> +		i8042_forcenorestore ? " forcenorestore" : "");
>  
>  	retval = i8042_pnp_init();
>  	if (retval)
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 9fbb8d31575ae..2233d93f90e81 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -115,6 +115,10 @@ module_param_named(nopnp, i8042_nopnp, bool, 0);
>  MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
>  #endif
>  
> +static bool i8042_forcenorestore;
> +module_param_named(forcenorestore, i8042_forcenorestore, bool, 0);
> +MODULE_PARM_DESC(forcenorestore, "Force no restore on s3 resume, copying s2idle behaviour");
> +
>  #define DEBUG
>  #ifdef DEBUG
>  static bool i8042_debug;
> @@ -1232,7 +1236,7 @@ static int i8042_pm_suspend(struct device *dev)
>  {
>  	int i;
>  
> -	if (pm_suspend_via_firmware())
> +	if (!i8042_forcenorestore && pm_suspend_via_firmware())
>  		i8042_controller_reset(true);
>  
>  	/* Set up serio interrupts for system wakeup. */
> @@ -1248,7 +1252,7 @@ static int i8042_pm_suspend(struct device *dev)
>  
>  static int i8042_pm_resume_noirq(struct device *dev)
>  {
> -	if (!pm_resume_via_firmware())
> +	if (i8042_forcenorestore || !pm_resume_via_firmware())
>  		i8042_interrupt(0, NULL);
>  
>  	return 0;
> @@ -1271,7 +1275,7 @@ static int i8042_pm_resume(struct device *dev)
>  	 * not restore the controller state to whatever it had been at boot
>  	 * time, so we do not need to do anything.
>  	 */
> -	if (!pm_suspend_via_firmware())
> +	if (i8042_forcenorestore || !pm_suspend_via_firmware())
>  		return 0;
>  
>  	/*

Re: [PATCH v4 1/2] i8042: Add forcenorestore quirk to leave controller untouched even on s3

[Dmitry Torokhov]

Hi Werner,

On Thu, Jan 04, 2024 at 07:31:17PM +0100, Werner Sembach wrote:
> On s3 resume the i8042 driver tries to restore the controller to a known
> state by reinitializing things, however this can confuse the controller
> with different effects. Mostly occasionally unresponsive keyboards after
> resume.
> 
> These issues do not rise on s0ix resume as here the controller is assumed
> to preserved its state from before suspend.
> 
> This patch adds a quirk for devices where the reinitialization on s3 resume
> is not needed and might be harmful as described above. It does this by
> using the s0ix resume code path at selected locations.
> 
> This new quirk goes beyond what the preexisting reset=never quirk does,
> which only skips some reinitialization steps.

I think the original change mentioned not only issues on resume, but
also after boot, which this one does not address, at least directly, so
I am not sure if this patch is the proper replacement.

I would also like to understand better what exact step is troublesome,
as I would be surprised if any interaction with the keyboard
controller while suspending causes the issue to manifest. Is it enough,
by chance, to skip restoring MUX mode and reset?

Also, shoudl this system use s2idle by chance?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/2] i8042: Add forcenorestore quirk to leave controller untouched even on s3

[Werner Sembach]

Am 09.01.24 um 22:48 schrieb Dmitry Torokhov:
> Hi Werner,
>
> On Thu, Jan 04, 2024 at 07:31:17PM +0100, Werner Sembach wrote:
>> On s3 resume the i8042 driver tries to restore the controller to a known
>> state by reinitializing things, however this can confuse the controller
>> with different effects. Mostly occasionally unresponsive keyboards after
>> resume.
>>
>> These issues do not rise on s0ix resume as here the controller is assumed
>> to preserved its state from before suspend.
>>
>> This patch adds a quirk for devices where the reinitialization on s3 resume
>> is not needed and might be harmful as described above. It does this by
>> using the s0ix resume code path at selected locations.
>>
>> This new quirk goes beyond what the preexisting reset=never quirk does,
>> which only skips some reinitialization steps.
> I think the original change mentioned not only issues on resume, but
> also after boot, which this one does not address, at least directly, so
> I am not sure if this patch is the proper replacement.

The original change introduced issues after boot and fixed issues after resume.

The new quirk fixes the issues after resume without introducing issues after boot.

The issues after boot where only affecting the NHxxRZQ and the N1xxCU iirc.

>
> I would also like to understand better what exact step is troublesome,
> as I would be surprised if any interaction with the keyboard
> controller while suspending causes the issue to manifest. Is it enough,
> by chance, to skip restoring MUX mode and reset?

SERIO_QUIRK_NOMUX and SERIO_QUIRK_RESET_ALWAYS are required in the old fix for 
the resume issues to go away (I don't know if SERIO_QUIRK_NOLOOP and 
SERIO_QUIRK_NOPNP are required, I threw them in just in case because we were 
running them already on the device, so they where somewhat "field proven" to not 
break things).

However while SERIO_QUIRK_NOMUX and SERIO_QUIRK_RESET_ALWAYS for themself did 
not individually introduce the boot problem, in combination they did.

That was all I was able to find out back when I tested the old quirks.

>
> Also, shoudl this system use s2idle by chance?
N1xxCU is s3 only (intel 10th gen clevo)
>
> Thanks.
>

Re: [PATCH v4 1/2] i8042: Add forcenorestore quirk to leave controller untouched even on s3

[Werner Sembach]

Hi,

gentle bump to this patch, still applies to v6.11-rc2

Kind regards,

Werner

Re: [PATCH v4 1/2] i8042: Add forcenorestore quirk to leave controller untouched even on s3

[Dmitry Torokhov]

On Thu, Jan 04, 2024 at 07:31:17PM +0100, Werner Sembach wrote:
> On s3 resume the i8042 driver tries to restore the controller to a known
> state by reinitializing things, however this can confuse the controller
> with different effects. Mostly occasionally unresponsive keyboards after
> resume.
> 
> These issues do not rise on s0ix resume as here the controller is assumed
> to preserved its state from before suspend.
> 
> This patch adds a quirk for devices where the reinitialization on s3 resume
> is not needed and might be harmful as described above. It does this by
> using the s0ix resume code path at selected locations.
> 
> This new quirk goes beyond what the preexisting reset=never quirk does,
> which only skips some reinitialization steps.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: stable@vger.kernel.org

Applied, thank you.

-- 
Dmitry

[PATCH v4 2/2] i8042: Use new forcenorestore quirk to replace old buggy quirk combination

[Werner Sembach]

The old quirk combination sometimes cause a laggy keyboard after boot. With
the new quirk the initial issue of an unresponsive keyboard after s3 resume
is also fixed, but it doesn't have the negative side effect of the
sometimes laggy keyboard.

Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Cc: stable@vger.kernel.org
---
 drivers/input/serio/i8042-acpipnpio.h | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index 10ec4534e5e14..e631a26394e92 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -1142,18 +1142,10 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
 					SERIO_QUIRK_NOLOOP | SERIO_QUIRK_NOPNP)
 	},
 	{
-		/*
-		 * Setting SERIO_QUIRK_NOMUX or SERIO_QUIRK_RESET_ALWAYS makes
-		 * the keyboard very laggy for ~5 seconds after boot and
-		 * sometimes also after resume.
-		 * However both are required for the keyboard to not fail
-		 * completely sometimes after boot or resume.
-		 */
 		.matches = {
 			DMI_MATCH(DMI_BOARD_NAME, "N150CU"),
 		},
-		.driver_data = (void *)(SERIO_QUIRK_NOMUX | SERIO_QUIRK_RESET_ALWAYS |
-					SERIO_QUIRK_NOLOOP | SERIO_QUIRK_NOPNP)
+		.driver_data = (void *)(SERIO_QUIRK_FORCENORESTORE)
 	},
 	{
 		.matches = {
-- 
2.34.1

Re: [PATCH v4 2/2] i8042: Use new forcenorestore quirk to replace old buggy quirk combination

[Hans de Goede]

Hi,

On 1/4/24 19:31, Werner Sembach wrote:
> The old quirk combination sometimes cause a laggy keyboard after boot. With
> the new quirk the initial issue of an unresponsive keyboard after s3 resume
> is also fixed, but it doesn't have the negative side effect of the
> sometimes laggy keyboard.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: stable@vger.kernel.org

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/input/serio/i8042-acpipnpio.h | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
> index 10ec4534e5e14..e631a26394e92 100644
> --- a/drivers/input/serio/i8042-acpipnpio.h
> +++ b/drivers/input/serio/i8042-acpipnpio.h
> @@ -1142,18 +1142,10 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
>  					SERIO_QUIRK_NOLOOP | SERIO_QUIRK_NOPNP)
>  	},
>  	{
> -		/*
> -		 * Setting SERIO_QUIRK_NOMUX or SERIO_QUIRK_RESET_ALWAYS makes
> -		 * the keyboard very laggy for ~5 seconds after boot and
> -		 * sometimes also after resume.
> -		 * However both are required for the keyboard to not fail
> -		 * completely sometimes after boot or resume.
> -		 */
>  		.matches = {
>  			DMI_MATCH(DMI_BOARD_NAME, "N150CU"),
>  		},
> -		.driver_data = (void *)(SERIO_QUIRK_NOMUX | SERIO_QUIRK_RESET_ALWAYS |
> -					SERIO_QUIRK_NOLOOP | SERIO_QUIRK_NOPNP)
> +		.driver_data = (void *)(SERIO_QUIRK_FORCENORESTORE)
>  	},
>  	{
>  		.matches = {

Re: [PATCH v4 2/2] i8042: Use new forcenorestore quirk to replace old buggy quirk combination

[Dmitry Torokhov]

On Thu, Jan 04, 2024 at 07:31:18PM +0100, Werner Sembach wrote:
> The old quirk combination sometimes cause a laggy keyboard after boot. With
> the new quirk the initial issue of an unresponsive keyboard after s3 resume
> is also fixed, but it doesn't have the negative side effect of the
> sometimes laggy keyboard.
> 
> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> Cc: stable@vger.kernel.org

Applied, thank you.

-- 
Dmitry