Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive
@ 2020-05-14 10:10 Rafael J. Wysocki
  2020-05-18  8:59 ` Rafael J. Wysocki
  0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2020-05-14 10:10 UTC (permalink / raw)
  To: Linux ACPI, Chris Chiu; +Cc: Linux PM, LKML

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---

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/).

Thanks!

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




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive
  2020-05-14 10:10 [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive Rafael J. Wysocki
@ 2020-05-18  8:59 ` Rafael J. Wysocki
  2020-05-18  9:32   ` Chris Chiu
  2020-05-18 11:11   ` Chris Chiu
  0 siblings, 2 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2020-05-18  8:59 UTC (permalink / raw)
  To: Chris Chiu; +Cc: Linux ACPI, Linux PM, LKML, Rafael J. Wysocki

On Thu, May 14, 2020 at 12:10 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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@intel.com>
> ---
>
> 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!

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive
  2020-05-18  8:59 ` Rafael J. Wysocki
@ 2020-05-18  9:32   ` Chris Chiu
  2020-05-18 11:11   ` Chris Chiu
  1 sibling, 0 replies; 5+ messages in thread
From: Chris Chiu @ 2020-05-18  9:32 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Linux ACPI, Linux PM, LKML, Rafael J. Wysocki

On Mon, May 18, 2020 at 4:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, May 14, 2020 at 12:10 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >
> > 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!
>

Sorry that I missed this email. I'll apply this patch and test again.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive
  2020-05-18  8:59 ` Rafael J. Wysocki
  2020-05-18  9:32   ` Chris Chiu
@ 2020-05-18 11:11   ` Chris Chiu
  2020-05-18 11:14     ` Rafael J. Wysocki
  1 sibling, 1 reply; 5+ messages in thread
From: Chris Chiu @ 2020-05-18 11:11 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Linux ACPI, Linux PM, LKML, Rafael J. Wysocki

On Mon, May 18, 2020 at 4:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, May 14, 2020 at 12:10 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >
> > 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;
> >
> >
> >
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive
  2020-05-18 11:11   ` Chris Chiu
@ 2020-05-18 11:14     ` Rafael J. Wysocki
  0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2020-05-18 11:14 UTC (permalink / raw)
  To: Chris Chiu
  Cc: Rafael J. Wysocki, Linux ACPI, Linux PM, LKML, Rafael J. Wysocki

On Mon, May 18, 2020 at 1:11 PM Chris Chiu <chiu@endlessm.com> wrote:
>
> On Mon, May 18, 2020 at 4:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, May 14, 2020 at 12:10 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > 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@intel.com>
> > > ---
> > >
> > > 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;
> > >
> > >
> > >
> > >

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14 10:10 [PATCH[RFT]] ACPI: EC: s2idle: Avoid flushing EC work when EC GPE is inactive Rafael J. Wysocki
2020-05-18  8:59 ` Rafael J. Wysocki
2020-05-18  9:32   ` Chris Chiu
2020-05-18 11:11   ` Chris Chiu
2020-05-18 11:14     ` Rafael J. Wysocki

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git