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@xxxxxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

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;
>  
>  	/*