Message ID | 1872413.KDFIRZOQLU@aspire.rjw.lan (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
>> 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, >
>>> 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
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
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
>> 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
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
>>> 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
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
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,