* [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
@ 2019-05-16 19:36 Furquan Shaikh
2019-06-13 20:24 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Furquan Shaikh @ 2019-05-16 19:36 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown; +Cc: linux-acpi, linux-kernel, rajatja, furquan
This change clears GPE status for wake-up devices before enabling that
GPE. This is required to ensure that stale GPE status does
not result in pre-mature wake on enabling GPE for wake-up devices.
Without this change, here is the sequence of events that is causing
suspend aborts on recent chrome books:
1. System decides to enter sleep.
2. All devices in the system are put into low power mode.
3. This results in acpi_dev_suspend being called for each ACPI
device.
4. If the device is wake capable, then acpi_dev_suspend calls
acpi_device_wakeup_enable to enable GPE for the device.
5. If GPE status is already set, enabling GPE for the wakeup device
results in generating a SCI which is handled by acpi_ev_detect_gpe
ultimately calling wakeup_source_activate that increments wakeup
events, and thus aborting the suspend attempt.
Signed-off-by: Furquan Shaikh <furquan@google.com>
---
drivers/acpi/device_pm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index b859d75eaf9f6..e05ee3ff45683 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -721,6 +721,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev,
if (error)
goto out;
+ acpi_clear_gpe(wakeup->gpe_device, wakeup->gpe_number);
+
status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
if (ACPI_FAILURE(status)) {
acpi_disable_wakeup_device_power(adev);
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
2019-05-16 19:36 [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling Furquan Shaikh
@ 2019-06-13 20:24 ` Rafael J. Wysocki
2019-06-14 0:58 ` Furquan Shaikh
2019-06-14 10:45 ` Mika Westerberg
0 siblings, 2 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-06-13 20:24 UTC (permalink / raw)
To: Furquan Shaikh, Mika Westerberg
Cc: Len Brown, linux-acpi, linux-kernel, rajatja
On Thursday, May 16, 2019 9:36:16 PM CEST Furquan Shaikh wrote:
> This change clears GPE status for wake-up devices before enabling that
> GPE. This is required to ensure that stale GPE status does
> not result in pre-mature wake on enabling GPE for wake-up devices.
>
> Without this change, here is the sequence of events that is causing
> suspend aborts on recent chrome books:
>
> 1. System decides to enter sleep.
> 2. All devices in the system are put into low power mode.
> 3. This results in acpi_dev_suspend being called for each ACPI
> device.
> 4. If the device is wake capable, then acpi_dev_suspend calls
> acpi_device_wakeup_enable to enable GPE for the device.
> 5. If GPE status is already set, enabling GPE for the wakeup device
> results in generating a SCI which is handled by acpi_ev_detect_gpe
> ultimately calling wakeup_source_activate that increments wakeup
> events, and thus aborting the suspend attempt.
>
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> ---
> drivers/acpi/device_pm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index b859d75eaf9f6..e05ee3ff45683 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -721,6 +721,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev,
> if (error)
> goto out;
>
> + acpi_clear_gpe(wakeup->gpe_device, wakeup->gpe_number);
> +
> status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
> if (ACPI_FAILURE(status)) {
> acpi_disable_wakeup_device_power(adev);
>
This patch may cause events to be missed if the GPE. I guess what you reall mean is
something like the patch below.
This should allow the kernel to see the events generated before the GPEs are
implicitly enabled, but it should clear them for the explicit users of acpi_enable_gpe().
Mika, what do you think?
---
drivers/acpi/acpica/acevents.h | 3 ++-
drivers/acpi/acpica/evgpe.c | 8 +++++++-
drivers/acpi/acpica/evgpeblk.c | 2 +-
drivers/acpi/acpica/evxface.c | 2 +-
drivers/acpi/acpica/evxfgpe.c | 2 +-
5 files changed, 12 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/acpica/acevents.h
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/acevents.h
+++ linux-pm/drivers/acpi/acpica/acevents.h
@@ -69,7 +69,8 @@ acpi_status
acpi_ev_mask_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 is_masked);
acpi_status
-acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
+acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info,
+ u8 clear_on_enable);
acpi_status
acpi_ev_remove_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
Index: linux-pm/drivers/acpi/acpica/evgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evgpe.c
+++ linux-pm/drivers/acpi/acpica/evgpe.c
@@ -146,6 +146,7 @@ acpi_ev_mask_gpe(struct acpi_gpe_event_i
* FUNCTION: acpi_ev_add_gpe_reference
*
* PARAMETERS: gpe_event_info - Add a reference to this GPE
+ * clear_on_enable - Clear GPE status before enabling it
*
* RETURN: Status
*
@@ -155,7 +156,8 @@ acpi_ev_mask_gpe(struct acpi_gpe_event_i
******************************************************************************/
acpi_status
-acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info)
+acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info,
+ u8 clear_on_enable)
{
acpi_status status = AE_OK;
@@ -170,6 +172,10 @@ acpi_ev_add_gpe_reference(struct acpi_gp
/* Enable on first reference */
+ if (clear_on_enable) {
+ (void)acpi_hw_clear_gpe(gpe_event_info);
+ }
+
status = acpi_ev_update_gpe_enable_mask(gpe_event_info);
if (ACPI_SUCCESS(status)) {
status = acpi_ev_enable_gpe(gpe_event_info);
Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
+++ linux-pm/drivers/acpi/acpica/evgpeblk.c
@@ -453,7 +453,7 @@ acpi_ev_initialize_gpe_block(struct acpi
continue;
}
- status = acpi_ev_add_gpe_reference(gpe_event_info);
+ status = acpi_ev_add_gpe_reference(gpe_event_info, FALSE);
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status,
"Could not enable GPE 0x%02X",
Index: linux-pm/drivers/acpi/acpica/evxface.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evxface.c
+++ linux-pm/drivers/acpi/acpica/evxface.c
@@ -971,7 +971,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
ACPI_GPE_DISPATCH_METHOD) ||
(ACPI_GPE_DISPATCH_TYPE(handler->original_flags) ==
ACPI_GPE_DISPATCH_NOTIFY)) && handler->originally_enabled) {
- (void)acpi_ev_add_gpe_reference(gpe_event_info);
+ (void)acpi_ev_add_gpe_reference(gpe_event_info, FALSE);
if (ACPI_GPE_IS_POLLING_NEEDED(gpe_event_info)) {
/* Poll edge triggered GPEs to handle existing events */
Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
+++ linux-pm/drivers/acpi/acpica/evxfgpe.c
@@ -108,7 +108,7 @@ acpi_status acpi_enable_gpe(acpi_handle
if (gpe_event_info) {
if (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) !=
ACPI_GPE_DISPATCH_NONE) {
- status = acpi_ev_add_gpe_reference(gpe_event_info);
+ status = acpi_ev_add_gpe_reference(gpe_event_info, TRUE);
if (ACPI_SUCCESS(status) &&
ACPI_GPE_IS_POLLING_NEEDED(gpe_event_info)) {
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
2019-06-13 20:24 ` Rafael J. Wysocki
@ 2019-06-14 0:58 ` Furquan Shaikh
2019-06-14 10:45 ` Mika Westerberg
1 sibling, 0 replies; 5+ messages in thread
From: Furquan Shaikh @ 2019-06-14 0:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Mika Westerberg, Len Brown, ACPI Devel Maling List,
Linux Kernel Mailing List, Rajat Jain
On Thu, Jun 13, 2019 at 1:24 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Thursday, May 16, 2019 9:36:16 PM CEST Furquan Shaikh wrote:
> > This change clears GPE status for wake-up devices before enabling that
> > GPE. This is required to ensure that stale GPE status does
> > not result in pre-mature wake on enabling GPE for wake-up devices.
> >
> > Without this change, here is the sequence of events that is causing
> > suspend aborts on recent chrome books:
> >
> > 1. System decides to enter sleep.
> > 2. All devices in the system are put into low power mode.
> > 3. This results in acpi_dev_suspend being called for each ACPI
> > device.
> > 4. If the device is wake capable, then acpi_dev_suspend calls
> > acpi_device_wakeup_enable to enable GPE for the device.
> > 5. If GPE status is already set, enabling GPE for the wakeup device
> > results in generating a SCI which is handled by acpi_ev_detect_gpe
> > ultimately calling wakeup_source_activate that increments wakeup
> > events, and thus aborting the suspend attempt.
> >
> > Signed-off-by: Furquan Shaikh <furquan@google.com>
> > ---
> > drivers/acpi/device_pm.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index b859d75eaf9f6..e05ee3ff45683 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -721,6 +721,8 @@ static int __acpi_device_wakeup_enable(struct acpi_device *adev,
> > if (error)
> > goto out;
> >
> > + acpi_clear_gpe(wakeup->gpe_device, wakeup->gpe_number);
> > +
> > status = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number);
> > if (ACPI_FAILURE(status)) {
> > acpi_disable_wakeup_device_power(adev);
> >
>
> This patch may cause events to be missed if the GPE. I guess what you reall mean is
> something like the patch below.
Thanks for the patch Rafael! This indeed fixes the issue on my platform.
FWIW, Tested-By: Furquan Shaikh <furquan@google.com>
>
> This should allow the kernel to see the events generated before the GPEs are
> implicitly enabled, but it should clear them for the explicit users of acpi_enable_gpe().
>
> Mika, what do you think?
>
> ---
> drivers/acpi/acpica/acevents.h | 3 ++-
> drivers/acpi/acpica/evgpe.c | 8 +++++++-
> drivers/acpi/acpica/evgpeblk.c | 2 +-
> drivers/acpi/acpica/evxface.c | 2 +-
> drivers/acpi/acpica/evxfgpe.c | 2 +-
> 5 files changed, 12 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpica/acevents.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/acevents.h
> +++ linux-pm/drivers/acpi/acpica/acevents.h
> @@ -69,7 +69,8 @@ acpi_status
> acpi_ev_mask_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 is_masked);
>
> acpi_status
> -acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
> +acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info,
> + u8 clear_on_enable);
>
> acpi_status
> acpi_ev_remove_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
> Index: linux-pm/drivers/acpi/acpica/evgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpe.c
> +++ linux-pm/drivers/acpi/acpica/evgpe.c
> @@ -146,6 +146,7 @@ acpi_ev_mask_gpe(struct acpi_gpe_event_i
> * FUNCTION: acpi_ev_add_gpe_reference
> *
> * PARAMETERS: gpe_event_info - Add a reference to this GPE
> + * clear_on_enable - Clear GPE status before enabling it
> *
> * RETURN: Status
> *
> @@ -155,7 +156,8 @@ acpi_ev_mask_gpe(struct acpi_gpe_event_i
> ******************************************************************************/
>
> acpi_status
> -acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info)
> +acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info,
> + u8 clear_on_enable)
> {
> acpi_status status = AE_OK;
>
> @@ -170,6 +172,10 @@ acpi_ev_add_gpe_reference(struct acpi_gp
>
> /* Enable on first reference */
>
> + if (clear_on_enable) {
> + (void)acpi_hw_clear_gpe(gpe_event_info);
> + }
> +
> status = acpi_ev_update_gpe_enable_mask(gpe_event_info);
> if (ACPI_SUCCESS(status)) {
> status = acpi_ev_enable_gpe(gpe_event_info);
> Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
> +++ linux-pm/drivers/acpi/acpica/evgpeblk.c
> @@ -453,7 +453,7 @@ acpi_ev_initialize_gpe_block(struct acpi
> continue;
> }
>
> - status = acpi_ev_add_gpe_reference(gpe_event_info);
> + status = acpi_ev_add_gpe_reference(gpe_event_info, FALSE);
> if (ACPI_FAILURE(status)) {
> ACPI_EXCEPTION((AE_INFO, status,
> "Could not enable GPE 0x%02X",
> Index: linux-pm/drivers/acpi/acpica/evxface.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evxface.c
> +++ linux-pm/drivers/acpi/acpica/evxface.c
> @@ -971,7 +971,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
> ACPI_GPE_DISPATCH_METHOD) ||
> (ACPI_GPE_DISPATCH_TYPE(handler->original_flags) ==
> ACPI_GPE_DISPATCH_NOTIFY)) && handler->originally_enabled) {
> - (void)acpi_ev_add_gpe_reference(gpe_event_info);
> + (void)acpi_ev_add_gpe_reference(gpe_event_info, FALSE);
> if (ACPI_GPE_IS_POLLING_NEEDED(gpe_event_info)) {
>
> /* Poll edge triggered GPEs to handle existing events */
> Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
> +++ linux-pm/drivers/acpi/acpica/evxfgpe.c
> @@ -108,7 +108,7 @@ acpi_status acpi_enable_gpe(acpi_handle
> if (gpe_event_info) {
> if (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) !=
> ACPI_GPE_DISPATCH_NONE) {
> - status = acpi_ev_add_gpe_reference(gpe_event_info);
> + status = acpi_ev_add_gpe_reference(gpe_event_info, TRUE);
> if (ACPI_SUCCESS(status) &&
> ACPI_GPE_IS_POLLING_NEEDED(gpe_event_info)) {
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
2019-06-13 20:24 ` Rafael J. Wysocki
2019-06-14 0:58 ` Furquan Shaikh
@ 2019-06-14 10:45 ` Mika Westerberg
2019-06-14 13:27 ` Rafael J. Wysocki
1 sibling, 1 reply; 5+ messages in thread
From: Mika Westerberg @ 2019-06-14 10:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Furquan Shaikh, Len Brown, linux-acpi, linux-kernel, rajatja
On Thu, Jun 13, 2019 at 10:24:41PM +0200, Rafael J. Wysocki wrote:
> This patch may cause events to be missed if the GPE. I guess what you reall mean is
> something like the patch below.
>
> This should allow the kernel to see the events generated before the GPEs are
> implicitly enabled, but it should clear them for the explicit users of acpi_enable_gpe().
>
> Mika, what do you think?
Looks good to me. I also tested this with two TBT systems (Skull Canyon
NUC and Dell XPS 9370) using ACPI hotplug and it did not cause any
problems if I boot the system with device connected.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
2019-06-14 10:45 ` Mika Westerberg
@ 2019-06-14 13:27 ` Rafael J. Wysocki
0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-06-14 13:27 UTC (permalink / raw)
To: Mika Westerberg
Cc: Rafael J. Wysocki, Furquan Shaikh, Len Brown,
ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain
On Fri, Jun 14, 2019 at 12:45 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 10:24:41PM +0200, Rafael J. Wysocki wrote:
> > This patch may cause events to be missed if the GPE. I guess what you reall mean is
> > something like the patch below.
> >
> > This should allow the kernel to see the events generated before the GPEs are
> > implicitly enabled, but it should clear them for the explicit users of acpi_enable_gpe().
> >
> > Mika, what do you think?
>
> Looks good to me. I also tested this with two TBT systems (Skull Canyon
> NUC and Dell XPS 9370) using ACPI hotplug and it did not cause any
> problems if I boot the system with device connected.
Awesome, thanks!
I'll add a changelog to it and post a full version over the weekend or
early next week.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-14 13:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16 19:36 [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling Furquan Shaikh
2019-06-13 20:24 ` Rafael J. Wysocki
2019-06-14 0:58 ` Furquan Shaikh
2019-06-14 10:45 ` Mika Westerberg
2019-06-14 13:27 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).