Re: Help me fix a regression caused by 56b9918490 (PM: sleep: Simplify suspend-to-idle control flow)

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Monday, November 25, 2019 2:27:00 PM CET Rafael J. Wysocki wrote:
> On Mon, Nov 25, 2019 at 4:40 AM Kenneth R. Crudup <kenny@panix.com> wrote:
> >
> >
> > Thanks for getting back to me.
> >
> > On Sun, 24 Nov 2019, Rafael J. Wysocki wrote:
> >
> > > > If "sleep_no_lps0" == 1, the machine never goes fully to sleep; the power
> > > > light stays on and the backlight goes off, but if I have external monitors
> > > > connected they're still showing dmesg activity. This is independent of the
> > > > state of "ec_no_wakeup".
> >
> > > Hmm.  The external monitors part is something you have never mentioned.
> >
> > I didn't realize that myself until I'd tried "sleep_no_lps0" testing for this
> > thread and happened to have my Thunderbolt dock connected.
> 
> OK, but with sleep_no_lps0, does it also hang during resume or not?
> 
> > > > If "ec_no_wakeup" == 1, the system *at times* will go to sleep and never return
> >
> > > This is unclear.  What exactly do you mean by "go to sleep"?
> >
> > It appears to do a suspend cycle; the screen goes off, the power light goes out,
> > and the power consumption (as measured at the charge port) (usually) goes to the
> > smallest draw this laptop is capable of in s2idle.
> >
> > > Which part of the behavior does the "at times" phrase apply to?  The
> > > "going to sleep" or coming back?  Or both?
> >
> > The coming back. Many times I'll hit a key on the keyboard (when "ec_no_wakeup"
> > is set) or open the lid or hit the the power button (if it's not set) and nothing
> > happens. IIRC the current draw doesn't increase either, but don't quote me on
> > that (it's easy enough to reproduce, so I'll try it out and report back).
> >
> > > > (i.e. no power light comes on, it's totally unresponsive until I do a hard
> > > > reset with a power-button long-press) whether I'm plugged in or not.
> > > > This is new behavior.
> >
> > > So how did it behave in 5.3.y?
> > ...
> > > > Help! What can I do to return to the behavior of right before the s0 rework?
> > > I guess you mean the 5.3.y behavior.  And what was it?
> > ...
> >
> > Seemed to always work; I don't recall any issues with s2idle in earlier
> > kernels. Sometimes my idle draw would be much higher than it should be, but
> > I have zero clue as to why that is (which is why I'm chasing down bleeding-
> > edge PM commits).
> >
> > > > If "ec_no_wakeup" == 0, the system goes fully to sleep and either of the
> > > > power button, lid opening or hitting a key resumes the laptop, but if I'm
> > > > plugged in and actually charging when I suspend (and I suspect if I plug
> > > > it in during suspend) it never returns, as in the case above.
> >
> > > OK, so ec_no_wakeup doesn't really change the behavior substantially,
> > > it only makes certain things more or less likely to happen.
> > > Does it still hang if you use the keyboard to wake up the system?
> >
> > When "ec_no_wakeup" is set, ONLY the keyboard wakes up the system, and the dead
> > system is unrelated to the method I'm using to wake things up.
> 
> In that case whatever happens in acpi_s2idle_wake() can be ruled out,
> because on your system that function effectively is a NOP with
> ec_no_wakeup.  I don't expect the test below to add anything new to
> what we know already.
> 
> > > > Where in the code could I start looking to try to find out where the machine
> > > > goes dead?
> > >
> > > Well, because you identified 56b991849 as the first bad commit, the
> > > following three lines of code in drivers/acpi/sleep.c are likely to be
> > > the source of the problem:
> > >
> > >         acpi_os_wait_events_complete(); /* synchronize EC GPE processing */
> > >         acpi_ec_flush_work();
> > >         acpi_os_wait_events_complete(); /* synchronize Notify handling */
> > >
> > > Can you please try to comment them out and retest?
> >
> > I'll do that and get back to you.
> >
> > > Note that you most likely won't be able to wake up the system via the
> > > lid/power button without them
> >
> > Yeah, I'm used to that.
> 
> So, as stated above, this test is not likely to be conclusive.
> 
> Now, given that the changes in acpi_s2idle_wake() don't matter, you
> seem to be missing the acpi_s2idle_sync() after
> dpm_noirq_resume_devices(), because dropping it was the only other
> substantial change made by commit 56b991849 AFAICS.
> 
> I'll send you a patch to try for that.

Appended (untested).

---
 drivers/acpi/sleep.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -977,6 +977,13 @@ static int acpi_s2idle_prepare_late(void
 	return 0;
 }
 
+static void acpi_s2idle_sync(void)
+{
+	acpi_os_wait_events_complete(); /* synchronize GPE processing */
+	acpi_ec_flush_work();
+	acpi_os_wait_events_complete(); /* synchronize Notify handling */
+}
+
 static void acpi_s2idle_wake(void)
 {
 	/*
@@ -1005,9 +1012,7 @@ static void acpi_s2idle_wake(void)
 		 * The EC driver uses the system workqueue and an additional
 		 * special one, so those need to be flushed too.
 		 */
-		acpi_os_wait_events_complete(); /* synchronize EC GPE processing */
-		acpi_ec_flush_work();
-		acpi_os_wait_events_complete(); /* synchronize Notify handling */
+		acpi_s2idle_sync();
 
 		rearm_wake_irq(acpi_sci_irq);
 	}
@@ -1024,6 +1029,8 @@ static void acpi_s2idle_restore_early(vo
 
 static void acpi_s2idle_restore(void)
 {
+	acpi_s2idle_sync();
+
 	s2idle_wakeup = false;
 
 	acpi_enable_all_runtime_gpes();