[PATCH 0/2] Suspend-to-idle fixes for Surface Pro3

[PATCH 0/2] Suspend-to-idle fixes for Surface Pro3

[Rafael J. Wysocki]

Hi All,

After some changes made during the 4.13 cycle, power button doesn't wake up
the system from suspend-to-idle any more on Surface Pro3.

It turns out that the platform expects EC events to be processed while
suspended for this to work (patch [1/2]) and the button driver needs to
report wakeup events properly too (patch [2/2]).

Thanks,
Rafael

[PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
functions in their Low Power S0 Idle _DSM, but still expect EC
events to be processed in the suspend-to-idle state for power button
wakeup (among other things) to work.  Surface Pro3 turns out to be
one of them.

Fortunately, it still provides Low Power S0 Idle _DSM with the screen
on/off functions supported, so modify the ACPI suspend-to-idle to use
the Low Power S0 Idle code path for all systems supporting the
ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.

Potentially, that will cause more systems to use suspend-to-idle by
default, so some future corrections may be necessary if it leads
to issues, but let it remain more straightforward for now.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389
Reported-by: Valentin Manea <valy@mrs.ro>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/sleep.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -707,7 +707,8 @@ static const struct acpi_device_id lps0_
 #define ACPI_LPS0_ENTRY		5
 #define ACPI_LPS0_EXIT		6
 
-#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
+#define ACPI_LPS0_SCREEN_MASK	((1 << ACPI_LPS0_SCREEN_OFF) | (1 << ACPI_LPS0_SCREEN_ON))
+#define ACPI_LPS0_S2I_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
 
 static acpi_handle lps0_device_handle;
 static guid_t lps0_dsm_guid;
@@ -910,7 +911,8 @@ static int lps0_device_attach(struct acp
 	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
 		char bitmask = *(char *)out_obj->buffer.pointer;
 
-		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) == ACPI_S2IDLE_FUNC_MASK) {
+		if ((bitmask & ACPI_LPS0_S2I_MASK) == ACPI_LPS0_S2I_MASK ||
+		    (bitmask & ACPI_LPS0_SCREEN_MASK) == ACPI_LPS0_SCREEN_MASK) {
 			lps0_dsm_func_mask = bitmask;
 			lps0_device_handle = adev->handle;
 			/*

Re: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Andy Shevchenko]

On Wed, 2018-01-10 at 13:26 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
> functions in their Low Power S0 Idle _DSM, but still expect EC
> events to be processed in the suspend-to-idle state for power button
> wakeup (among other things) to work.  Surface Pro3 turns out to be
> one of them.
> 
> Fortunately, it still provides Low Power S0 Idle _DSM with the screen
> on/off functions supported, so modify the ACPI suspend-to-idle to use
> the Low Power S0 Idle code path for all systems supporting the
> ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
> ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.
> 
> Potentially, that will cause more systems to use suspend-to-idle by
> default, so some future corrections may be necessary if it leads
> to issues, but let it remain more straightforward for now.
 
> -#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> +#define ACPI_LPS0_SCREEN_MASK	((1 << ACPI_LPS0_SCREEN_OFF) |
> (1 << ACPI_LPS0_SCREEN_ON))

> +#define ACPI_LPS0_S2I_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))

Just a nitpick: Can we leave S2IDLE instead of S2I?
Would it make sense for potential code readers?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Rafael J. Wysocki]

On Wed, Jan 10, 2018 at 2:24 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, 2018-01-10 at 13:26 +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
>> functions in their Low Power S0 Idle _DSM, but still expect EC
>> events to be processed in the suspend-to-idle state for power button
>> wakeup (among other things) to work.  Surface Pro3 turns out to be
>> one of them.
>>
>> Fortunately, it still provides Low Power S0 Idle _DSM with the screen
>> on/off functions supported, so modify the ACPI suspend-to-idle to use
>> the Low Power S0 Idle code path for all systems supporting the
>> ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
>> ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.
>>
>> Potentially, that will cause more systems to use suspend-to-idle by
>> default, so some future corrections may be necessary if it leads
>> to issues, but let it remain more straightforward for now.
>
>> -#define ACPI_S2IDLE_FUNC_MASK        ((1 << ACPI_LPS0_ENTRY) | (1 <<
>> ACPI_LPS0_EXIT))
>> +#define ACPI_LPS0_SCREEN_MASK        ((1 << ACPI_LPS0_SCREEN_OFF) |
>> (1 << ACPI_LPS0_SCREEN_ON))
>
>> +#define ACPI_LPS0_S2I_MASK   ((1 << ACPI_LPS0_ENTRY) | (1 <<
>> ACPI_LPS0_EXIT))
>
> Just a nitpick: Can we leave S2IDLE instead of S2I?
> Would it make sense for potential code readers?

I wanted it to be shorter, but if that is a problem, I'd rather call
it PLATFORM than S2IDLE (as technically they are related to the
low-power mode of the platform).

I'll send an update shortly.

Re: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Andy Shevchenko]

On Wed, 2018-01-10 at 23:25 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 10, 2018 at 2:24 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, 2018-01-10 at 13:26 +0100, Rafael J. Wysocki wrote:

> > Just a nitpick: Can we leave S2IDLE instead of S2I?
> > Would it make sense for potential code readers?
> 
> I wanted it to be shorter, but if that is a problem, I'd rather call
> it PLATFORM than S2IDLE (as technically they are related to the
> low-power mode of the platform).

It's not problem per se, though without a context it would take time to
get into S2I acronym from the code for not familiar reader.

> I'll send an update shortly.

Thanks!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v2 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
functions in their Low Power S0 Idle _DSM, but still expect EC
events to be processed in the suspend-to-idle state for power button
wakeup (among other things) to work.  Surface Pro3 turns out to be
one of them.

Fortunately, it still provides Low Power S0 Idle _DSM with the screen
on/off functions supported, so modify the ACPI suspend-to-idle to use
the Low Power S0 Idle code path for all systems supporting the
ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.

Potentially, that will cause more systems to use suspend-to-idle by
default, so some future corrections may be necessary if it leads
to issues, but let it remain more straightforward for now.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389#add_comment
Reported-by: Valentin Manea <valy@mrs.ro>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Rename ACPI_LPS0_S2I_MASK to ACPI_LPS0_PLATFORM_MASK.

---
 drivers/acpi/sleep.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -707,7 +707,8 @@ static const struct acpi_device_id lps0_
 #define ACPI_LPS0_ENTRY		5
 #define ACPI_LPS0_EXIT		6
 
-#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
+#define ACPI_LPS0_SCREEN_MASK	((1 << ACPI_LPS0_SCREEN_OFF) | (1 << ACPI_LPS0_SCREEN_ON))
+#define ACPI_LPS0_PLATFORM_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
 
 static acpi_handle lps0_device_handle;
 static guid_t lps0_dsm_guid;
@@ -910,7 +911,8 @@ static int lps0_device_attach(struct acp
 	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
 		char bitmask = *(char *)out_obj->buffer.pointer;
 
-		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) == ACPI_S2IDLE_FUNC_MASK) {
+		if ((bitmask & ACPI_LPS0_PLATFORM_MASK) == ACPI_LPS0_PLATFORM_MASK ||
+		    (bitmask & ACPI_LPS0_SCREEN_MASK) == ACPI_LPS0_SCREEN_MASK) {
 			lps0_dsm_func_mask = bitmask;
 			lps0_device_handle = adev->handle;
 			/*

Re: [PATCH v2 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Andy Shevchenko]

On Thu, 2018-01-11 at 02:13 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
> functions in their Low Power S0 Idle _DSM, but still expect EC
> events to be processed in the suspend-to-idle state for power button
> wakeup (among other things) to work.  Surface Pro3 turns out to be
> one of them.
> 
> Fortunately, it still provides Low Power S0 Idle _DSM with the screen
> on/off functions supported, so modify the ACPI suspend-to-idle to use
> the Low Power S0 Idle code path for all systems supporting the
> ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
> ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.
> 
> Potentially, that will cause more systems to use suspend-to-idle by
> default, so some future corrections may be necessary if it leads
> to issues, but let it remain more straightforward for now.
> 

FWIW, 
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389#add_comment
> Reported-by: Valentin Manea <valy@mrs.ro>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> 
> -> v2: Rename ACPI_LPS0_S2I_MASK to ACPI_LPS0_PLATFORM_MASK.
> 
> ---
>  drivers/acpi/sleep.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -707,7 +707,8 @@ static const struct acpi_device_id lps0_
>  #define ACPI_LPS0_ENTRY		5
>  #define ACPI_LPS0_EXIT		6
>  
> -#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> +#define ACPI_LPS0_SCREEN_MASK	((1 << ACPI_LPS0_SCREEN_OFF) |
> (1 << ACPI_LPS0_SCREEN_ON))
> +#define ACPI_LPS0_PLATFORM_MASK	((1 << ACPI_LPS0_ENTRY) | (1
> << ACPI_LPS0_EXIT))
>  
>  static acpi_handle lps0_device_handle;
>  static guid_t lps0_dsm_guid;
> @@ -910,7 +911,8 @@ static int lps0_device_attach(struct acp
>  	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
>  		char bitmask = *(char *)out_obj->buffer.pointer;
>  
> -		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> ACPI_S2IDLE_FUNC_MASK) {
> +		if ((bitmask & ACPI_LPS0_PLATFORM_MASK) ==
> ACPI_LPS0_PLATFORM_MASK ||
> +		    (bitmask & ACPI_LPS0_SCREEN_MASK) ==
> ACPI_LPS0_SCREEN_MASK) {
>  			lps0_dsm_func_mask = bitmask;
>  			lps0_device_handle = adev->handle;
>  			/*
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

RE: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Wednesday, January 10, 2018 6:26 AM
> To: Linux ACPI <linux-acpi@vger.kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux PM <linux-
> pm@vger.kernel.org>; Platform Driver <platform-driver-x86@vger.kernel.org>;
> Valentin Manea <valy@mrs.ro>
> Subject: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
> functions in their Low Power S0 Idle _DSM, but still expect EC
> events to be processed in the suspend-to-idle state for power button
> wakeup (among other things) to work.  Surface Pro3 turns out to be
> one of them.
> 
> Fortunately, it still provides Low Power S0 Idle _DSM with the screen
> on/off functions supported, so modify the ACPI suspend-to-idle to use
> the Low Power S0 Idle code path for all systems supporting the
> ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
> ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.
> 
> Potentially, that will cause more systems to use suspend-to-idle by
> default, so some future corrections may be necessary if it leads
> to issues, but let it remain more straightforward for now.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389
> Reported-by: Valentin Manea <valy@mrs.ro>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/sleep.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -707,7 +707,8 @@ static const struct acpi_device_id lps0_
>  #define ACPI_LPS0_ENTRY		5
>  #define ACPI_LPS0_EXIT		6
> 
> -#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> +#define ACPI_LPS0_SCREEN_MASK	((1 << ACPI_LPS0_SCREEN_OFF) | (1 <<
> ACPI_LPS0_SCREEN_ON))
> +#define ACPI_LPS0_S2I_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
> 
>  static acpi_handle lps0_device_handle;
>  static guid_t lps0_dsm_guid;
> @@ -910,7 +911,8 @@ static int lps0_device_attach(struct acp
>  	if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
>  		char bitmask = *(char *)out_obj->buffer.pointer;
> 
> -		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> ACPI_S2IDLE_FUNC_MASK) {
> +		if ((bitmask & ACPI_LPS0_S2I_MASK) == ACPI_LPS0_S2I_MASK ||
> +		    (bitmask & ACPI_LPS0_SCREEN_MASK) ==
> ACPI_LPS0_SCREEN_MASK) {
>  			lps0_dsm_func_mask = bitmask;
>  			lps0_device_handle = adev->handle;
>  			/*

In making this change I believe you'll need to cache the values that you found from the
function mask to test them later too.
Here:
https://github.com/torvalds/linux/blob/master/drivers/acpi/sleep.c#L943

This is because later on both ACPI_LPS0_SCREEN_OFF and ACPI_LPS0_ENTRY are called
whether or not they both exist.  Fortunately looking at the DSDT in the attached bug nothing
happens if calling the undefined Arg2 == 0x05/Arg2 == 0x06 but that might not always
be the case.

Re: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Rafael J. Wysocki]

On Wed, Jan 10, 2018 at 6:38 PM,  <Mario.Limonciello@dell.com> wrote:
>
>
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86-
>> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
>> Sent: Wednesday, January 10, 2018 6:26 AM
>> To: Linux ACPI <linux-acpi@vger.kernel.org>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
>> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux PM <linux-
>> pm@vger.kernel.org>; Platform Driver <platform-driver-x86@vger.kernel.org>;
>> Valentin Manea <valy@mrs.ro>
>> Subject: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
>> functions in their Low Power S0 Idle _DSM, but still expect EC
>> events to be processed in the suspend-to-idle state for power button
>> wakeup (among other things) to work.  Surface Pro3 turns out to be
>> one of them.
>>
>> Fortunately, it still provides Low Power S0 Idle _DSM with the screen
>> on/off functions supported, so modify the ACPI suspend-to-idle to use
>> the Low Power S0 Idle code path for all systems supporting the
>> ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
>> ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.
>>
>> Potentially, that will cause more systems to use suspend-to-idle by
>> default, so some future corrections may be necessary if it leads
>> to issues, but let it remain more straightforward for now.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389
>> Reported-by: Valentin Manea <valy@mrs.ro>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/acpi/sleep.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/sleep.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/sleep.c
>> +++ linux-pm/drivers/acpi/sleep.c
>> @@ -707,7 +707,8 @@ static const struct acpi_device_id lps0_
>>  #define ACPI_LPS0_ENTRY              5
>>  #define ACPI_LPS0_EXIT               6
>>
>> -#define ACPI_S2IDLE_FUNC_MASK        ((1 << ACPI_LPS0_ENTRY) | (1 <<
>> ACPI_LPS0_EXIT))
>> +#define ACPI_LPS0_SCREEN_MASK        ((1 << ACPI_LPS0_SCREEN_OFF) | (1 <<
>> ACPI_LPS0_SCREEN_ON))
>> +#define ACPI_LPS0_S2I_MASK   ((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
>>
>>  static acpi_handle lps0_device_handle;
>>  static guid_t lps0_dsm_guid;
>> @@ -910,7 +911,8 @@ static int lps0_device_attach(struct acp
>>       if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
>>               char bitmask = *(char *)out_obj->buffer.pointer;
>>
>> -             if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
>> ACPI_S2IDLE_FUNC_MASK) {
>> +             if ((bitmask & ACPI_LPS0_S2I_MASK) == ACPI_LPS0_S2I_MASK ||
>> +                 (bitmask & ACPI_LPS0_SCREEN_MASK) ==
>> ACPI_LPS0_SCREEN_MASK) {
>>                       lps0_dsm_func_mask = bitmask;
>>                       lps0_device_handle = adev->handle;
>>                       /*
>
> In making this change I believe you'll need to cache the values that you found from the
> function mask to test them later too.

But that's what lps0_dsm_func_mask is for if I understand you correctly.

> Here:
> https://github.com/torvalds/linux/blob/master/drivers/acpi/sleep.c#L943
>
> This is because later on both ACPI_LPS0_SCREEN_OFF and ACPI_LPS0_ENTRY are called
> whether or not they both exist.

No, that's not the case.

acpi_sleep_run_lps0_dsm() checks if the given function is there in the
mask returned by function 0 and it doesn't evaluate the _DSM
otherwise.

Thanks,
Rafael

RE: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems

[Mario.Limonciello]

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J.
> Wysocki
> Sent: Wednesday, January 10, 2018 4:23 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> Darren Hart <dvhart@infradead.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Linux PM <linux-pm@vger.kernel.org>; Platform Driver
> <platform-driver-x86@vger.kernel.org>; valy@mrs.ro
> Subject: Re: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems
> 
> On Wed, Jan 10, 2018 at 6:38 PM,  <Mario.Limonciello@dell.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86-
> >> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> >> Sent: Wednesday, January 10, 2018 6:26 AM
> >> To: Linux ACPI <linux-acpi@vger.kernel.org>
> >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> >> <dvhart@infradead.org>; LKML <linux-kernel@vger.kernel.org>; Linux PM
> <linux-
> >> pm@vger.kernel.org>; Platform Driver <platform-driver-x86@vger.kernel.org>;
> >> Valentin Manea <valy@mrs.ro>
> >> Subject: [PATCH 1/2] ACPI / PM: Use Low Power S0 Idle on more systems
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> Some systems don't support the ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT
> >> functions in their Low Power S0 Idle _DSM, but still expect EC
> >> events to be processed in the suspend-to-idle state for power button
> >> wakeup (among other things) to work.  Surface Pro3 turns out to be
> >> one of them.
> >>
> >> Fortunately, it still provides Low Power S0 Idle _DSM with the screen
> >> on/off functions supported, so modify the ACPI suspend-to-idle to use
> >> the Low Power S0 Idle code path for all systems supporting the
> >> ACPI_LPS0_ENTRY and ACPI_LPS0_EXIT or the ACPI_LPS0_SCREEN_OFF and
> >> ACPI_LPS0_SCREEN_ON functions in their Low Power S0 Idle _DSM.
> >>
> >> Potentially, that will cause more systems to use suspend-to-idle by
> >> default, so some future corrections may be necessary if it leads
> >> to issues, but let it remain more straightforward for now.
> >>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389
> >> Reported-by: Valentin Manea <valy@mrs.ro>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  drivers/acpi/sleep.c |    6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> Index: linux-pm/drivers/acpi/sleep.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/acpi/sleep.c
> >> +++ linux-pm/drivers/acpi/sleep.c
> >> @@ -707,7 +707,8 @@ static const struct acpi_device_id lps0_
> >>  #define ACPI_LPS0_ENTRY              5
> >>  #define ACPI_LPS0_EXIT               6
> >>
> >> -#define ACPI_S2IDLE_FUNC_MASK        ((1 << ACPI_LPS0_ENTRY) | (1 <<
> >> ACPI_LPS0_EXIT))
> >> +#define ACPI_LPS0_SCREEN_MASK        ((1 << ACPI_LPS0_SCREEN_OFF) | (1 <<
> >> ACPI_LPS0_SCREEN_ON))
> >> +#define ACPI_LPS0_S2I_MASK   ((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> >>
> >>  static acpi_handle lps0_device_handle;
> >>  static guid_t lps0_dsm_guid;
> >> @@ -910,7 +911,8 @@ static int lps0_device_attach(struct acp
> >>       if (out_obj && out_obj->type == ACPI_TYPE_BUFFER) {
> >>               char bitmask = *(char *)out_obj->buffer.pointer;
> >>
> >> -             if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> >> ACPI_S2IDLE_FUNC_MASK) {
> >> +             if ((bitmask & ACPI_LPS0_S2I_MASK) == ACPI_LPS0_S2I_MASK ||
> >> +                 (bitmask & ACPI_LPS0_SCREEN_MASK) ==
> >> ACPI_LPS0_SCREEN_MASK) {
> >>                       lps0_dsm_func_mask = bitmask;
> >>                       lps0_device_handle = adev->handle;
> >>                       /*
> >
> > In making this change I believe you'll need to cache the values that you found
> from the
> > function mask to test them later too.
> 
> But that's what lps0_dsm_func_mask is for if I understand you correctly.
> 
> > Here:
> > https://github.com/torvalds/linux/blob/master/drivers/acpi/sleep.c#L943
> >
> > This is because later on both ACPI_LPS0_SCREEN_OFF and ACPI_LPS0_ENTRY are
> called
> > whether or not they both exist.
> 
> No, that's not the case.
> 
> acpi_sleep_run_lps0_dsm() checks if the given function is there in the
> mask returned by function 0 and it doesn't evaluate the _DSM
> otherwise.
> 
> Thanks,
> Rafael

Thanks yes, I see this more closely now you're right.

[PATCH 2/2] platform/x86: surfacepro3: Support for wakeup from suspend-to-idle

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify surface_button_notify() to make it wake up the system from
suspend-to-idle (by reporting "hard" wakeup events while suspended)
and add wakeup initialization to surface_button_add() for wakeup
events reported by this driver to work at all.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389
Reported-by: Valentin Manea <valy@mrs.ro>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/platform/x86/surfacepro3_button.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/platform/x86/surfacepro3_button.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/surfacepro3_button.c
+++ linux-pm/drivers/platform/x86/surfacepro3_button.c
@@ -119,7 +119,7 @@ static void surface_button_notify(struct
 	if (key_code == KEY_RESERVED)
 		return;
 	if (pressed)
-		pm_wakeup_event(&device->dev, 0);
+		pm_wakeup_dev_event(&device->dev, 0, button->suspended);
 	if (button->suspended)
 		return;
 	input_report_key(input, key_code, pressed?1:0);
@@ -185,6 +185,8 @@ static int surface_button_add(struct acp
 	error = input_register_device(input);
 	if (error)
 		goto err_free_input;
+
+	device_init_wakeup(&device->dev, true);
 	dev_info(&device->dev,
 			"%s [%s]\n", name, acpi_device_bid(device));
 	return 0;

Re: [PATCH 2/2] platform/x86: surfacepro3: Support for wakeup from suspend-to-idle

[Andy Shevchenko]

On Wed, 2018-01-10 at 13:26 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify surface_button_notify() to make it wake up the system from
> suspend-to-idle (by reporting "hard" wakeup events while suspended)
> and add wakeup initialization to surface_button_add() for wakeup
> events reported by this driver to work at all.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=198389
> Reported-by: Valentin Manea <valy@mrs.ro>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Change seems sane to me.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(in case you would like to push it through PM tree, otherwise I can take
it)

> ---
>  drivers/platform/x86/surfacepro3_button.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/drivers/platform/x86/surfacepro3_button.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/surfacepro3_button.c
> +++ linux-pm/drivers/platform/x86/surfacepro3_button.c
> @@ -119,7 +119,7 @@ static void surface_button_notify(struct
>  	if (key_code == KEY_RESERVED)
>  		return;
>  	if (pressed)
> -		pm_wakeup_event(&device->dev, 0);
> +		pm_wakeup_dev_event(&device->dev, 0, button-
> >suspended);
>  	if (button->suspended)
>  		return;
>  	input_report_key(input, key_code, pressed?1:0);
> @@ -185,6 +185,8 @@ static int surface_button_add(struct acp
>  	error = input_register_device(input);
>  	if (error)
>  		goto err_free_input;
> +
> +	device_init_wakeup(&device->dev, true);
>  	dev_info(&device->dev,
>  			"%s [%s]\n", name, acpi_device_bid(device));
>  	return 0;
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy