Re: [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive

[Chris Chiu]

On Mon, May 18, 2020 at 4:59 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Thu, May 14, 2020 at 12:10 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > Flushing the EC work while suspended to idle when the EC GPE status
> > is not set causes some EC wakeup events (notably power button and
> > lid ones) to be missed after a series of spurious wakeups on the Dell
> > XPS13 9360 in my office.
> >
> > If that happens, the machine cannot be woken up from suspend-to-idle
> > by a power button press or lid status change and it needs to be woken
> > up in some other way (eg. by a key press).
> >
> > Flushing the EC work only after successful dispatching the EC GPE,
> > which means that its status has been set, avoids the issue, so change
> > the code in question accordingly.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > Hi Chris,
> >
> > Please check if the key press wakeup still works on your system with this patch
> > applied (on top of https://patchwork.kernel.org/patch/11538065/).
>
> Hi Chris,
>
> Since I haven't heard back from you and the problem at hand is a
> regression on the machine where it happens, I'm going to push this
> patch for merging.
>
> If it causes the key press wakeup issue to reappear on your machine,
> I'm afraid that we'll need to quirk it in the EC driver.
>
> Thanks!

Hi Rafael,
My laptop works w/o problem waking up from a keystroke with this patch
on top of https://patchwork.kernel.org/patch/11538065/).

Chris
>
> > ---
> >  drivers/acpi/ec.c    |    6 +++++-
> >  drivers/acpi/sleep.c |   15 ++++-----------
> >  2 files changed, 9 insertions(+), 12 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/ec.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/ec.c
> > +++ linux-pm/drivers/acpi/ec.c
> > @@ -2020,9 +2020,13 @@ bool acpi_ec_dispatch_gpe(void)
> >          * to allow the caller to process events properly after that.
> >          */
> >         ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
> > -       if (ret == ACPI_INTERRUPT_HANDLED)
> > +       if (ret == ACPI_INTERRUPT_HANDLED) {
> >                 pm_pr_dbg("EC GPE dispatched\n");
> >
> > +               /* Flush the event and query workqueues. */
> > +               acpi_ec_flush_work();
> > +       }
> > +
> >         return false;
> >  }
> >  #endif /* CONFIG_PM_SLEEP */
> > Index: linux-pm/drivers/acpi/sleep.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/sleep.c
> > +++ linux-pm/drivers/acpi/sleep.c
> > @@ -980,13 +980,6 @@ static int acpi_s2idle_prepare_late(void
> >         return 0;
> >  }
> >
> > -static void acpi_s2idle_sync(void)
> > -{
> > -       /* The EC driver uses special workqueues that need to be flushed. */
> > -       acpi_ec_flush_work();
> > -       acpi_os_wait_events_complete(); /* synchronize Notify handling */
> > -}
> > -
> >  static bool acpi_s2idle_wake(void)
> >  {
> >         if (!acpi_sci_irq_valid())
> > @@ -1018,7 +1011,7 @@ static bool acpi_s2idle_wake(void)
> >                         return true;
> >
> >                 /*
> > -                * Cancel the wakeup and process all pending events in case
> > +                * Cancel the SCI wakeup and process all pending events in case
> >                  * there are any wakeup ones in there.
> >                  *
> >                  * Note that if any non-EC GPEs are active at this point, the
> > @@ -1026,8 +1019,7 @@ static bool acpi_s2idle_wake(void)
> >                  * should be missed by canceling the wakeup here.
> >                  */
> >                 pm_system_cancel_wakeup();
> > -
> > -               acpi_s2idle_sync();
> > +               acpi_os_wait_events_complete();
> >
> >                 /*
> >                  * The SCI is in the "suspended" state now and it cannot produce
> > @@ -1060,7 +1052,8 @@ static void acpi_s2idle_restore(void)
> >          * of GPEs.
> >          */
> >         acpi_os_wait_events_complete(); /* synchronize GPE processing */
> > -       acpi_s2idle_sync();
> > +       acpi_ec_flush_work(); /* flush the EC driver's workqueues */
> > +       acpi_os_wait_events_complete(); /* synchronize Notify handling */
> >
> >         s2idle_wakeup = false;
> >
> >
> >
> >