Re: [PATCH AUTOSEL 4.18 04/27] Input: i8042 - enable keyboard wakeups by default when s2idle is used

[Dmitry Torokhov]

Hi Sasha,

On Mon, Oct 22, 2018 at 06:19:01AM -0400, Sasha Levin wrote:
> From: Daniel Drake <drake@xxxxxxxxxxxx>
> 
> [ Upstream commit 684bec1092b6991ff2a7751e8a763898576eb5c2 ]
> 
> Previously, on typical consumer laptops, pressing a key on the keyboard
> when the system is in suspend would cause it to wake up (default or
> unconditional behaviour). This happens because the EC generates a SCI
> interrupt in this scenario.
> 
> That is no longer true on modern laptops based on Intel WhiskeyLake,
> including Acer Swift SF314-55G, Asus UX333FA, Asus UX433FN and Asus
> UX533FD. We confirmed with Asus EC engineers that the "Modern Standby"
> design has been modified so that the EC no longer generates a SCI
> in this case; the keyboard controller itself should be used for wakeup.
> 
> In order to retain the standard behaviour of being able to use the
> keyboard to wake up the system, enable serio wakeups by default on
> platforms that are using s2idle.

I disagree that this should be included in stable as it does not fix a
regression on the aforementioned platforms and has a potential to alter
the behavior. The fact that the system does not wake up on keyboard is
not a showstopper, wakeup usually happens on lid open anyway.

It is hitting 4.19 LTS train and that should be enough. Please do not
pick up for earlier stables.

Thanks.

> 
> Link: https://lkml.kernel.org/r/CAB4CAwfQ0mPMqCLp95TVjw4J0r5zKPWkSvvkK4cpZUGE--w8bQ@xxxxxxxxxxxxxx
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Signed-off-by: Daniel Drake <drake@xxxxxxxxxxxx>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  drivers/input/serio/i8042.c | 29 ++++++++++++++++++++---------
>  include/linux/suspend.h     |  2 ++
>  kernel/power/suspend.c      |  6 ++++++
>  3 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 824f4c1c1f31..6462f1798fbb 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -1392,15 +1392,26 @@ static void __init i8042_register_ports(void)
>  	for (i = 0; i < I8042_NUM_PORTS; i++) {
>  		struct serio *serio = i8042_ports[i].serio;
>  
> -		if (serio) {
> -			printk(KERN_INFO "serio: %s at %#lx,%#lx irq %d\n",
> -				serio->name,
> -				(unsigned long) I8042_DATA_REG,
> -				(unsigned long) I8042_COMMAND_REG,
> -				i8042_ports[i].irq);
> -			serio_register_port(serio);
> -			device_set_wakeup_capable(&serio->dev, true);
> -		}
> +		if (!serio)
> +			continue;
> +
> +		printk(KERN_INFO "serio: %s at %#lx,%#lx irq %d\n",
> +			serio->name,
> +			(unsigned long) I8042_DATA_REG,
> +			(unsigned long) I8042_COMMAND_REG,
> +			i8042_ports[i].irq);
> +		serio_register_port(serio);
> +		device_set_wakeup_capable(&serio->dev, true);
> +
> +		/*
> +		 * On platforms using suspend-to-idle, allow the keyboard to
> +		 * wake up the system from sleep by enabling keyboard wakeups
> +		 * by default.  This is consistent with keyboard wakeup
> +		 * behavior on many platforms using suspend-to-RAM (ACPI S3)
> +		 * by default.
> +		 */
> +		if (pm_suspend_via_s2idle() && i == I8042_KBD_PORT_NO)
> +			device_set_wakeup_enable(&serio->dev, true);
>  	}
>  }
>  
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 440b62f7502e..206b735f383f 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -251,6 +251,7 @@ static inline bool idle_should_enter_s2idle(void)
>  	return unlikely(s2idle_state == S2IDLE_STATE_ENTER);
>  }
>  
> +extern bool pm_suspend_via_s2idle(void);
>  extern void __init pm_states_init(void);
>  extern void s2idle_set_ops(const struct platform_s2idle_ops *ops);
>  extern void s2idle_wake(void);
> @@ -282,6 +283,7 @@ static inline void pm_set_suspend_via_firmware(void) {}
>  static inline void pm_set_resume_via_firmware(void) {}
>  static inline bool pm_suspend_via_firmware(void) { return false; }
>  static inline bool pm_resume_via_firmware(void) { return false; }
> +static inline bool pm_suspend_via_s2idle(void) { return false; }
>  
>  static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
>  static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 87331565e505..3eeafaff190a 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -63,6 +63,12 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>  enum s2idle_states __read_mostly s2idle_state;
>  static DEFINE_RAW_SPINLOCK(s2idle_lock);
>  
> +bool pm_suspend_via_s2idle(void)
> +{
> +	return mem_sleep_current == PM_SUSPEND_TO_IDLE;
> +}
> +EXPORT_SYMBOL_GPL(pm_suspend_via_s2idle);
> +
>  void s2idle_set_ops(const struct platform_s2idle_ops *ops)
>  {
>  	lock_system_sleep();
> -- 
> 2.17.1
> 

-- 
Dmitry