Bug 196115

Summary: Power button doesn't suspend and 6s long press needed to resume from suspend-to-idle - Dell Latitude 7275
Product: ACPI Reporter: Jérôme de Bretagne (jerome.debretagne)
Component: Power-Sleep-WakeAssignee: Rafael J. Wysocki (rjw)
Status: CLOSED CODE_FIX    
Severity: normal CC: jerome.debretagne, rui.zhang, superm1, yu.c.chen
Priority: P1    
Hardware: Intel   
OS: Linux   
Kernel Version: 4.13 4.12 4.11 4.10 4.9 3.16 Subsystem:
Regression: No Bisected commit-id:
Attachments: intel-hid: Check power button press event code unconditionally in wakeup mode
intel-hid: Check power button press event code unconditionally for the suspend case
intel-hid: Check power button press event code for the suspend case v2

Description Jérôme de Bretagne 2017-06-18 16:36:28 UTC
Hi,

Dell Latitude 7275 is a 2-in-1 detachable laptop with an Intel Core-M processor (6th gen "Skylake"). There is a similar consumer product branded XPS 12 (9250). This system as the 2 following sleep-wake issues:

1/ Pressing the power button doesn't trigger the suspend.


2/ When suspending manually from the command line in "s2idle" mode instead of the default "deep" value with:
 # echo freeze > /sys/power/state
the system can resume reliably but only with a *very* long press of the power button, for about 6 to 7 seconds.

It is quite surprising at first, especially since the system wakes up instantaneously from sleep on Windows with a usual short press.

The wake-up issue has been discussed on the linux-pm / linux-acpi mailing lists, I'll share the outputs here in follow-up comments.

Thanks,
Jérôme


P.S. The resume case was detected in this other Dell Latitude 7275 issue:
     https://bugzilla.kernel.org/show_bug.cgi?id=195897#c8:
I'm creating this separate bug entry as it is a totally different issue than the bug mentioned above.
Comment 1 Jérôme de Bretagne 2017-06-18 16:50:57 UTC
(In reply to Chen Yu from comment #21 in Bug 195897)
> (In reply to Chen Yu from comment #20in Bug 195897)
> > After suspended to idle(with Rafael's patch modified for the Latitude 7275)
> > on top of BIOS 1.1.31, have to hold the power button for 6 second to
> > wakeup.Well, I think there is still problem that it takes so much time to
> > resume. Another test might be, how about waking up the system from s2idle
> by
> > rtcwake?

Waking up from s2idle with rtcwake is instantaneous.

> AFAIK, BIOS is not involved during s2idle, is it possible that the system
> has already resumed but the graphic did not show up? maybe this can be
> verified by ping the 7275 across s2idle test cycle.

The graphic stack is not the issue here. In this previous comment, BIOS was mentioned since the behavior of that system has changed between version 1.1.20 and the current latest 1.1.31.

Quoting Rafael from http://marc.info/?l=linux-pm&m=149731067302800&w=2:

"However, BIOS 1.1.31 contains additional Notify () invocations in the NEVT () method used for processing EC events (invoked by _Q66) with event values (0xCE and 0xCF) corresponding to the 5-button array power button scancodes."


and then, in http://marc.info/?l=linux-acpi&m=149731336104641&w=2 :

"intel-hid could check the 0xCE event unconditionally in the "wakeup" mode, but that will be a separate patch on top of my series."
Comment 2 Jérôme de Bretagne 2017-07-09 16:50:42 UTC
I'm continuing the discussion about how to handle the suspend case here:

http://marc.info/?l=linux-acpi&m=149961874514664&w=2

before being able to propose a proper patch covering both suspend and wakeup scenarios.
Comment 3 Rafael J. Wysocki 2017-07-27 00:30:18 UTC
Created attachment 257721 [details]
intel-hid: Check power button press event code unconditionally in wakeup mode

Please check if this patch makes the power button wakeup from suspend-to-idle work on your system.
Comment 4 Jérôme de Bretagne 2017-07-27 10:45:40 UTC
I've just tested this patch on linux-pm master branch, being currently 4.13-rc2. It is working fine and makes the power button wakeup from suspend-to-idle on my Latitude 7275. Thanks Rafael.

Feel free to add:
Tested-by: Jérôme de Bretagne <jerome.debretagne@gmail.com>

Let me know if you need any other inputs for the suspend case.
Comment 5 Jérôme de Bretagne 2017-09-08 22:50:23 UTC
Created attachment 258281 [details]
intel-hid: Check power button press event code unconditionally for the suspend case

By taking inspiration from the code from Yu for the Surface Pro 3 found in [1], I've finally been able to create a small patch that makes suspend work with the power button on the Dell Latitude 7275.

I've taken the same approach as for the wakeup case, which is to check the OxCE event unconditionally, but outside the "wakeup" mode this time. KEY_POWER is then reported using input_report_key, as done for the Surface Pro 3.

Rafael, Yu, please let me know if such a patch would be an acceptable implementation to fix this suspend issue, especially the "unconditional" part of it? Thank you.


[1] https://bugzilla.kernel.org/show_bug.cgi?id=84651
Comment 6 Jérôme de Bretagne 2017-09-08 23:06:01 UTC
Adding you Mario to see if you have any comments on this suspend fix proposal,  since you reviewed the previous patch from Rafael for the wakeup scenario.

Also by email you previously proposed to catch and ignore the "unknown event 0xCF" that are still appearing in the logs. That would be a small (and nice) fine-tuning to add indeed.
Comment 7 Chen Yu 2017-09-09 02:52:55 UTC
(In reply to Jérôme de Bretagne from comment #5)
> Created attachment 258281 [details]
> intel-hid: Check power button press event code unconditionally for the
> suspend case 
> I've taken the same approach as for the wakeup case, which is to check the
> OxCE event unconditionally, but outside the "wakeup" mode this time.
> KEY_POWER is then reported using input_report_key, as done for the Surface
> Pro 3.
> 
> Rafael, Yu, please let me know if such a patch would be an acceptable
> implementation to fix this suspend issue, especially the "unconditional"
> part of it? Thank you.
> 
Looks ok for me.
Comment 8 Mario Limonciello 2017-09-11 16:29:06 UTC
@Jérôme,

Were you aware this landed?
https://github.com/torvalds/linux/commit/635173a17b0323c28a39fb06fa36d876035cd2b9#diff-8a4b68d9537ee44bd31caf61049c1c8b

I think it accomplishes your fix too doesn't it?
Comment 9 Jérôme de Bretagne 2017-09-11 17:32:23 UTC
@Mario,

Fully aware indeed. However, the commit you mention from Rafael added support only for the wakeup case.

This follow-up proposal in comment #5 is now adding support for the suspend case, that has never been working with the hardware Power button (until this patch).

It would be great if you could have look at it, as I wonder if there could be any side-effects for other systems that expose the 5-array button. Thanks.


P.S. To make it more explicit, I've always had to trigger suspend from the command line until now on this system.
Comment 10 Mario Limonciello 2017-09-11 18:17:36 UTC
@Jérôme,

Ah OK thanks for clarifying.  I think this approach looks fine to me too.
Comment 11 Jérôme de Bretagne 2017-09-11 19:16:28 UTC
Thanks to you both, Yu and Mario!
Comment 12 Jérôme de Bretagne 2017-09-12 20:36:31 UTC
Created attachment 258337 [details]
intel-hid: Check power button press event code for the suspend case v2

v2:
 - Move from an unconditional to a conditional check, based on
   priv->array being NULL, to only target the affected platforms

 - On those platforms, catch also and ignore the corresponding 
   power release notification (0xCF) to stop it from being
   reported as an "unknown event" message in the logs
Comment 13 Jérôme de Bretagne 2017-11-12 21:40:54 UTC
The first of the 2 discussed issues is now fixed upstream.

Indeed, the patch proposed in comment #3 for the wakeup case is now part of Linux 4.14, commit here for reference:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=635173a17b0323c28a39fb06fa36d876035cd2b9

Waiting for the second patch to land upstream, hopefully in Linux 4.15, before marking this bug as closed overall.
Comment 14 Jérôme de Bretagne 2018-01-31 21:18:56 UTC
The second of the 2 discussed issues is now also fixed upstream, the second patch is part of Linux 4.15, commit here for reference:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/platform/x86/intel-hid.c?h=v4.15&id=821b85366284542e00dd834062144c637e818ee0

this can now be closed.