diff mbox

[v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent systems

Message ID 2279758.LZ0rCGQtcH@aspire.rjw.lan (mailing list archive)
State Mainlined
Headers show

Commit Message

Rafael J. Wysocki June 23, 2017, 1:15 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Some recent Dell laptops, including the XPS13 model numbers 9360 and
9365, cannot be woken up from suspend-to-idle by pressing the power
button which is unexpected and makes that feature less usable on
those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
not expected to be used at all (the OS these systems ship with never
exercises the ACPI S3 path in the firmware) and suspend-to-idle is
the only viable system suspend mechanism there.

The reason why the power button wakeup from suspend-to-idle doesn't
work on those systems is because their power button events are
signaled by the EC (Embedded Controller), whose GPE (General Purpose
Event) line is disabled during suspend-to-idle transitions in Linux.
That is done on purpose, because in general the EC tends to be noisy
for various reasons (battery and thermal updates and similar, for
example) and all events signaled by it would kick the CPUs out of
deep idle states while in suspend-to-idle, which effectively might
defeat its purpose.

Of course, on the Dell systems in question the EC GPE must be enabled
during suspend-to-idle transitions for the button press events to
be signaled while suspended at all, but fortunately there is a way
out of this puzzle.

First of all, those systems have the ACPI_FADT_LOW_POWER_S0 flag set
in their ACPI tables, which means that the OS is expected to prefer
the "low power S0 idle" system state over ACPI S3 on them.  That
causes the most recent versions of other OSes to simply ignore ACPI
S3 on those systems, so it is reasonable to expect that it should not
be necessary to block GPEs during suspend-to-idle on them.

Second, in addition to that, the systems in question provide a special
firmware interface that can be used to indicate to the platform that
the OS is transitioning into a system-wide low-power state in which
certain types of activity are not desirable or that it is leaving
such a state and that (in principle) should allow the platform to
adjust its operation mode accordingly.

That interface is a special _DSM object under a System Power
Management Controller device (PNP0D80).  The expected way to use it
is to invoke function 0 from it on system initialization, functions
3 and 5 during suspend transitions and functions 4 and 6 during
resume transitions (to reverse the actions carried out by the
former).  In particular, function 5 from the "Low-Power S0" device
_DSM is expected to cause the platform to put itself into a low-power
operation mode which should include making the EC less verbose (so to
speak).  Next, on resume, function 6 switches the platform back to
the "working-state" operation mode.

In accordance with the above, modify the ACPI suspend-to-idle code
to look for the "Low-Power S0" _DSM interface on platforms with the
ACPI_FADT_LOW_POWER_S0 flag set in the ACPI tables.  If it's there,
use it during suspend-to-idle transitions as prescribed and avoid
changing the GPE configuration in that case.  [That should reflect
what the most recent versions of other OSes do.]

Also modify the ACPI EC driver to make it handle events during
suspend-to-idle in the usual way if the "Low-Power S0" _DSM interface
is going to be used to make the power button events work while
suspended on the Dell machines mentioned above

Link: http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2:
 Add the s2idle_in_progress variable to distinguish between s2idle and other
 transitions, as pm_suspend_via_firmware() is not entirely sufficient for that,
 and use it in acpi_sleep_no_ec_events().

---
 drivers/acpi/ec.c       |    2 
 drivers/acpi/internal.h |    2 
 drivers/acpi/sleep.c    |  113 ++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 112 insertions(+), 5 deletions(-)

Comments

Limonciello, Mario June 23, 2017, 3:37 p.m. UTC | #1
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Friday, June 23, 2017 8:15 AM
> To: Linux ACPI <linux-acpi@vger.kernel.org>
> Cc: Linux PM <linux-pm@vger.kernel.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Tom Lanyon <tom@oneshoeco.com>; Jérôme de
> Bretagne <jerome.debretagne@gmail.com>; Linus Torvalds <torvalds@linux-
> foundation.org>; Zheng, Lv <lv.zheng@intel.com>
> Subject: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on recent
> systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Some recent Dell laptops, including the XPS13 model numbers 9360 and
> 9365, cannot be woken up from suspend-to-idle by pressing the power
> button which is unexpected and makes that feature less usable on
> those systems.  Moreover, on the 9365 ACPI S3 (suspend-to-RAM) is
> not expected to be used at all (the OS these systems ship with never
> exercises the ACPI S3 path in the firmware) and suspend-to-idle is
> the only viable system suspend mechanism there.
> 
> The reason why the power button wakeup from suspend-to-idle doesn't
> work on those systems is because their power button events are
> signaled by the EC (Embedded Controller), whose GPE (General Purpose
> Event) line is disabled during suspend-to-idle transitions in Linux.
> That is done on purpose, because in general the EC tends to be noisy
> for various reasons (battery and thermal updates and similar, for
> example) and all events signaled by it would kick the CPUs out of
> deep idle states while in suspend-to-idle, which effectively might
> defeat its purpose.
> 
> Of course, on the Dell systems in question the EC GPE must be enabled
> during suspend-to-idle transitions for the button press events to
> be signaled while suspended at all, but fortunately there is a way
> out of this puzzle.
> 
> First of all, those systems have the ACPI_FADT_LOW_POWER_S0 flag set
> in their ACPI tables, which means that the OS is expected to prefer
> the "low power S0 idle" system state over ACPI S3 on them.  That
> causes the most recent versions of other OSes to simply ignore ACPI
> S3 on those systems, so it is reasonable to expect that it should not
> be necessary to block GPEs during suspend-to-idle on them.
> 
> Second, in addition to that, the systems in question provide a special
> firmware interface that can be used to indicate to the platform that
> the OS is transitioning into a system-wide low-power state in which
> certain types of activity are not desirable or that it is leaving
> such a state and that (in principle) should allow the platform to
> adjust its operation mode accordingly.
> 
> That interface is a special _DSM object under a System Power
> Management Controller device (PNP0D80).  The expected way to use it
> is to invoke function 0 from it on system initialization, functions
> 3 and 5 during suspend transitions and functions 4 and 6 during
> resume transitions (to reverse the actions carried out by the
> former).  In particular, function 5 from the "Low-Power S0" device
> _DSM is expected to cause the platform to put itself into a low-power
> operation mode which should include making the EC less verbose (so to
> speak).  Next, on resume, function 6 switches the platform back to
> the "working-state" operation mode.
> 
> In accordance with the above, modify the ACPI suspend-to-idle code
> to look for the "Low-Power S0" _DSM interface on platforms with the
> ACPI_FADT_LOW_POWER_S0 flag set in the ACPI tables.  If it's there,
> use it during suspend-to-idle transitions as prescribed and avoid
> changing the GPE configuration in that case.  [That should reflect
> what the most recent versions of other OSes do.]
> 
> Also modify the ACPI EC driver to make it handle events during
> suspend-to-idle in the usual way if the "Low-Power S0" _DSM interface
> is going to be used to make the power button events work while
> suspended on the Dell machines mentioned above
> 
> Link:
> http://www.uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.
> pdf
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> -> v2:
>  Add the s2idle_in_progress variable to distinguish between s2idle and other
>  transitions, as pm_suspend_via_firmware() is not entirely sufficient for that,
>  and use it in acpi_sleep_no_ec_events().
> 
> ---
>  drivers/acpi/ec.c       |    2
>  drivers/acpi/internal.h |    2
>  drivers/acpi/sleep.c    |  113
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 112 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -650,18 +650,108 @@ static const struct platform_suspend_ops
>  	.recover = acpi_pm_finish,
>  };
> 
> +static bool s2idle_in_progress;
>  static bool s2idle_wakeup;
> 
> +/*
> + * On platforms supporting the Low Power S0 Idle interface there is an ACPI
> + * device object with the PNP0D80 compatible device ID (System Power
> Management
> + * Controller) and a specific _DSM method under it.  That method, if present,
> + * can be used to indicate to the platform that the OS is transitioning into a
> + * low-power state in which certain types of activity are not desirable or that
> + * it is leaving such a state, which allows the platform to adjust its operation
> + * mode accordingly.
> + */
> +static const struct acpi_device_id lps0_device_ids[] = {
> +	{"PNP0D80", },
> +	{"", },
> +};
> +
> +#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> +
> +#define ACPI_LPS0_SCREEN_OFF	3
> +#define ACPI_LPS0_SCREEN_ON	4
> +#define ACPI_LPS0_ENTRY		5
> +#define ACPI_LPS0_EXIT		6
The spec you shared also defines device constraints (function 1). It would be very 
useful if these constraints  could be parsed and compared against the actual power 
states of devices on the system at least for debugging purposes.  I'm not sure if you 
already had a plan for that in a future series.

> +
> +#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> ACPI_LPS0_EXIT))
> +
> +static acpi_handle lps0_device_handle;
> +static guid_t lps0_dsm_guid;
> +static char lps0_dsm_func_mask;
> +
> +static void acpi_sleep_run_lps0_dsm(unsigned int func)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (!(lps0_dsm_func_mask & (1 << func)))
> +		return;
> +
> +	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1,
> func, NULL);
> +	ACPI_FREE(out_obj);
> +
> +	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation
> %s\n",
> +			  func, out_obj ? "successful" : "failed");
> +}
> +
> +static int lps0_device_attach(struct acpi_device *adev,
> +			      const struct acpi_device_id *not_used)
> +{
> +	union acpi_object *out_obj;
> +
> +	if (lps0_device_handle)
> +		return 0;
> +
> +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +		return 0;
Although the spec you shared refers to PNP0D80/INT33A1 in the context 
of LPIT, the PNP0D80 device is not "only" used for low power S0.  It's 
available on systems that don't support modern standby too.

I for example see it on a system running Windows that does not support 
modern standby such as the Precision 5510.

All of the ASL executed in PNP0D80 is guarded with a check 
whether or not that low power idle supported bit has been set and whether
or not running on an OSPM that exported a group of features indicating it 
should support it to ensure its run in the right context.

Since Linux responds as the latest group of Windows features but doesn't 
look at ACPI_FADT_LOW_POWER_S0 yet to determine whether to default to 
suspend to idle i'm a little worried about developing more unexercised code 
paths specific to Linux.

For example:
System has ACPI_FADT_LOW_POWER_S0 set, but also supports S3
(such as XPS 9360)
* On Windows PNP0D80 should be used, all ASL code specific to 
modern standby will be run.
* On Linux (with current patch) if a user invokes S3, PNP0D80 doesn't get used
[Win7 should still be using this PNP0D80 codepath, not used by Linux]

* On Linux (if PNP0D80 was supported on all systems but) a user invoked
S3, PNP0D80 functions would be run.
[This should be an undefined behavior since the ASL would run the modern
standby related code but then go into S3]


And yes I realize have argued both for and against exporting PNP0D80 to more 
systems above.  

I think the proper way to align to Windows behavior is recognize PNP0D80 on
all systems and also look at ACPI_FADT_LOW_POWER_S0 at the same time to
align using S2I instead of S3 by default.

Perhaps this is best placed in a follow up patch that can be easily reverted without 
messing up the wonderful work you've done so far in case my idea ends up causing 
other regressions that are not yet envisioned.

> +
> +	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
> +	/* Check if the _DSM is present and as expected. */
> +	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
> +	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) {
> +			lps0_dsm_func_mask = bitmask;
> +			lps0_device_handle = adev->handle;
> +		}
> +
> +		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> +				  bitmask);
> +	} else {
> +		acpi_handle_debug(adev->handle,
> +				  "_DSM function 0 evaluation failed\n");
> +	}
> +	ACPI_FREE(out_obj);
> +	return 0;
> +}
> +
> +static struct acpi_scan_handler lps0_handler = {
> +	.ids = lps0_device_ids,
> +	.attach = lps0_device_attach,
> +};
> +
>  static int acpi_freeze_begin(void)
>  {
>  	acpi_scan_lock_acquire();
> +	s2idle_in_progress = true;
>  	return 0;
>  }
> 
>  static int acpi_freeze_prepare(void)
>  {
> -	acpi_enable_all_wakeup_gpes();
> -	acpi_os_wait_events_complete();
> +	if (lps0_device_handle) {
> +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);

Since there is a separate event specifically for the screen being turned off
I think it would make sense to also export this so that the graphics stack 
could potentially also call this in the future.

In the short term it makes sense to me to call it from the ACPI driver immediately
before resiliency like now though.
Srinivas Pandruvada June 23, 2017, 4:06 p.m. UTC | #2
On Fri, 2017-06-23 at 15:37 +0000, Mario.Limonciello@dell.com wrote:

[...]

> > 
> > +#define ACPI_LPS0_SCREEN_ON	4
> > +#define ACPI_LPS0_ENTRY		5
> > +#define ACPI_LPS0_EXIT		6
> The spec you shared also defines device constraints (function 1). It
> would be very 
> useful if these constraints  could be parsed and compared against the
> actual power 
> states of devices on the system at least for debugging purposes.  I'm
> not sure if you 
> already had a plan for that in a future series.
> 
For debug purpose, I have worked on a patch to dump the constraint
table in debugfs. But in the freeze path whether we meet the
constraints or not will not make any difference, other than for just
debugging.

Thanks,
Srinivas
Limonciello, Mario June 23, 2017, 6:01 p.m. UTC | #3
> -----Original Message-----

> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]

> Sent: Friday, June 23, 2017 11:06 AM

> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; rjw@rjwysocki.net; linux-

> acpi@vger.kernel.org

> Cc: linux-pm@vger.kernel.org; andriy.shevchenko@linux.intel.com;

> dvhart@infradead.org; linux-kernel@vger.kernel.org;

> mika.westerberg@linux.intel.com; tom@oneshoeco.com;

> jerome.debretagne@gmail.com; torvalds@linux-foundation.org;

> lv.zheng@intel.com

> Subject: Re: [PATCH v2] ACPI / sleep: EC-based wakeup from suspend-to-idle on

> recent systems

> 

> On Fri, 2017-06-23 at 15:37 +0000, Mario.Limonciello@dell.com wrote:

> 

> [...]

> 

> > >

> > > +#define ACPI_LPS0_SCREEN_ON	4

> > > +#define ACPI_LPS0_ENTRY		5

> > > +#define ACPI_LPS0_EXIT		6

> > The spec you shared also defines device constraints (function 1). It

> > would be very

> > useful if these constraints  could be parsed and compared against the

> > actual power

> > states of devices on the system at least for debugging purposes.  I'm

> > not sure if you

> > already had a plan for that in a future series.

> >

> For debug purpose, I have worked on a patch to dump the constraint

> table in debugfs. But in the freeze path whether we meet the

> constraints or not will not make any difference, other than for just

> debugging.

> 

> Thanks,

> Srinivas


Right that was what I thought would be most interesting.  You can potentially
output to syslog as a last step going down what isn't in the right state to match
the constraint table.
Rafael J. Wysocki June 24, 2017, 12:43 a.m. UTC | #4
On Friday, June 23, 2017 03:37:36 PM Mario.Limonciello@dell.com wrote:
> > -----Original Message-----

[cut]

> > Index: linux-pm/drivers/acpi/sleep.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/sleep.c
> > +++ linux-pm/drivers/acpi/sleep.c
> > @@ -650,18 +650,108 @@ static const struct platform_suspend_ops
> >  	.recover = acpi_pm_finish,
> >  };
> > 
> > +static bool s2idle_in_progress;
> >  static bool s2idle_wakeup;
> > 
> > +/*
> > + * On platforms supporting the Low Power S0 Idle interface there is an ACPI
> > + * device object with the PNP0D80 compatible device ID (System Power
> > Management
> > + * Controller) and a specific _DSM method under it.  That method, if present,
> > + * can be used to indicate to the platform that the OS is transitioning into a
> > + * low-power state in which certain types of activity are not desirable or that
> > + * it is leaving such a state, which allows the platform to adjust its operation
> > + * mode accordingly.
> > + */
> > +static const struct acpi_device_id lps0_device_ids[] = {
> > +	{"PNP0D80", },
> > +	{"", },
> > +};
> > +
> > +#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
> > +
> > +#define ACPI_LPS0_SCREEN_OFF	3
> > +#define ACPI_LPS0_SCREEN_ON	4
> > +#define ACPI_LPS0_ENTRY		5
> > +#define ACPI_LPS0_EXIT		6
> The spec you shared also defines device constraints (function 1). It would be very 
> useful if these constraints  could be parsed and compared against the actual power 
> states of devices on the system at least for debugging purposes.  I'm not sure if you 
> already had a plan for that in a future series.

As Srinivas said, there is a plan to use it for debug purposes going forward.

> 
> > +
> > +#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 <<
> > ACPI_LPS0_EXIT))
> > +
> > +static acpi_handle lps0_device_handle;
> > +static guid_t lps0_dsm_guid;
> > +static char lps0_dsm_func_mask;
> > +
> > +static void acpi_sleep_run_lps0_dsm(unsigned int func)
> > +{
> > +	union acpi_object *out_obj;
> > +
> > +	if (!(lps0_dsm_func_mask & (1 << func)))
> > +		return;
> > +
> > +	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1,
> > func, NULL);
> > +	ACPI_FREE(out_obj);
> > +
> > +	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation
> > %s\n",
> > +			  func, out_obj ? "successful" : "failed");
> > +}
> > +
> > +static int lps0_device_attach(struct acpi_device *adev,
> > +			      const struct acpi_device_id *not_used)
> > +{
> > +	union acpi_object *out_obj;
> > +
> > +	if (lps0_device_handle)
> > +		return 0;
> > +
> > +	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> > +		return 0;
> Although the spec you shared refers to PNP0D80/INT33A1 in the context 
> of LPIT, the PNP0D80 device is not "only" used for low power S0.  It's 
> available on systems that don't support modern standby too.
> 
> I for example see it on a system running Windows that does not support 
> modern standby such as the Precision 5510.
> 
> All of the ASL executed in PNP0D80 is guarded with a check 
> whether or not that low power idle supported bit has been set and whether
> or not running on an OSPM that exported a group of features indicating it 
> should support it to ensure its run in the right context.
> 
> Since Linux responds as the latest group of Windows features but doesn't 
> look at ACPI_FADT_LOW_POWER_S0 yet to determine whether to default to 
> suspend to idle i'm a little worried about developing more unexercised code 
> paths specific to Linux.
> 
> For example:
> System has ACPI_FADT_LOW_POWER_S0 set, but also supports S3
> (such as XPS 9360)
> * On Windows PNP0D80 should be used, all ASL code specific to 
> modern standby will be run.
> * On Linux (with current patch) if a user invokes S3, PNP0D80 doesn't get used
> [Win7 should still be using this PNP0D80 codepath, not used by Linux]

Well, this is the situation already without this patch. :-)

> 
> * On Linux (if PNP0D80 was supported on all systems but) a user invoked
> S3, PNP0D80 functions would be run.
> [This should be an undefined behavior since the ASL would run the modern
> standby related code but then go into S3]
> 
> 
> And yes I realize have argued both for and against exporting PNP0D80 to more 
> systems above.  
> 
> I think the proper way to align to Windows behavior is recognize PNP0D80 on
> all systems and also look at ACPI_FADT_LOW_POWER_S0 at the same time to
> align using S2I instead of S3 by default.
> 
> Perhaps this is best placed in a follow up patch that can be easily reverted without 
> messing up the wonderful work you've done so far in case my idea ends up causing 
> other regressions that are not yet envisioned.

As you correctly pointed out above, the published documentation refers to the
PNP0D80 _DSM in the "low-power S0" context only and this is the context in
which we are expected to use it for the time being.

In principle, its coverage can be extended in the future as far as I'm concerned,
but it woud be good to be able to clearly demonstrate the benefit from doing
that (like fixing suspend issues or reducing the power draw while suspended on
some systems etc).

That clearly is not directly related to the issue targeted by this patch, however.

> > +
> > +	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
> > +	/* Check if the _DSM is present and as expected. */
> > +	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
> > +	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) {
> > +			lps0_dsm_func_mask = bitmask;
> > +			lps0_device_handle = adev->handle;
> > +		}
> > +
> > +		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> > +				  bitmask);
> > +	} else {
> > +		acpi_handle_debug(adev->handle,
> > +				  "_DSM function 0 evaluation failed\n");
> > +	}
> > +	ACPI_FREE(out_obj);
> > +	return 0;
> > +}
> > +
> > +static struct acpi_scan_handler lps0_handler = {
> > +	.ids = lps0_device_ids,
> > +	.attach = lps0_device_attach,
> > +};
> > +
> >  static int acpi_freeze_begin(void)
> >  {
> >  	acpi_scan_lock_acquire();
> > +	s2idle_in_progress = true;
> >  	return 0;
> >  }
> > 
> >  static int acpi_freeze_prepare(void)
> >  {
> > -	acpi_enable_all_wakeup_gpes();
> > -	acpi_os_wait_events_complete();
> > +	if (lps0_device_handle) {
> > +		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
> 
> Since there is a separate event specifically for the screen being turned off
> I think it would make sense to also export this so that the graphics stack 
> could potentially also call this in the future.

Agreed.

It would require extra care to avoid invoking the same _DSM function twice in
a row, though.

> In the short term it makes sense to me to call it from the ACPI driver immediately
> before resiliency like now though.

OK

Thanks,
Rafael
diff mbox

Patch

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -650,18 +650,108 @@  static const struct platform_suspend_ops
 	.recover = acpi_pm_finish,
 };
 
+static bool s2idle_in_progress;
 static bool s2idle_wakeup;
 
+/*
+ * On platforms supporting the Low Power S0 Idle interface there is an ACPI
+ * device object with the PNP0D80 compatible device ID (System Power Management
+ * Controller) and a specific _DSM method under it.  That method, if present,
+ * can be used to indicate to the platform that the OS is transitioning into a
+ * low-power state in which certain types of activity are not desirable or that
+ * it is leaving such a state, which allows the platform to adjust its operation
+ * mode accordingly.
+ */
+static const struct acpi_device_id lps0_device_ids[] = {
+	{"PNP0D80", },
+	{"", },
+};
+
+#define ACPI_LPS0_DSM_UUID	"c4eb40a0-6cd2-11e2-bcfd-0800200c9a66"
+
+#define ACPI_LPS0_SCREEN_OFF	3
+#define ACPI_LPS0_SCREEN_ON	4
+#define ACPI_LPS0_ENTRY		5
+#define ACPI_LPS0_EXIT		6
+
+#define ACPI_S2IDLE_FUNC_MASK	((1 << ACPI_LPS0_ENTRY) | (1 << ACPI_LPS0_EXIT))
+
+static acpi_handle lps0_device_handle;
+static guid_t lps0_dsm_guid;
+static char lps0_dsm_func_mask;
+
+static void acpi_sleep_run_lps0_dsm(unsigned int func)
+{
+	union acpi_object *out_obj;
+
+	if (!(lps0_dsm_func_mask & (1 << func)))
+		return;
+
+	out_obj = acpi_evaluate_dsm(lps0_device_handle, &lps0_dsm_guid, 1, func, NULL);
+	ACPI_FREE(out_obj);
+
+	acpi_handle_debug(lps0_device_handle, "_DSM function %u evaluation %s\n",
+			  func, out_obj ? "successful" : "failed");
+}
+
+static int lps0_device_attach(struct acpi_device *adev,
+			      const struct acpi_device_id *not_used)
+{
+	union acpi_object *out_obj;
+
+	if (lps0_device_handle)
+		return 0;
+
+	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+		return 0;
+
+	guid_parse(ACPI_LPS0_DSM_UUID, &lps0_dsm_guid);
+	/* Check if the _DSM is present and as expected. */
+	out_obj = acpi_evaluate_dsm(adev->handle, &lps0_dsm_guid, 1, 0, NULL);
+	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) {
+			lps0_dsm_func_mask = bitmask;
+			lps0_device_handle = adev->handle;
+		}
+
+		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
+				  bitmask);
+	} else {
+		acpi_handle_debug(adev->handle,
+				  "_DSM function 0 evaluation failed\n");
+	}
+	ACPI_FREE(out_obj);
+	return 0;
+}
+
+static struct acpi_scan_handler lps0_handler = {
+	.ids = lps0_device_ids,
+	.attach = lps0_device_attach,
+};
+
 static int acpi_freeze_begin(void)
 {
 	acpi_scan_lock_acquire();
+	s2idle_in_progress = true;
 	return 0;
 }
 
 static int acpi_freeze_prepare(void)
 {
-	acpi_enable_all_wakeup_gpes();
-	acpi_os_wait_events_complete();
+	if (lps0_device_handle) {
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_OFF);
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_ENTRY);
+	} else {
+		/*
+		 * The configuration of GPEs is changed here to avoid spurious
+		 * wakeups, but that should not be necessary if this is a
+		 * "low-power S0" platform and the low-power S0 _DSM is present.
+		 */
+		acpi_enable_all_wakeup_gpes();
+		acpi_os_wait_events_complete();
+	}
 	if (acpi_sci_irq_valid())
 		enable_irq_wake(acpi_sci_irq);
 
@@ -700,11 +790,17 @@  static void acpi_freeze_restore(void)
 	if (acpi_sci_irq_valid())
 		disable_irq_wake(acpi_sci_irq);
 
-	acpi_enable_all_runtime_gpes();
+	if (lps0_device_handle) {
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_EXIT);
+		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SCREEN_ON);
+	} else {
+		acpi_enable_all_runtime_gpes();
+	}
 }
 
 static void acpi_freeze_end(void)
 {
+	s2idle_in_progress = false;
 	acpi_scan_lock_release();
 }
 
@@ -727,11 +823,15 @@  static void acpi_sleep_suspend_setup(voi
 
 	suspend_set_ops(old_suspend_ordering ?
 		&acpi_suspend_ops_old : &acpi_suspend_ops);
+
+	acpi_scan_add_handler(&lps0_handler);
 	freeze_set_ops(&acpi_freeze_ops);
 }
 
 #else /* !CONFIG_SUSPEND */
-#define s2idle_wakeup	(false)
+#define s2idle_in_progress	(false)
+#define s2idle_wakeup		(false)
+#define lps0_device_handle	(NULL)
 static inline void acpi_sleep_suspend_setup(void) {}
 #endif /* !CONFIG_SUSPEND */
 
@@ -740,6 +840,11 @@  bool acpi_s2idle_wakeup(void)
 	return s2idle_wakeup;
 }
 
+bool acpi_sleep_no_ec_events(void)
+{
+	return !s2idle_in_progress || !lps0_device_handle;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static u32 saved_bm_rld;
 
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1835,7 +1835,7 @@  static int acpi_ec_suspend(struct device
 	struct acpi_ec *ec =
 		acpi_driver_data(to_acpi_device(dev));
 
-	if (ec_freeze_events)
+	if (acpi_sleep_no_ec_events() && ec_freeze_events)
 		acpi_ec_disable_event(ec);
 	return 0;
 }
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -199,9 +199,11 @@  void acpi_ec_remove_query_handler(struct
   -------------------------------------------------------------------------- */
 #ifdef CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT
 extern bool acpi_s2idle_wakeup(void);
+extern bool acpi_sleep_no_ec_events(void);
 extern int acpi_sleep_init(void);
 #else
 static inline bool acpi_s2idle_wakeup(void) { return false; }
+static inline bool acpi_sleep_no_ec_events(void) { return true; }
 static inline int acpi_sleep_init(void) { return -ENXIO; }
 #endif