diff mbox series

[v3] i8042: enable keyboard wakeups by default when s2idle is used

Message ID 20180911012901.3928-1-drake@endlessm.com (mailing list archive)
State Not Applicable, archived
Headers show
Series [v3] i8042: enable keyboard wakeups by default when s2idle is used | expand

Commit Message

Daniel Drake Sept. 11, 2018, 1:29 a.m. UTC 
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 WhiskyLake,
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.

Link: https://lkml.kernel.org/r/CAB4CAwfQ0mPMqCLp95TVjw4J0r5zKPWkSvvkK4cpZUGE--w8bQ@mail.gmail.com
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Daniel Drake <drake@endlessm.com>
---
 drivers/input/serio/i8042.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

v2: tweak comment as suggested by Rafael

v3: fix checkpatch line length warning

Comments

Rafael J. Wysocki Sept. 11, 2018, 7:53 a.m. UTC | #1 
On Tue, Sep 11, 2018 at 3:29 AM Daniel Drake <drake@endlessm.com> wrote:
>
> 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 WhiskyLake,
> 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.
>
> Link: https://lkml.kernel.org/r/CAB4CAwfQ0mPMqCLp95TVjw4J0r5zKPWkSvvkK4cpZUGE--w8bQ@mail.gmail.com
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Daniel Drake <drake@endlessm.com>
> ---
>  drivers/input/serio/i8042.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> v2: tweak comment as suggested by Rafael
>
> v3: fix checkpatch line length warning
>
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index b8bc71569349..aa2f9e9521f3 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -1395,15 +1395,27 @@ 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 by default, make the
> +                * keyboard wake up the system from sleep by enabling keyboard
> +                * wakeups by default.  That is consistent with keyboard
> +                * wakeup behavior on many platforms using suspend-to-RAM
> +                * (ACPI S3) by default.
> +                */
> +               if (mem_sleep_current == PM_SUSPEND_TO_IDLE

On a second thought, it may be better to use mem_sleep_default here.

And you need to provide an empty stub for it for !CONFIG_SUSPEND.

It may be even better to have a wrapper function around it, something
like pm_default_s2idle() maybe?

> +                   && i == I8042_KBD_PORT_NO)
> +                       device_set_wakeup_enable(&serio->dev, true);
>         }
>  }
>
> --
Daniel Drake Sept. 14, 2018, 6:49 a.m. UTC | #2 
On Tue, Sep 11, 2018 at 3:53 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On a second thought, it may be better to use mem_sleep_default here.

It looks like mem_sleep_default will ordinarily be PM_SUSPEND_MAX on a
s2idle system.

The acpi/sleep.c code that deals with the lps0 device just does this:

        /*
         * Use suspend-to-idle by default if the default
         * suspend mode was not set from the command line.
         */
        if (mem_sleep_default > PM_SUSPEND_MEM)
            mem_sleep_current = PM_SUSPEND_TO_IDLE;

> And you need to provide an empty stub for it for !CONFIG_SUSPEND.
>
> It may be even better to have a wrapper function around it, something
> like pm_default_s2idle() maybe?

A wrapper sounds good. Would you prefer that to be introduced in a
separate patch, or should I roll it into this patch?

Daniel
Rafael J. Wysocki Sept. 14, 2018, 6:53 a.m. UTC | #3 
On Fri, Sep 14, 2018 at 8:49 AM Daniel Drake <drake@endlessm.com> wrote:
>
> On Tue, Sep 11, 2018 at 3:53 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On a second thought, it may be better to use mem_sleep_default here.
>
> It looks like mem_sleep_default will ordinarily be PM_SUSPEND_MAX on a
> s2idle system.
>
> The acpi/sleep.c code that deals with the lps0 device just does this:
>
>         /*
>          * Use suspend-to-idle by default if the default
>          * suspend mode was not set from the command line.
>          */
>         if (mem_sleep_default > PM_SUSPEND_MEM)
>             mem_sleep_current = PM_SUSPEND_TO_IDLE;

Right, sorry.

> > And you need to provide an empty stub for it for !CONFIG_SUSPEND.
> >
> > It may be even better to have a wrapper function around it, something
> > like pm_default_s2idle() maybe?
>
> A wrapper sounds good. Would you prefer that to be introduced in a
> separate patch, or should I roll it into this patch?

I think it can go into this patch.  After all it is needed because of
the other changes here.

Thanks,
Rafael
kernel test robot Sept. 20, 2018, 1:29 a.m. UTC | #4 
Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on input/next]
[also build test ERROR on v4.19-rc4 next-20180919]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Drake/i8042-enable-keyboard-wakeups-by-default-when-s2idle-is-used/20180911-173115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: x86_64-randconfig-f0-09171244 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/input/serio/i8042.c: In function 'i8042_register_ports':
>> drivers/input/serio/i8042.c:1416:7: error: 'mem_sleep_current' undeclared (first use in this function)
      if (mem_sleep_current == PM_SUSPEND_TO_IDLE
          ^~~~~~~~~~~~~~~~~
   drivers/input/serio/i8042.c:1416:7: note: each undeclared identifier is reported only once for each function it appears in

vim +/mem_sleep_current +1416 drivers/input/serio/i8042.c

  1390	
  1391	static void __init i8042_register_ports(void)
  1392	{
  1393		int i;
  1394	
  1395		for (i = 0; i < I8042_NUM_PORTS; i++) {
  1396			struct serio *serio = i8042_ports[i].serio;
  1397	
  1398			if (!serio)
  1399				continue;
  1400	
  1401			printk(KERN_INFO "serio: %s at %#lx,%#lx irq %d\n",
  1402				serio->name,
  1403				(unsigned long) I8042_DATA_REG,
  1404				(unsigned long) I8042_COMMAND_REG,
  1405				i8042_ports[i].irq);
  1406			serio_register_port(serio);
  1407			device_set_wakeup_capable(&serio->dev, true);
  1408	
  1409			/*
  1410			 * On platforms using suspend-to-idle by default, make the
  1411			 * keyboard wake up the system from sleep by enabling keyboard
  1412			 * wakeups by default.  That is consistent with keyboard
  1413			 * wakeup behavior on many platforms using suspend-to-RAM
  1414			 * (ACPI S3) by default.
  1415			 */
> 1416			if (mem_sleep_current == PM_SUSPEND_TO_IDLE
  1417			    && i == I8042_KBD_PORT_NO)
  1418				device_set_wakeup_enable(&serio->dev, true);
  1419		}
  1420	}
  1421	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index b8bc71569349..aa2f9e9521f3 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1395,15 +1395,27 @@  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 by default, make the
+		 * keyboard wake up the system from sleep by enabling keyboard
+		 * wakeups by default.  That is consistent with keyboard
+		 * wakeup behavior on many platforms using suspend-to-RAM
+		 * (ACPI S3) by default.
+		 */
+		if (mem_sleep_current == PM_SUSPEND_TO_IDLE
+		    && i == I8042_KBD_PORT_NO)
+			device_set_wakeup_enable(&serio->dev, true);
 	}
 }