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

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

[Jérôme de Bretagne]

Hi,

Here is some feedback for Dell Latitude 7275, as suggested by Srinivas
in https://bugzilla.kernel.org/show_bug.cgi?id=195897#c15 .

For some context on that model, suspend-to-RAM is not safe as the
system hangs (raised in the above bug report). I'm wondering if this
is expected somehow, due to S3 not being supported on that model, as
discussed for the XPS 13 9365. Maybe Mario or someone else from Dell
would be able to confirm this point.

While it's not set by default, suspend-to-idle works fine. However the
power button needs a very long press of about 6-7s to trigger the
wake-up (quite surprising at first).

I've applied Rafael's patch series on 4.12-rc4, with the 3rd patch
modified to add the Latitude 7275 DMI values, to check if the proposed
fix for the XPS 13 9360 and 9365 could also work for this other quite
recent Dell model.

Sadly with the latest BIOS 1.1.31, this patch set doesn't seem to
change anything: the long press is still needed.

Surprisingly and more for reference I suppose, this patch set works as
intended with an older BIOS 1.1.20 : the system can then be woken up
from system-to-idle with a usual short press. Cf.
https://bugzilla.kernel.org/show_bug.cgi?id=195897#c9 for more details
about why I've downgraded to and tested that specific older BIOS.

I'm wondering if there could be ways to modify slightly this patch
series to also cover the Latitude 7275 when running the current BIOS.
If I can provide any inputs that could be useful to investigate,
please let me know.

Thanks,
Jérome

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

[Rafael J. Wysocki]

Hi,

On Thu, Jun 8, 2017 at 1:05 AM, Jérôme de Bretagne
<jerome.debretagne@gmail.com> wrote:
> Hi,
>
> Here is some feedback for Dell Latitude 7275, as suggested by Srinivas
> in https://bugzilla.kernel.org/show_bug.cgi?id=195897#c15 .
>
> For some context on that model, suspend-to-RAM is not safe as the
> system hangs (raised in the above bug report). I'm wondering if this
> is expected somehow, due to S3 not being supported on that model, as
> discussed for the XPS 13 9365. Maybe Mario or someone else from Dell
> would be able to confirm this point.
>
> While it's not set by default, suspend-to-idle works fine. However the
> power button needs a very long press of about 6-7s to trigger the
> wake-up (quite surprising at first).
>
> I've applied Rafael's patch series on 4.12-rc4, with the 3rd patch
> modified to add the Latitude 7275 DMI values, to check if the proposed
> fix for the XPS 13 9360 and 9365 could also work for this other quite
> recent Dell model.
>
> Sadly with the latest BIOS 1.1.31, this patch set doesn't seem to
> change anything: the long press is still needed.
>
> Surprisingly and more for reference I suppose, this patch set works as
> intended with an older BIOS 1.1.20 : the system can then be woken up
> from system-to-idle with a usual short press. Cf.
> https://bugzilla.kernel.org/show_bug.cgi?id=195897#c9 for more details
> about why I've downgraded to and tested that specific older BIOS.
>
> I'm wondering if there could be ways to modify slightly this patch
> series to also cover the Latitude 7275 when running the current BIOS.

Maybe.

I'll look at the acpidump output data attached to the bug entry.

> If I can provide any inputs that could be useful to investigate,
> please let me know.

I only see one acpidump output attachment in the bug entry, so please
also attach the one from the other BIOS revision.

Thanks,
Rafael

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

[Jérôme de Bretagne]

Hi Rafael,

> I'll look at the acpidump output data attached to the bug entry.

Thanks.

> I only see one acpidump output attachment in the bug entry, so please
> also attach the one from the other BIOS revision.

Here you go, I've just attached the second one. I've also updated
their respective descriptions to better see which one is which.

Thanks,
Jérome

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

[Rafael J. Wysocki]

On Thu, Jun 8, 2017 at 1:38 AM, Jérôme de Bretagne
<jerome.debretagne@gmail.com> wrote:
> Hi Rafael,
>
>> I'll look at the acpidump output data attached to the bug entry.
>
> Thanks.
>
>> I only see one acpidump output attachment in the bug entry, so please
>> also attach the one from the other BIOS revision.
>
> Here you go, I've just attached the second one. I've also updated
> their respective descriptions to better see which one is which.

Thank you.

RE: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Mario.Limonciello]

Hi Jerome,

> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Wednesday, June 7, 2017 6:06 PM
> To: ACPI Devel Maling List <linux-acpi@vger.kernel.org>; linux-
> pm@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Darren Hart
> <dvhart@infradead.org>; Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on
> recent Dell laptops
> 
> Hi,
> 
> Here is some feedback for Dell Latitude 7275, as suggested by Srinivas
> in https://bugzilla.kernel.org/show_bug.cgi?id=195897#c15 .
> 
> For some context on that model, suspend-to-RAM is not safe as the
> system hangs (raised in the above bug report). I'm wondering if this
> is expected somehow, due to S3 not being supported on that model, as
> discussed for the XPS 13 9365. Maybe Mario or someone else from Dell
> would be able to confirm this point.
> 
The Latitude 7275 was not a machine that was tested with Linux during 
development.
This machine shipped with Windows 8, right?
I believe this machine also supports Connected Standby (the precursor
to Modern Standby that behaves very similarly).

I have an ask out to someone who can confirm that.	

> While it's not set by default, suspend-to-idle works fine. However the
> power button needs a very long press of about 6-7s to trigger the
> wake-up (quite surprising at first).
> 
> I've applied Rafael's patch series on 4.12-rc4, with the 3rd patch
> modified to add the Latitude 7275 DMI values, to check if the proposed
> fix for the XPS 13 9360 and 9365 could also work for this other quite
> recent Dell model.
> 
> Sadly with the latest BIOS 1.1.31, this patch set doesn't seem to
> change anything: the long press is still needed.
> 
> Surprisingly and more for reference I suppose, this patch set works as
> intended with an older BIOS 1.1.20 : the system can then be woken up
> from system-to-idle with a usual short press. Cf.
> https://bugzilla.kernel.org/show_bug.cgi?id=195897#c9 for more details
> about why I've downgraded to and tested that specific older BIOS.
> 
> I'm wondering if there could be ways to modify slightly this patch
> series to also cover the Latitude 7275 when running the current BIOS.
> If I can provide any inputs that could be useful to investigate,
> please let me know.
> 
> Thanks,
> Jérome

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

[Jérôme de Bretagne]

Hi Mario,

>> Here is some feedback for Dell Latitude 7275, as suggested by Srinivas
>> in https://bugzilla.kernel.org/show_bug.cgi?id=195897#c15 .
>>
>> For some context on that model, suspend-to-RAM is not safe as the
>> system hangs (raised in the above bug report). I'm wondering if this
>> is expected somehow, due to S3 not being supported on that model, as
>> discussed for the XPS 13 9365. Maybe Mario or someone else from Dell
>> would be able to confirm this point.
>>
> The Latitude 7275 was not a machine that was tested with Linux during
> development.
> This machine shipped with Windows 8, right?

I've read that this machine shipped at first with the option to have it either
with Windows 10 or Windows 8.1. Mine came with Windows 10.

> I believe this machine also supports Connected Standby (the precursor
> to Modern Standby that behaves very similarly).

I've in fact found some WinHEC slides stating that it was among the first
systems to support Modern Standby, here for reference:
https://f.ch9.ms/public/WinHEC/06_WinHECDec2016-DesigningPowerEfficient_v02.pptx

> I have an ask out to someone who can confirm that.

Thanks. I'm hoping there's a chance to make it work with suspend-to-RAM
on Linux still, in addition to suspend-to-idle, by fixing the current hang.

Thanks,
Jérome

RE: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Mario.Limonciello]

> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Thursday, June 8, 2017 6:44 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>; Linux PM <linux-
> pm@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>;
> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on
> recent Dell laptops
> 
> Hi Mario,
> 
> >> Here is some feedback for Dell Latitude 7275, as suggested by Srinivas
> >> in https://bugzilla.kernel.org/show_bug.cgi?id=195897#c15 .
> >>
> >> For some context on that model, suspend-to-RAM is not safe as the
> >> system hangs (raised in the above bug report). I'm wondering if this
> >> is expected somehow, due to S3 not being supported on that model, as
> >> discussed for the XPS 13 9365. Maybe Mario or someone else from Dell
> >> would be able to confirm this point.
> >>
> > The Latitude 7275 was not a machine that was tested with Linux during
> > development.
> > This machine shipped with Windows 8, right?
> 
> I've read that this machine shipped at first with the option to have it either
> with Windows 10 or Windows 8.1. Mine came with Windows 10.
> 
> > I believe this machine also supports Connected Standby (the precursor
> > to Modern Standby that behaves very similarly).
> 
> I've in fact found some WinHEC slides stating that it was among the first
> systems to support Modern Standby, here for reference:
> https://f.ch9.ms/public/WinHEC/06_WinHECDec2016-
> DesigningPowerEfficient_v02.pptx
> 
> > I have an ask out to someone who can confirm that.	
> 
> Thanks. I'm hoping there's a chance to make it work with suspend-to-RAM
> on Linux still, in addition to suspend-to-idle, by fixing the current hang.

I got a response back that confirmed that this machine only is supported
with CS/MS.  I would not expect S3 to work properly.

Even if S3 is part of the ACPI namespace, the behavior that Windows follows
is that S3 is not offered when the low power idle bit is set in FADT.

I know that the current behavior in Linux does not review low power idle
bit to determine whether to offer S2I or S3 so I'm inquiring if we can remove
S3 from a future BIOS update on this platform to let Linux work better
even though we don't ship this platform with Linux.

For now I would recommend that you focus efforts exclusively on making
S2I work, and ignore S3 problems.


Regarding the long button press necessary to wake the system, you might
not have seen the discussions and threads that explain this.
Essentially the EC is sending a SCI when you press the button press, and it's
supposed be interpreted by a driver and that driver would wake the system.
(intel-hid or intel-vbtn on Linux).
If you long press the power button, after a certain period of  time it uses
a GPIO to wake up the system.

It's unclear to me exactly when you tested s2idle-dell-test.
Can you re-test with Rafael's newest submissions and changes to s2idle-dell-test
committed 28 hours ago?
You'll still have to add your system to the DMI whitelist for the last patch
but he has reworked a bunch of related wakeup code that has also changed.

Thanks,

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

[Jérôme de Bretagne]

> I got a response back that confirmed that this machine only is supported
> with CS/MS.  I would not expect S3 to work properly.

Thanks for the confirmation, that's what I expected following the feedback on
other recent Dell machines.

> Even if S3 is part of the ACPI namespace, the behavior that Windows follows
> is that S3 is not offered when the low power idle bit is set in FADT.
>
> I know that the current behavior in Linux does not review low power idle
> bit to determine whether to offer S2I or S3

Is it something that is envisioned already as a future evolution in Linux as
well, if more and more systems are now designed with S2I in mind?

> so I'm inquiring if we can remove
> S3 from a future BIOS update on this platform to let Linux work better
> even though we don't ship this platform with Linux.

That would be a great fix, removing the default to S3 for this system on Linux.

> For now I would recommend that you focus efforts exclusively on making
> S2I work, and ignore S3 problems.

Well noted!

> It's unclear to me exactly when you tested s2idle-dell-test.
> Can you re-test with Rafael's newest submissions and changes to s2idle-dell-test
> committed 28 hours ago?
> You'll still have to add your system to the DMI whitelist for the last patch
> but he has reworked a bunch of related wakeup code that has also changed.

I've just done so, using the updated s2idle-dell-test branch + my DMI
adaptation.

I still get exactly the same results than during my previous tests:
- it provides the expected fix when running BIOS 1.1.20: short press to wake-up
- it doesn't fix with the current BIOS 1.1.31: long press still needed
to wake-up
so something has clearly changed between these 2 versions. Btw, the EC is
explicitly updated when flashing version 1.1.31, that's one of the
visible steps.

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.

RE: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Mario.Limonciello]

> -----Original Message-----
> From: Jérôme de Bretagne [mailto:jerome.debretagne@gmail.com]
> Sent: Friday, June 9, 2017 6:11 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>; Linux PM <linux-
> pm@vger.kernel.org>; Rafael J. Wysocki <rjw@rjwysocki.net>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>;
> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> Subject: Re: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on
> recent Dell laptops
> 
> > I got a response back that confirmed that this machine only is supported
> > with CS/MS.  I would not expect S3 to work properly.
> 
> Thanks for the confirmation, that's what I expected following the feedback on
> other recent Dell machines.

Specifically the Dell machines that aren't sold with Linux.  The machines sold
with Linux do undergo S3 testing and BIOS issues are sorted out for S3.

Those it makes sense to leave S3 in the BIOS until S2I is working well on Linux
at least.

> 
> > Even if S3 is part of the ACPI namespace, the behavior that Windows follows
> > is that S3 is not offered when the low power idle bit is set in FADT.
> >
> > I know that the current behavior in Linux does not review low power idle
> > bit to determine whether to offer S2I or S3
> 
> Is it something that is envisioned already as a future evolution in Linux as
> well, if more and more systems are now designed with S2I in mind?

Eventually this will change, but for now first we need to sort out the problems
with the systems that do S2I.

> 
> > so I'm inquiring if we can remove
> > S3 from a future BIOS update on this platform to let Linux work better
> > even though we don't ship this platform with Linux.
> 
> That would be a great fix, removing the default to S3 for this system on Linux.
> 
> > For now I would recommend that you focus efforts exclusively on making
> > S2I work, and ignore S3 problems.
> 
> Well noted!
> 
> > It's unclear to me exactly when you tested s2idle-dell-test.
> > Can you re-test with Rafael's newest submissions and changes to s2idle-dell-test
> > committed 28 hours ago?
> > You'll still have to add your system to the DMI whitelist for the last patch
> > but he has reworked a bunch of related wakeup code that has also changed.
> 
> I've just done so, using the updated s2idle-dell-test branch + my DMI
> adaptation.
> 
> I still get exactly the same results than during my previous tests:
> - it provides the expected fix when running BIOS 1.1.20: short press to wake-up
> - it doesn't fix with the current BIOS 1.1.31: long press still needed
> to wake-up
> so something has clearly changed between these 2 versions. Btw, the EC is
> explicitly updated when flashing version 1.1.31, that's one of the
> visible steps.
> 
> 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?  As far as I can tell 
the fix that went  into that was specifically related to power button 
handling off AC adapter.

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

[Jérôme de Bretagne]

Hi Mario, Hi Rafael,

>> 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?
How would you recommend me to test that 2nd assumption?

Thanks,
Jérome


P.S. I've gone in that direction after seeing the following messages in
 /var/log/kernel.log on normal power key press at runtime:
[ 2727.596359] intel-hid INT33D5:00: unknown event 0xce
[ 2727.769140] intel-hid INT33D5:00: unknown event 0xcf

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

[Srinivas Pandruvada]

On Mon, 2017-06-12 at 00:01 +0200, Jérôme de Bretagne wrote:
> Hi Mario, Hi Rafael,
> 
> > 
> > > 
> > > 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);
0xce is a 5 button array press and 0xcf is release.

> +
> + /* Wake up on 5-button array events only. */
> + if (event == 0xc0 || !priv->array)
> + return;
I don't think 0xc0 is any valid 5 button code.

> +
> + 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?
For the platforms which supported Win 8.1 only the event should be
handled by vbtn driver.

Thanks,
Srinivas

> 
> 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?
> How would you recommend me to test that 2nd assumption?
> 
> Thanks,
> Jérome
> 
> 
> P.S. I've gone in that direction after seeing the following messages
> in
>  /var/log/kernel.log on normal power key press at runtime:
> [ 2727.596359] intel-hid INT33D5:00: unknown event 0xce
> [ 2727.769140] intel-hid INT33D5:00: unknown event 0xcf

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

[Rafael J. Wysocki]

On Mon, Jun 12, 2017 at 12:01 AM, Jérôme de Bretagne
<jerome.debretagne@gmail.com> wrote:
> Hi Mario, Hi Rafael,
>
>>> 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.

> 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

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

[Rafael J. Wysocki]

On Mon, Jun 12, 2017 at 5:54 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Mon, 2017-06-12 at 00:01 +0200, Jérôme de Bretagne wrote:
>> Hi Mario, Hi Rafael,
>>
>> >
>> > >
>> > > 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);
> 0xce is a 5 button array press and 0xcf is release.

Right.  And it is there in intel_array_keymap[].

>> +
>> + /* Wake up on 5-button array events only. */
>> + if (event == 0xc0 || !priv->array)
>> + return;
> I don't think 0xc0 is any valid 5 button code.
>
>> +
>> + 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?
> For the platforms which supported Win 8.1 only the event should be
> handled by vbtn driver.

This is not intel-vbtn, but intel-hid.

Thanks,
Rafael

RE: [PATCH 0/3] ACPI / sleep: Support power button wakeup from S2I on recent Dell laptops

[Mario.Limonciello]

> -----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 <jerome.debretagne@gmail.com>
> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; ACPI Devel Maling List
> <linux-acpi@vger.kernel.org>; Linux PM <linux-pm@vger.kernel.org>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>;
> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>
> 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
> <jerome.debretagne@gmail.com> wrote:
> > Hi Mario, Hi Rafael,
> >
> >>> 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.
> 
> > 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).

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.

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

[Rafael J. Wysocki]

On Mon, Jun 12, 2017 at 10:00 PM,  <Mario.Limonciello@dell.com> wrote:
>> -----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 <jerome.debretagne@gmail.com>
>> Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; ACPI Devel Maling List
>> <linux-acpi@vger.kernel.org>; Linux PM <linux-pm@vger.kernel.org>; Rafael J.
>> Wysocki <rjw@rjwysocki.net>; Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com>; Darren Hart <dvhart@infradead.org>;
>> Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>; Mika Westerberg
>> <mika.westerberg@linux.intel.com>
>> 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
>> <jerome.debretagne@gmail.com> wrote:
>> > Hi Mario, Hi Rafael,
>> >
>> >>> 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.
>>
>> > 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

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

[Rafael J. Wysocki]

On Monday, June 12, 2017 10:35:50 PM Jérôme de Bretagne wrote:
> > 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.
> 
> I'll provide both BTNC and HEBC log results to compare, with both BIOS
> versions, a bit later today.
> 
> > 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).
> 
> That was my assumption also, since my earlier 3-liner patch was doing
> this exactly: trying to wake up on a regular 0xCE event. And it did work.

OK

Can you try the patch below, please?

---
 drivers/platform/x86/intel-hid.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/platform/x86/intel-hid.c
===================================================================
--- linux-pm.orig/drivers/platform/x86/intel-hid.c
+++ linux-pm/drivers/platform/x86/intel-hid.c
@@ -270,12 +270,17 @@ static int intel_hid_probe(struct platfo
 	}
 
 	/* Setup 5 button array */
-	status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
-	if (ACPI_SUCCESS(status) && (event_cap & 0x20000)) {
-		dev_info(&device->dev, "platform supports 5 button array\n");
-		err = intel_button_array_input_setup(device);
-		if (err)
-			pr_err("Failed to setup Intel 5 button array hotkeys\n");
+	if (acpi_has_method(handle, "BTNC") && acpi_has_method(handle, "BTNE")) {
+		unsigned long long event_cap;
+
+		status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
+		if ((ACPI_SUCCESS(status) && (event_cap & 0x20000)) ||
+		    status == AE_NOT_FOUND) {
+			dev_info(&device->dev, "5-button array supported\n");
+			err = intel_button_array_input_setup(device);
+			if (err)
+				dev_dbg(&device->dev, "5-button array setup failure\n");
+		}
 	}
 
 	status = acpi_install_notify_handler(handle,

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

[Jérôme de Bretagne]

> 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.

I'll provide both BTNC and HEBC log results to compare, with both BIOS
versions, a bit later today.

> 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).

That was my assumption also, since my earlier 3-liner patch was doing
this exactly: trying to wake up on a regular 0xCE event. And it did work.

Jérome

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

[Jérôme de Bretagne]

2017-06-12 22:35 GMT+02:00 Jérôme de Bretagne <jerome.debretagne@gmail.com>:
>> 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.
>
> I'll provide both BTNC and HEBC log results to compare, with both BIOS
> versions, a bit later today.

Here are both requested logs, with BIOS 1.1.31 first:

* right after status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
[    4.167874] intel-hid INT33D5:00: intel_hid_probe: event_cap =
0xffff9bfe71bdc410
[    4.167875] intel-hid INT33D5:00: intel_hid_probe: status = 5

* right after status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap);
=> No log output for that one


And with BIOS 1.1.20 then:

* right after status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
[    4.172543] intel-hid INT33D5:00: intel_hid_probe: event_cap =
0xffff8d1f70950810
[    4.172545] intel-hid INT33D5:00: intel_hid_probe: status = 5

* right after status = acpi_evaluate_integer(handle, "BTNC", NULL, &button_cap);
=> No log output for that one


Status = 5 confirms the assumption that HEBC is not found in both cases.
I guess the absence of logs for BTNC is more surprising, isn't it?

Jérome

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

[Jérôme de Bretagne]

>> That was my assumption also, since my earlier 3-liner patch was doing
>> this exactly: trying to wake up on a regular 0xCE event. And it did work.
>
> OK
>
> Can you try the patch below, please?

Thank you for this patch proposal. I've applied it but the system with
BIOS 1.1.31 doesn't wake up on short press.

I can see the following (added) messages in the logs for info:

- short press first while suspended -> no wake-up
[  702.567912] intel-hid INT33D5:00: notify_handler with event 0xce
[  702.567913] intel-hid INT33D5:00: 0xce == 0xc0 || !priv->array
[  702.765067] intel-hid INT33D5:00: notify_handler with event 0xcf
[  702.765072] intel-hid INT33D5:00: 0xcf == 0xc0 || !priv->array
- long press then -> waking up the system
[  704.954703] intel-hid INT33D5:00: notify_handler with event 0xce
[  704.954704] intel-hid INT33D5:00: 0xce == 0xc0 || !priv->array
[  711.646088] intel-hid INT33D5:00: notify_handler with event 0xcf
[  711.646092] intel-hid INT33D5:00: unknown event 0xcf

confirming that priv->array is indeed not set, even with the patch below.

With BIOS 1.1.20, I don't have such logs, notify_handler seems never called.

Jérome

> ---
>  drivers/platform/x86/intel-hid.c |   17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/platform/x86/intel-hid.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/intel-hid.c
> +++ linux-pm/drivers/platform/x86/intel-hid.c
> @@ -270,12 +270,17 @@ static int intel_hid_probe(struct platfo
>         }
>
>         /* Setup 5 button array */
> -       status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
> -       if (ACPI_SUCCESS(status) && (event_cap & 0x20000)) {
> -               dev_info(&device->dev, "platform supports 5 button array\n");
> -               err = intel_button_array_input_setup(device);
> -               if (err)
> -                       pr_err("Failed to setup Intel 5 button array hotkeys\n");
> +       if (acpi_has_method(handle, "BTNC") && acpi_has_method(handle, "BTNE")) {
> +               unsigned long long event_cap;
> +
> +               status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
> +               if ((ACPI_SUCCESS(status) && (event_cap & 0x20000)) ||
> +                   status == AE_NOT_FOUND) {
> +                       dev_info(&device->dev, "5-button array supported\n");
> +                       err = intel_button_array_input_setup(device);
> +                       if (err)
> +                               dev_dbg(&device->dev, "5-button array setup failure\n");
> +               }
>         }
>
>         status = acpi_install_notify_handler(handle,
>

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

[Jérôme de Bretagne]

>>> That was my assumption also, since my earlier 3-liner patch was doing
>>> this exactly: trying to wake up on a regular 0xCE event. And it did work.

One more ouput for tonight. After doing a diff of the acpidump from
both BIOS versions, I've found explicit references to the two 0xCE and
0xCF values right in the delta, in the dsdt.dsl files.

Here is the section that seems relevant in the newer one, for v1.1.31:

        If (Local1 & 0x08)
        {
            Local1 = ECBT (One, 0x04)
            If (Local1)
            {
                If (OSYS >= 0x07DF)
                {
                    Notify (\_SB.HIDD, 0xCE) // Hardware-Specific
                }
                ElseIf (CondRefOf (\_SB.PCI0.LPCB.ECDV.VGBI))
                {
                    Notify (\_SB.PCI0.LPCB.ECDV.VGBI, 0xC0) // Hardware-Specific
                }
            }
            ElseIf (OSYS >= 0x07DF)
            {
                Notify (\_SB.HIDD, 0xCF) // Hardware-Specific
            }
            ElseIf (CondRefOf (\_SB.PCI0.LPCB.ECDV.VGBI))
            {
                Notify (\_SB.PCI0.LPCB.ECDV.VGBI, 0xC1) // Hardware-Specific
            }
        }

        If (Local1 & 0x0100)
        {
            Local1 = ECBT (One, 0x10)
            If (Local1)
            {
                Notify (\_SB.HIDD, 0xC6) // Hardware-Specific
            }
            Else
            {
                Notify (\_SB.HIDD, 0xC7) // Hardware-Specific
            }
        }


I'm new to ACPI but could that ECBT mean "EC Button" maybe?

Jerome

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

[Rafael J. Wysocki]

On Tue, Jun 13, 2017 at 12:50 AM, Jérôme de Bretagne
<jerome.debretagne@gmail.com> wrote:
>>> That was my assumption also, since my earlier 3-liner patch was doing
>>> this exactly: trying to wake up on a regular 0xCE event. And it did work.
>>
>> OK
>>
>> Can you try the patch below, please?
>
> Thank you for this patch proposal. I've applied it but the system with
> BIOS 1.1.31 doesn't wake up on short press.
>
> I can see the following (added) messages in the logs for info:
>
> - short press first while suspended -> no wake-up
> [  702.567912] intel-hid INT33D5:00: notify_handler with event 0xce
> [  702.567913] intel-hid INT33D5:00: 0xce == 0xc0 || !priv->array
> [  702.765067] intel-hid INT33D5:00: notify_handler with event 0xcf
> [  702.765072] intel-hid INT33D5:00: 0xcf == 0xc0 || !priv->array
> - long press then -> waking up the system
> [  704.954703] intel-hid INT33D5:00: notify_handler with event 0xce
> [  704.954704] intel-hid INT33D5:00: 0xce == 0xc0 || !priv->array
> [  711.646088] intel-hid INT33D5:00: notify_handler with event 0xcf
> [  711.646092] intel-hid INT33D5:00: unknown event 0xcf
>
> confirming that priv->array is indeed not set, even with the patch below.
>
> With BIOS 1.1.20, I don't have such logs, notify_handler seems never called.

OK

I've just had a look at the acpidump output files you attached to
https://bugzilla.kernel.org/show_bug.cgi?id=195897 and actually none
of them has anything like BTNC or BTNE or similar under the device
object intel-hid binds to.  In fact, the definition of that device
object on both the BIOS versions is exactly the same AFAICS.

However, BIOS 1.1.31 contains additional Notify () invocations in the
NEVT () method used for processing EC events (invoked by _Q66) with
event values corresponding to the 5-button array power button
scancodes.  That's why your first debug patch works.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Tue, Jun 13, 2017 at 1:29 AM, Jérôme de Bretagne
<jerome.debretagne@gmail.com> wrote:
>>>> That was my assumption also, since my earlier 3-liner patch was doing
>>>> this exactly: trying to wake up on a regular 0xCE event. And it did work.
>
> One more ouput for tonight. After doing a diff of the acpidump from
> both BIOS versions, I've found explicit references to the two 0xCE and
> 0xCF values right in the delta, in the dsdt.dsl files.

Right.  That's what my last message was about. :-)

> Here is the section that seems relevant in the newer one, for v1.1.31:
>
>         If (Local1 & 0x08)
>         {
>             Local1 = ECBT (One, 0x04)
>             If (Local1)
>             {
>                 If (OSYS >= 0x07DF)
>                 {
>                     Notify (\_SB.HIDD, 0xCE) // Hardware-Specific
>                 }
>                 ElseIf (CondRefOf (\_SB.PCI0.LPCB.ECDV.VGBI))
>                 {
>                     Notify (\_SB.PCI0.LPCB.ECDV.VGBI, 0xC0) // Hardware-Specific
>                 }
>             }
>             ElseIf (OSYS >= 0x07DF)
>             {
>                 Notify (\_SB.HIDD, 0xCF) // Hardware-Specific
>             }
>             ElseIf (CondRefOf (\_SB.PCI0.LPCB.ECDV.VGBI))
>             {
>                 Notify (\_SB.PCI0.LPCB.ECDV.VGBI, 0xC1) // Hardware-Specific
>             }
>         }
>
>         If (Local1 & 0x0100)
>         {
>             Local1 = ECBT (One, 0x10)
>             If (Local1)
>             {
>                 Notify (\_SB.HIDD, 0xC6) // Hardware-Specific
>             }
>             Else
>             {
>                 Notify (\_SB.HIDD, 0xC7) // Hardware-Specific
>             }
>         }
>
>
> I'm new to ACPI but could that ECBT mean "EC Button" maybe?

That's just a method name which may or may not mean the above.

\_SB.PCI0.LPCB.ECDV is the EC, so it is all related.

However, the bottom line is that there is no way for the kernel at all
to know in advance that these events are going to be generated.

Thanks,
Rafael

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

[Jérôme de Bretagne]

>> One more ouput for tonight. After doing a diff of the acpidump from
>> both BIOS versions, I've found explicit references to the two 0xCE and
>> 0xCF values right in the delta, in the dsdt.dsl files.
>
> Right.  That's what my last message was about. :-)

Indeed, our messages have crossed each other :-)

>> I'm new to ACPI but could that ECBT mean "EC Button" maybe?
>
> That's just a method name which may or may not mean the above.
>
> \_SB.PCI0.LPCB.ECDV is the EC, so it is all related.
>
> However, the bottom line is that there is no way for the kernel at all
> to know in advance that these events are going to be generated.

Ok, so I understand that this is totally device-specific :-( Would a patch
to handle this specific case be acceptable in the kernel though?

Thanks,
Jerome

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

[Rafael J. Wysocki]

On Tue, Jun 13, 2017 at 2:00 AM, Jérôme de Bretagne
<jerome.debretagne@gmail.com> wrote:
>>> One more ouput for tonight. After doing a diff of the acpidump from
>>> both BIOS versions, I've found explicit references to the two 0xCE and
>>> 0xCF values right in the delta, in the dsdt.dsl files.
>>
>> Right.  That's what my last message was about. :-)
>
> Indeed, our messages have crossed each other :-)
>
>>> I'm new to ACPI but could that ECBT mean "EC Button" maybe?
>>
>> That's just a method name which may or may not mean the above.
>>
>> \_SB.PCI0.LPCB.ECDV is the EC, so it is all related.
>>
>> However, the bottom line is that there is no way for the kernel at all
>> to know in advance that these events are going to be generated.
>
> Ok, so I understand that this is totally device-specific :-(

Well, device-and-BIOS-version-specific even.

> Would a patch to handle this specific case be acceptable in the kernel though?

We have quirks. :-)

That said I think intel-hid could check the 0xCE event unconditionally
in the "wakeup" mode, but that will be a separate patch on top of my
series.

Thanks,
Rafael

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

[Jérôme de Bretagne]

>>> However, the bottom line is that there is no way for the kernel at all
>>> to know in advance that these events are going to be generated.
>>
>> Would a patch to handle this specific case be acceptable in the kernel though?
>
> We have quirks. :-)
>
> That said I think intel-hid could check the 0xCE event unconditionally
> in the "wakeup" mode, but that will be a separate patch on top of my
> series.

Thank you again, Rafael and Mario, to have helped understand the
special behavior implemented on this system / BIOS combination. I
guess I'll wait for your s2-idle-dell-test patch series to land first, to
discuss then how to handle this other Dell model (with Alex Hung?).

Or do you have another suggestion about how to proceed?

In the meantime, I'll update the bug report I had created:
https://bugzilla.kernel.org/show_bug.cgi?id=195897
to track and share there all the discoveries from this thread.

Jerome

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

[Jérôme de Bretagne]

Hi Rafael,

>>> Would a patch to handle this specific case be acceptable in the kernel though?
>>
>> We have quirks. :-)
>>
>> That said I think intel-hid could check the 0xCE event unconditionally
>> in the "wakeup" mode, but that will be a separate patch on top of my
>> series.
>
> I guess I'll wait for your s2-idle-dell-test patch series to land first, to
> discuss then how to handle this other Dell model (with Alex Hung?).
>
> In the meantime, I'll update the bug report I had created:
> https://bugzilla.kernel.org/show_bug.cgi?id=195897

It seems your series should land in 4.13 so I'm looking at proposing a
patch on top of it, as discussed.

However, I realize that I only know how to handle the wakeup case but
not the suspend one which doesn't work either. Indeed I've always
triggered suspend via the command line so far.

In intel-hid.c I see no obvious code referring to suspend. With added
debug logs, I can see that notify_handler receives the same 0xce / 0xcf
Notify events upon power button press when the system is running.

What would be the expected way then to trigger a normal suspend
sequence, when catching these uncommon events? Should a more
standard event be created instead? If that's a right approach, is
there existing kernel code to do so ?

Thank you,
Jerome

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

[Rafael J. Wysocki]

On Friday, June 02, 2017 11:06:49 AM Tom Lanyon wrote:
> On 2 June 2017 at 00:59, Rafael J. Wysocki <rafael@kernel.org> 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

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

[Tom Lanyon]

On 2 June 2017 at 00:59, Rafael J. Wysocki <rafael@kernel.org> 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

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

[Rafael J. Wysocki]

On Thu, Jun 1, 2017 at 12:43 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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 <andriy.shevchenko@linux.intel.com>

Thanks!

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

[Rafael J. Wysocki]

Hi,

On Thu, Jun 1, 2017 at 1:50 PM, Tom Lanyon <tom@oneshoeco.com> 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

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

[Tom Lanyon]

[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

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

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Rafael J. Wysocki]

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 issue at hand is still the same as before:

On Wednesday, April 26, 2017 11:21:11 PM Rafael J. Wysocki wrote:
>
> The underlying issue is that on some relatively new Dell laltops, including
> Dell XPS13 9360 and 9365, pressing the power button is not sufficient to
> wake up the system from suspend-to-idle (it has to be pressed and held
> down for around 5 sec for the wakeup to happen) which is not expected
> and does not match the Windows' behavior.
> 
> This turns out to be a consequence of the way power button events are signaled
> on those systems, which is through the Embedded Controller (EC).  Namely,
> button events go to the EC which then signals the event through its ACPI GPE
> (General Purpose Event), which triggers an ACPI SCI (System Control Interrupt),
> whose handler executes a specicif AML control method and triggers a Notify()
> targetting a devie object associated with the power button.  The problem with
> suspend-to-idle is that the EC GPE is disabled during suspend, because
> otherwise all EC events would wake up the system from suspend-to-idle (and
> there can be many of them).

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.

The third patch allows the EC GPE to become a wakeup GPE on the affected Dell
laptops and finally makes power button events wake up those systems from
suspend-to-idle.

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.

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.

Thanks,
Rafael