Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
On Fri, Jun 14, 2019 at 12:45 PM Mika Westerberg 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.
Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
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.
Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
On Thu, Jun 13, 2019 at 1:24 PM Rafael J. Wysocki 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
> > ---
> > 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
>
> 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);
>
Re: [PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
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
> ---
> 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
@@
[PATCH] ACPI: PM: Clear wake-up device GPEs before enabling
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
---
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
