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

["Rafael J. Wysocki" <rafael@kernel.org>]

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.