On Thu, Jun 13, 2019 at 1:24 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> 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)) {
>
>
>
>
