On Mon, Jun 12, 2017 at 10:00 PM,  [off-list ref] wrote:

quoted

-----Original Message-----
From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J.
Wysocki
Sent: Monday, June 12, 2017 2:12 PM
To: Jérôme de Bretagne <redacted>
Cc: Limonciello, Mario <redacted>; ACPI Devel Maling List
[off-list ref]; Linux PM [off-list ref]; Rafael J.
Wysocki [off-list ref]; Andy Shevchenko
[off-list ref]; Darren Hart [off-list ref];
Srinivas Pandruvada [off-list ref]; Mika Westerberg
[off-list ref]
Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on
recent Dell laptops

On Mon, Jun 12, 2017 at 12:01 AM, Jérôme de Bretagne
[off-list ref] wrote:

quoted

Hi Mario, Hi Rafael,

quoted

quoted

Some assumptions now: either the SCI is ignored erroneously or it's
not interpreted correctly by the expected driver? I guess that's the 2
possibilities I'll try to investigate.

Well that's too bad.  Yes, you're correct that the EC has changed between
those versions.  Are you testing on or off AC adapter?

I haven't detected a pattern specific to the state of the AC adapter.

Good news on another front. By adding some more logs in the
s2idle-dell-test branch, I've been able to check and confirm that
notify_handler() from intel-hid.c is actually called upon short power key
press during s2idle on my Latitude 7275.

However, the system with BIOS 1.1.31 doesn't match (anymore?) the
current criteria to call pm_wakeup_hard_event introduced in commit:

platform: x86: intel-hid: Wake up the system from suspend-to-idle
7871dc61497a71be93c4f80d43ac109152510e7e

This ugly 3-line modification I've added on the actual patch here:

+ if (priv->wakeup_mode) {
+
+ /* Wake up Dell Latitude 7275 BIOS 1.1.31. */
+ if (event == 0xce)
+ pm_wakeup_hard_event(&device->dev);
+
+ /* Wake up on 5-button array events only. */
+ if (event == 0xc0 || !priv->array)
+ return;
+
+ if (sparse_keymap_entry_from_scancode(priv->array, event))
+ pm_wakeup_hard_event(&device->dev);
+ else
+ dev_info(&device->dev, "unknown event 0x%x\n", event);
+
+ return;
+ }

make the system wake up on a standard short press finally!

Is the "Wake up on 5-button array events only." assumption broad
enough to cover the various Intel systems that need this behavior?

Or am I just detecting another ACPI issue in fact that prevents this
system from setting up properly the 5 button array with BIOS 1.1.31?

It looks like priv->array is not set on your system with the new BIOS,
which means that intel_button_array_input_setup() is not called any
more, so either evaluating the HEBC method fails entirely, or its
return value is not as expected.

quoted

How would you recommend me to test that 2nd assumption?

Log the status and event_cap right after the

status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);

statement in intel_hid_probe().

Thanks,
Rafael

I don't see HEBC in the ASL for the 7275.  It would be good to provide
this information across both of the BIOS versions (working and non-working).

As well as what
        status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap);
evaluates as on both too.

Since this platform shipped with Win 8.1 initially rather than Win10,
It's possible 5 button array was not part of the INT33D5 spec at that time
and wasn't used by the Windows Intel HID filter driver.

My suspicion:
Win 8.1 didn't support 5 button array definition yet and those events that
are normally part of 5 button array (as detected by bit 17) were coming
across as regular HID event codes (like you were seeing 0xCE and 0xCF
unknown events from debug log when OS was running).

Perhaps someone from Intel can confirm that (the spec is not public).

Ugh.

OK, we'll see what can be done about that.

If that's correct, the intel-hid driver would need some changing to not
check HEBC, but just check BTNC directly every time and Rafael's patch
would need some changing too since it's only looking at button array event
not HID event.

Evaluating HEBC seems to be redundant, although on some systems it may
initialize something (I guess).

I'll try to cut a patch for this thing.

Thanks,
Rafael