RE: [PATCH v2 4/4] ACPI / button: Add document for ACPI control method lid device restrictions

From: Zheng, Lv <hidden>
Date: 2016-07-12 07:36:03
Also in: linux-acpi, lkml

Hi,

From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
owner@vger.kernel.org] On Behalf Of Benjamin Tissoires
Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
method lid device restrictions

[I just realised Ctrl+enter means "send" for gmail, see the end of the
answers]

On Mon, Jul 11, 2016 at 1:42 PM, Benjamin Tissoires
[off-list ref] wrote:

quoted

On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv [off-list ref] wrote:

quoted

Hi,

quoted

From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI

control

quoted

method lid device restrictions

Hi,

On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng [off-list ref] wrote:

quoted

There are many AML tables reporting wrong initial lid state, and

some of

quoted

them never reports lid state. As a proxy layer acting between, ACPI

button

quoted

driver is not able to handle all such cases, but need to re-define the
usage model of the ACPI lid. That is:
1. It's initial state is not reliable;
2. There may not be open event;
3. Userspace should only take action against the close event which is
   reliable, always sent after a real lid close.
This patch adds documentation of the usage model.

Link: https://lkml.org/2016/3/7/460
Link: https://github.com/systemd/systemd/issues/2087
Signed-off-by: Lv Zheng <redacted>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: Benjamin Tissoires <redacted>
Cc: linux-input@vger.kernel.org
---
 Documentation/acpi/acpi-lid.txt |   62

+++++++++++++++++++++++++++++++++++++++

quoted

 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/acpi/acpi-lid.txt

diff --git a/Documentation/acpi/acpi-lid.txt

b/Documentation/acpi/acpi-

quoted

lid.txt

quoted

new file mode 100644
index 0000000..7e4f7ed

--- /dev/null
+++ b/Documentation/acpi/acpi-lid.txt

@@ -0,0 +1,62 @@
+Usage Model of the ACPI Control Method Lid Device
+
+Copyright (C) 2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+Platforms containing lids convey lid state (open/close) to OSPMs

using

quoted

+control method lid device. To implement this, the AML tables issue
+Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state

has

quoted

+changed. The _LID control method for the lid device must be

implemented to

quoted

+report the "current" state of the lid as either "opened" or "closed".
+
+This document describes the restrictions and the expections of the

Linux

quoted

+ACPI lid device driver.
+
+
+1. Restrictions of the returning value of the _LID control method
+
+The _LID control method is described to return the "current" lid

state.

quoted

+However the word of "current" has ambiguity, many AML tables

return

quoted

the lid

quoted

+state upon the last lid notification instead of returning the lid state
+upon the last _LID evaluation. There won't be difference when the

_LID

quoted

+control method is evaluated during the runtime, the problem is its

initial

quoted

+returning value. When the AML tables implement this control

method

quoted

with

quoted

+cached value, the initial returning value is likely not reliable. There

are

quoted

+simply so many examples always retuning "closed" as initial lid

state.

quoted

+
+2. Restrictions of the lid state change notifications
+
+There are many AML tables never notifying when the lid device

state is

quoted

+changed to "opened". But it is ensured that the AML tables always

notify

quoted

+"closed" when the lid state is changed to "closed". This is normally

used

quoted

+to trigger some system power saving operations on Windows.

Since it is

quoted

+fully tested, this notification is reliable for all AML tables.
+
+3. Expections for the userspace users of the ACPI lid device driver
+
+The userspace programs should stop relying on
+/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is

only

quoted

+used for the validation purpose.

I'd say: this file actually calls the _LID method described above. And
given the previous explanation, it is not reliable enough on some
platforms. So it is strongly advised for user-space program to not
solely rely on this file to determine the actual lid state.

[Lv Zheng]
OK.

quoted

+
+New userspace programs should rely on the lid "closed"

notification to

quoted

+trigger some power saving operations and may stop taking actions

according

quoted

+to the lid "opened" notification. A new input switch event -

SW_ACPI_LID is

quoted

+prepared for the new userspace to implement this ACPI control

method

quoted

lid

quoted

+device specific logics.

That's not entirely what we discussed before (to prevent regressions):
- if the device doesn't have reliable LID switch state, then there
would be the new input event, and so userspace should only rely on
opened notifications.
- if the device has reliable switch information, the new input event
should not be exported and userspace knows that the current input
switch event is reliable.

Also, using a new "switch" event is a terrible idea. Switches have a
state (open/close) and you are using this to forward a single open
event. So using a switch just allows you to say to userspace you are
using the "new" LID meaning, but you'll still have to manually reset
the switch and you will have to document how this event is not a
switch.

Please use a simple KEY_LID_OPEN event you will send through
[input_key_event(KEY_LID_OPEN, 1), input_sync(),
input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace

knows

quoted

how to handle.

[Lv Zheng]
It should be KEY_LID_CLOSE.

yep, sorry.

quoted

However I checked the KEY code definitions.
It seems their values are highly dependent on the HID specification.

That was convenient enough when the code was written. Now, we can
extend new keycodes as we want, as long as Dmitry agrees :)

quoted

I'm not sure which key code value should I use for this.
Could you point me out?

I think using 0x277 (or 0x278 given that KEY_DATA is reusing
KEY_FASTREVERSE) would be fine.

[Lv Zheng] 
Got it!
Thanks.

quoted

+
+During the period the userspace hasn't been switched to use the

new

quoted

+SW_ACPI_LID event, Linux users can use the following boot

parameter

quoted

to

quoted

+handle possible issues:
+  button.lid_init_state=method:
+   This is the default behavior of the Linux ACPI lid driver, Linux

kernel

quoted

+   reports the initial lid state using the returning value of the _LID
+   control method.
+   This can be used to fix some platforms if the _LID control

method's

quoted

+   returning value is reliable.
+  button.lid_init_state=open:
+   Linux kernel always reports the initial lid state as "opened".
+   This may fix some platforms if the returning value of the _LID

control

quoted

+   method is not reliable.

This worries me as there is no plan after "During the period the
userspace hasn't been switched to use the new event".

I really hope you'll keep sending SW_LID for reliable LID platforms,
and not remove it entirely as you will break platforms.

[Lv Zheng]
We won't remove SW_LID from the kernel :).

And we haven't removed SW_LID from the acpi button driver.
We'll just stop sending "initial lid state" from acpi button driver, i.e.,

the behavior carried out by "button.lid_init_state=ignore".

That won't do for the very same use case than before. It makes sense
to boot a laptop while the LID is closed if you have an external
monitor plugged in (the docking station allows to have an extra power
button accessible when the lid is closed).

[Lv Zheng] 
Exactly.
The "button.lid_init_state=ignore" is the original behavior of the ACPI button driver.

quoted

Maybe it is not sufficient, after the userspace has been changed to

support the new event, we should stop sending SW_LID from acpi button
driver.

I'd say do not touch SW_LID at all (and allow users to tweak its
behavior for local fixes, which is what you currently have).
Just send the extra KEY_LID_CLOSE, no matter what.
And start listing which devices have troubles, and we can add those
into a hwdb file shipped with logind. I hope the systemd team will
agree with me.

[Lv Zheng] 
OK.
We'll provide the dmidecode files for those platforms via an off-list way after the agreement is reached.
Thanks for the help.

Cheers
-Lv

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help