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

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

On Tue, Nov 26, 2019 at 12:32 AM Kenneth R. Crudup <kenny@panix.com> wrote:
>
>
> On Mon, 25 Nov 2019, Rafael J. Wysocki wrote:
>
> > It looks like the platform has problems with switching power between
> > AC and battery while suspended.  Also this appears to be related to the EC.
>
> That being said, I always keep a stock kernel on this machine (latest is Kubuntu
> vmlinuz-5.0.0-35-generic) and the failure mode isn't reproducible on that. I'm
> still wondering what's substantially different between the versions.
>
> BTW, I've got breaking it down to a science now:
>
> - Let the battery drain pretty well down (< ~80%)
> - Boot the kernel I wish to test, while still on battery
> - Intiate a suspend/resume, which will come back OK
> - Initiate another suspend
> - Plug in the charger. If I have my power meter in, I see it do the PD
>   negotiation (it'll start off at 20V/150mA, then it'll PD ramp up to
>   a full 2.5-2.75A @20v to charge the battery)
> - Try to resume. It'll be totally dead and I have to long-power-button
>   to get it back
>
> - If I have "ec_no_wakeup=1" set, then the first time it seems to resume
>   OK from the procedure above, then I'll wait about two minutes then
>   flip the laptop around a bunch (apparently getting the accelerometer
>   involved, which I guess happens during a car ride) which can also
>   trigger the hard hangup when I plug it in before suspending
>
> > Also I'm wondering if the problem is reproducible with ec_no_wakeup
> > and the last patch I posted applied.
>
> With the stock -rc kernel, I used to be able to work around this issue with
> "ec_no_wakeup=1", but sometimes the battery would drain while "suspended"
> far higher than idle at random.
>
> With the second patchset (the introduction of acpi_s2idle_sync()) the
> *second* resume while the battery is charging locks it up hard.
>
> > But with that patch applied, please try to comment out the
> > acpi_s2idle_sync() in acpi_s2idle_wake() and retest.
>
> So get this- I took it out of acpi_s2idle_wake() and left it in acpi_s2idle_restore() ;
> when I do this, I get the issue where it seems to no longer care about the
> charging state, BUT it is back to responding to charger events (i.e., it won't
> go to sleep as long as it's plugged in, and "ec_no_wakeup=1" has no effect on this.
>
> It also turns out that it doesn't apparently matter if I remove the call of acpi_s2idle_sync()
> from either of acpi_s2idle_wake() or acpi_s2idle_restore() but I get the behavior above.
> I took a shot in the dark and commented out the call to acpi_ec_flush_work() in acpi_s2idle_sync()
> and left the calls to acpi_s2idle_sync() in both _wake and _restore and also get this
> behavior, which to me implies that whatever is in the acpi_ec_flush_work() call (and I
> looked at it, but didn't go any further) is where it's hanging up.

It doesn't have to be like that.

What acpi_ec_flush_work() does it to process the EC events received so
far and they may trigger notifications that are processed
subsequently.  The next acpi_os_wait_events_complete() is there to
wait for that processing to finish.

It looks like it hangs somewhere in the platform firmware, but it is
hard to say where exactly.

> I can put WARN_ON(cond)
> calls in places that'll write to EFI pstore, if you think that would be useful.
>
> As this workaround does at least (so far, anyway) keep the machine from going off into
> a black hole,

So to be precise you mean commenting out acpi_ec_flush_work() in
acpi_s2idle_sync() with the

https://lore.kernel.org/linux-pm/alpine.DEB.2.21.1911220920500.24730@hp-x360n/T/#m199e175157c9cddf48fdee16681f6c2f0549a364

patch applied?

And does the power button/lid wakeup work without that?

> I'm willing to live with it until/as we find a workaround that gets it
> right; even though it'll be somewhat annoying to not go to sleep when charged AND
> automatically wake up on power connection/disconnection events (can those be filtered
> out of /proc/acpi/wakeup_sources ? Nothing looked obvious) at least I have a laptop
> when I need it.

There should be a way to filter them out.

From the above it looks like this happens with "ec_no_wakeup=1" too,
is that correct?

> Can you think of any other strategy to try (and I'm all ears at this point; this
> kind of breakage- while almost certainly that something in this HP BIOS/ACPI
> is doing wrong, should be properly worked around for the rest of us poor suckers
> who got these devices so I'm willing to help either fix it,

It's rather that the platform firmware has expectations that we don't
know about.

Yes, it should be possible to diagnose it further with some extra work.