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

[Rafael J. Wysocki]

On Mon, May 18, 2020 at 1:11 PM Chris Chiu <chiu@xxxxxxxxxxxx> wrote:
>
> 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/).

Great, thanks for the confirmation!

> > > ---
> > >  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;
> > >
> > >
> > >
> > >