2017-06-01 10:43:29

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
> Hi All,
>
> This is a follow-up for a patch series posted some time ago:
>
> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
>
> The first two patches from that series are in 4.12-rc already and the
> rest
> have been rearranged.
>

...

> The first two patches in the current series  update the drivers used
> for button
> events processing on the affected systems so that they signal wakeup
> as
> expected and avoid propagating the wakeup events as button events to
> user
> space.

...

> There is no code dependency between patches [1-2/3] and patch [3/3],
> but all
> of them together are necessary for the feature in question to work on
> both the
> affected systems, so IMO they should be applied together.
>
> The series is available from a git branch at
>  
>   git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> s2idle-dell-test
>  
> and has been included into the testing branch thereof.
>  
> If there are any concerns regarding this series, please let me know.

Is this supposed to go via linux PM tree?

I'm fine with the first two:
Acked-by: Andy Shevchenko <[email protected]>


--
Andy Shevchenko <[email protected]>
Intel Finland Oy


2017-06-01 11:50:52

by Tom Lanyon

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[resend as text/plain]

On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
> Hi All,
>
> This is a follow-up for a patch series posted some time ago:
>
> http://marc.info/?l=linux-kernel&m=149324246701378&w=2

I've applied Rafael's s2idle-dell-test branch to 4.12.0-rc3 and tested
on a Dell 9365 and, whilst it's significantly improved, it's not yet
working correctly.

Previously I could suspend (s2idle and deep), but it took an awkward
~8 second press of the power button to get it to resume, and I could
never get resume to work when triggering suspend/resume via close/open
of the lid.

With this patchset applied, I can suspend (s2idle) and a momentary
press of the power button resumes successfully. I can also use the
lid switch to both suspend and resume successfully.

However, the EC events appear to trigger the machine to wake very
frequently whilst it's supposed to be suspended. This is visible via
the kernel messages at the end of this mail (leaving it in a suspended
state for a few hours resulted in many thousands of these messages),
and the high power draw witnessed.

Let me know if there's anything I can do to help debug further.

Tom

[ 43.669798] PM: Syncing filesystems ... done.
[ 43.675412] PM: Preparing system for sleep (freeze)
[ 43.695469] Freezing user space processes ... (elapsed 0.001 seconds) done.
[ 43.696769] OOM killer disabled.
[ 43.696770] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[ 43.697914] PM: Suspending system (freeze)
[ 43.910325] wlan0: deauthenticating from 9c:1c:12:c7:c2:f1 by local
choice (Reason: 3=DEAUTH_LEAVING)
[ 44.112944] psmouse serio1: Failed to disable mouse on isa0060/serio1
[ 45.721790] PM: suspend of devices complete after 1811.701 msecs
[ 45.739769] PM: late suspend of devices complete after 17.956 msecs
[ 45.742599] ACPI : EC: interrupt blocked
[ 45.744774] xhci_hcd 0000:00:14.0: System wakeup enabled by ACPI
[ 45.776254] PM: noirq suspend of devices complete after 34.702 msecs
[ 45.776256] PM: suspend-to-idle
[ 47.029237] Suspended for 0.327 seconds
[ 47.029335] PM: resume from suspend-to-idle
[ 47.029587] ACPI : EC: interrupt unblocked
[ 47.064470] xhci_hcd 0000:00:14.0: System wakeup disabled by ACPI
[ 47.065268] PM: noirq resume of devices complete after 35.894 msecs
[ 47.066264] ACPI : EC: interrupt blocked
[ 47.079210] xhci_hcd 0000:00:14.0: System wakeup enabled by ACPI
[ 47.112461] PM: noirq suspend of devices complete after 47.019 msecs
[ 47.112538] ACPI : EC: interrupt unblocked
[ 47.147672] xhci_hcd 0000:00:14.0: System wakeup disabled by ACPI
[ 47.148516] PM: noirq resume of devices complete after 36.051 msecs
[ 47.149610] ACPI : EC: interrupt blocked
[ 47.161002] xhci_hcd 0000:00:14.0: System wakeup enabled by ACPI
[ 47.192546] PM: noirq suspend of devices complete after 43.955 msecs
[ 47.192551] PM: suspend-to-idle
[ 48.374756] Suspended for 1.836 seconds
[ 48.374870] PM: resume from suspend-to-idle
[ 48.375284] ACPI : EC: interrupt unblocked

2017-06-01 14:59:52

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

Hi,

On Thu, Jun 1, 2017 at 1:50 PM, Tom Lanyon <[email protected]> wrote:
> [resend as text/plain]
>
> On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
>> Hi All,
>>
>> This is a follow-up for a patch series posted some time ago:
>>
>> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
>
> I've applied Rafael's s2idle-dell-test branch to 4.12.0-rc3 and tested
> on a Dell 9365 and, whilst it's significantly improved, it's not yet
> working correctly.
>
> Previously I could suspend (s2idle and deep), but it took an awkward
> ~8 second press of the power button to get it to resume, and I could
> never get resume to work when triggering suspend/resume via close/open
> of the lid.
>
> With this patchset applied, I can suspend (s2idle) and a momentary
> press of the power button resumes successfully. I can also use the
> lid switch to both suspend and resume successfully.
>
> However, the EC events appear to trigger the machine to wake very
> frequently whilst it's supposed to be suspended. This is visible via
> the kernel messages at the end of this mail (leaving it in a suspended
> state for a few hours resulted in many thousands of these messages),
> and the high power draw witnessed.
>
> Let me know if there's anything I can do to help debug further.

Quoting from my cover letter:

"After this series there still is a concern regarding the possible increase of
power draw that may result from the processing of non-wakeup EC events while
suspended which is why the change only affects Dell XPS13 9360 and 9365
for now."

So that is what happens, unfortunately, and we can't do much about it
at the moment.

The only way to avoid that would be to reconfigure the EC during
suspend to stop generating non-wakeup events, but today we have no
reliable way to do that.

Thanks,
Rafael

2017-06-01 15:00:27

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

On Thu, Jun 1, 2017 at 12:43 PM, Andy Shevchenko
<[email protected]> wrote:
> On Thu, 2017-06-01 at 01:23 +0200, Rafael J. Wysocki wrote:
>> Hi All,
>>
>> This is a follow-up for a patch series posted some time ago:
>>
>> http://marc.info/?l=linux-kernel&m=149324246701378&w=2
>>
>> The first two patches from that series are in 4.12-rc already and the
>> rest
>> have been rearranged.
>>
>
> ...
>
>> The first two patches in the current series update the drivers used
>> for button
>> events processing on the affected systems so that they signal wakeup
>> as
>> expected and avoid propagating the wakeup events as button events to
>> user
>> space.
>
> ...
>
>> There is no code dependency between patches [1-2/3] and patch [3/3],
>> but all
>> of them together are necessary for the feature in question to work on
>> both the
>> affected systems, so IMO they should be applied together.
>>
>> The series is available from a git branch at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
>> s2idle-dell-test
>>
>> and has been included into the testing branch thereof.
>>
>> If there are any concerns regarding this series, please let me know.
>
> Is this supposed to go via linux PM tree?

Yes, it is.

> I'm fine with the first two:
> Acked-by: Andy Shevchenko <[email protected]>

Thanks!

2017-06-02 01:07:13

by Tom Lanyon

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

On 2 June 2017 at 00:59, Rafael J. Wysocki <[email protected]> wrote:
>
> Quoting from my cover letter:
>
> "After this series there still is a concern regarding the possible increase of
> power draw that may result from the processing of non-wakeup EC events while
> suspended which is why the change only affects Dell XPS13 9360 and 9365
> for now."
>
> So that is what happens, unfortunately, and we can't do much about it
> at the moment.

OK, but at the moment this is a regression in functionality on those
platforms. Without this patchset, I can successfully s2idle
suspend/resume on an XPS 9365 (albeit with a little bit of awkward
fiddling of the power button to resume). After the patchset, I can't
realistically go into s2idle at all.

> The only way to avoid that would be to reconfigure the EC during
> suspend to stop generating non-wakeup events, but today we have no
> reliable way to do that.

I thought I had read one one of the threads that this was possible in
the same way that it is for Windows on these laptops. What's missing
to make this possible?

Tom

2017-06-02 23:23:43

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

On Friday, June 02, 2017 11:06:49 AM Tom Lanyon wrote:
> On 2 June 2017 at 00:59, Rafael J. Wysocki <[email protected]> wrote:
> >
> > Quoting from my cover letter:
> >
> > "After this series there still is a concern regarding the possible increase of
> > power draw that may result from the processing of non-wakeup EC events while
> > suspended which is why the change only affects Dell XPS13 9360 and 9365
> > for now."
> >
> > So that is what happens, unfortunately, and we can't do much about it
> > at the moment.
>
> OK, but at the moment this is a regression in functionality on those
> platforms. Without this patchset, I can successfully s2idle
> suspend/resume on an XPS 9365 (albeit with a little bit of awkward
> fiddling of the power button to resume). After the patchset, I can't
> realistically go into s2idle at all.

Well, if you think about s2idle as a state in which there's no activity in the
system at all, this is not how it is defined. In fact, if there is a non-wakeup
edge-triggered interrupt occurring while suspended, for example, it will
cause the IRQ core to run a low-level handler for it even though the action
handler(s) won't be run, which is far from "no activity".

s2idle basically is a state in which all system components are (or at least
should be) in low-power states most of the time and user space has been
frozen and patch [3/3] in this series doesn't change that really, so I really
wouldn't call it a "regression in functionality". [I guess the messages in the
log are somewhat confusing, but see below.]

Of course, it does increase the amount of activity in the system while
suspended which in turn causes more energy to be used and battery life to
shorten, so it may be regarded as a power regression. Still, it really is a
tradeoff between functionality (power button wakeups working as expected)
and energy-efficiency and I know about a few people who actually prefer the
functionality to be there.

On top of that, there are a few things that can be optimized slightly. For
instance, we generally run too much code when those EC wakeups happen,
we print too much to the log (even without debug enabled) etc, which also
should reduce the amout of energy that goes away and I have some patches
going in that direction. Moreover, the messages printed to the logs are
not quite as accurate as they should be which needs to be fixed too.

What really matters is how much more energy the system uses after that patch
(or how much less time it will stay on battery) while suspended. Can you please
try to estimate that for your system?

In any case, you have a point that there may be users wanting the systems
to use less energy while suspended to idle at the cost of semi-functional power
button wakeups, so I guess I'll add an acpi_sleep= kernel command line switch
to force-disable EC wakeups even on systems where they would be enabled by
default.

> > The only way to avoid that would be to reconfigure the EC during
> > suspend to stop generating non-wakeup events, but today we have no
> > reliable way to do that.
>
> I thought I had read one one of the threads that this was possible in
> the same way that it is for Windows on these laptops. What's missing
> to make this possible?

Let's say there is work in progress to make it possible to use that interface
in Linux, but it may not actually do everything we want it to do, so it may not
help much here on this particular laptop model.

Thanks,
Rafael