diff mbox

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

Message ID 1872413.KDFIRZOQLU@aspire.rjw.lan (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Rafael J. Wysocki June 12, 2017, 8:34 p.m. UTC 
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(-)

Comments

Jérôme de Bretagne June 12, 2017, 10:50 p.m. UTC | #1 
>> 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,
>
Jérôme de Bretagne June 12, 2017, 11:29 p.m. UTC | #2 
>>> 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
Rafael J. Wysocki June 12, 2017, 11:37 p.m. UTC | #3 
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
Rafael J. Wysocki June 12, 2017, 11:52 p.m. UTC | #4 
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
Jérôme de Bretagne June 13, 2017, midnight UTC | #5 
>> 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
Rafael J. Wysocki June 13, 2017, 12:22 a.m. UTC | #6 
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
Jérôme de Bretagne June 13, 2017, 9:11 p.m. UTC | #7 
>>> 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
Jérôme de Bretagne July 9, 2017, 4:45 p.m. UTC | #8 
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
diff mbox

Patch

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,