[PATCH 0/5] ACPI: ACPI documentations and trivial fixes

[PATCH 0/5] ACPI: ACPI documentations and trivial fixes

[Lv Zheng]

This patch adds ACPI documentations related to:
1. AML debugger
2. Control method LID device
3. ACPICA release process
This patch also contains several trivial fixes detected during the
documentation work.

Lv Zheng (5):
  ACPI: Add documentation describing ACPICA release automation
  ACPI / debugger: Fix regressions that AML debugger stops working
  ACPI / debugger: Add AML debugger documentation
  ACPI / button: Add SW_ACPI_LID for new usage model
  ACPI: Add configuration item to configure ACPICA error logs out

 Documentation/acpi/acpi-lid.txt         |   42 ++++++
 Documentation/acpi/aml-debugger.txt     |   56 +++++++
 Documentation/acpi/linuxized-acpica.txt |  251 +++++++++++++++++++++++++++++++
 drivers/acpi/Kconfig                    |    7 +
 drivers/acpi/acpi_dbg.c                 |    6 +-
 drivers/acpi/button.c                   |   20 ++-
 include/acpi/platform/aclinux.h         |    4 +
 include/linux/mod_devicetable.h         |    2 +-
 include/uapi/linux/input-event-codes.h  |    3 +-
 9 files changed, 381 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/acpi/acpi-lid.txt
 create mode 100644 Documentation/acpi/aml-debugger.txt
 create mode 100644 Documentation/acpi/linuxized-acpica.txt

-- 
1.7.10

[PATCH 1/5] ACPI: Add documentation describing ACPICA release automation

[Lv Zheng]

This patch adds documentation on ACPICA release automation into the kernel
Documentation/acpi folder.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 Documentation/acpi/linuxized-acpica.txt |  251 +++++++++++++++++++++++++++++++
 1 file changed, 251 insertions(+)
 create mode 100644 Documentation/acpi/linuxized-acpica.txt

diff --git a/Documentation/acpi/linuxized-acpica.txt b/Documentation/acpi/linuxized-acpica.txt
new file mode 100644
index 0000000..aec60c2
--- /dev/null
+++ b/Documentation/acpi/linuxized-acpica.txt
@@ -0,0 +1,251 @@
+Linuxized ACPICA - Introduction of ACPICA Release Automation
+
+Copyright (C) 2013-2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+This document describes the ACPICA project and the relationship between
+ACPICA and Linux.  It also includes the descriptions on how ACPICA code is
+automatically released to the following Linux subdirectories:
+drivers/acpi/acpica, include/acpi and tools/power/acpi.
+
+
+1. ACPICA Project
+
+   The ACPI Component Architecture (ACPICA) project provides an operating
+   system (OS)-independent reference implementation of the Advanced
+   Configuration and Power Interface Specification (ACPI).  It has been
+   adapted by various host OSes.  By directly integrating ACPICA, Linux can
+   also benefit from the application experiences of ACPICA from other host
+   OSes.
+
+   The homepage of ACPICA project is: www.acpica.org, it is maintained and
+   supported by Intel Corporation.
+
+   The following figure dipicts the Linux ACPI subystem where the ACPICA
+   adaption is included:
+
+      +---------------------------------------------------------+
+      |                                                         |
+      |   +---------------------------------------------------+ |
+      |   | +------------------+                              | |
+      |   | | Table Management |                              | |
+      |   | +------------------+                              | |
+      |   | +----------------------+                          | |
+      |   | | Namespace Management |                          | |
+      |   | +----------------------+                          | |
+      |   | +------------------+       ACPICA Components      | |
+      |   | | Event Management |                              | |
+      |   | +------------------+                              | |
+      |   | +---------------------+                           | |
+      |   | | Resource Management |                           | |
+      |   | +---------------------+                           | |
+      |   | +---------------------+                           | |
+      |   | | Hardware Management |                           | |
+      |   | +---------------------+                           | |
+      | +---------------------------------------------------+ | |
+      | | |                            +------------------+ | | |
+      | | |                            | OS Service Layer | | | |
+      | | |                            +------------------+ | | |
+      | | +-------------------------------------------------|-+ |
+      | |   +--------------------+                          |   |
+      | |   | Device Enumeration |                          |   |
+      | |   +--------------------+                          |   |
+      | |   +------------------+                            |   |
+      | |   | Power Management |                            |   |
+      | |   +------------------+     Linux/ACPI Components  |   |
+      | |   +--------------------+                          |   |
+      | |   | Thermal Management |                          |   |
+      | |   +--------------------+                          |   |
+      | |   +--------------------------+                    |   |
+      | |   | Drivers for ACPI Devices |                    |   |
+      | |   +--------------------------+                    |   |
+      | |   +--------+                                      |   |
+      | |   | ...... |                                      |   |
+      | |   +--------+                                      |   |
+      | +---------------------------------------------------+   |
+      |                                                         |
+      +---------------------------------------------------------+
+
+                 Figure 1. Linux ACPI Software Components
+
+   NOTE:
+    A. OS Service Layer - Provided by Linux to offer OS dependent
+       implementation of predefined ACPICA interfaces (acpi_os_*).
+         include/acpi/acpiosxf.h
+         drivers/acpi/osl.c
+         include/acpi/platform
+    B. ACPICA Functionalities - Released from ACPICA code base to offer
+       OS independent implementation of ACPICA interfaces (acpi_*).
+         drivers/acpi/acpica
+         include/acpi/ac*.h
+         tools/power/acpi
+    C. Linux/ACPI Functionalities - Providing Linux specific ACPI
+       functionalities to other Linux kernel subsystems and user space
+       programs.
+         drivers/acpi
+         include/linux/acpi.h
+         include/linux/acpi*.h
+         include/acpi
+         tools/power/acpi
+    D. Architecture Specific ACPICA/ACPI Functionalities - Provided by
+       ACPI subsystem to offer architecture specific implementation of ACPI
+       interfaces.  They are Linux specific components, and is out of the
+       scope of this document.
+         include/asm/acpi.h
+         include/asm/acpi*.h
+         arch/*/acpi
+
+2. ACPICA Release
+
+   ACPICA project maintains a code base at the following repository URL:
+   https://github.com/acpica/acpica.git.  It is released once a month.
+
+   As the coding style adopted by the ACPICA project is not acceptable by
+   Linux, there is a release process to convert the ACPICA GIT commits into
+   the Linux acceptable patches.  The patches generated by this process are
+   known as "Linuxized ACPICA Patches".  The release process is performed
+   against a local copy of the ACPICA git repository.  Each commit in the
+   monthly release is converted into the linuxized ACPICA patch.  They form
+   a montly ACPICA release patchset for Linux ACPI community.  The
+   following figure dipicts the Linux upstream process of the ACPICA
+   commits:
+
+    +-----------------------------+
+    | acpica / master (-) commits |
+    +-----------------------------+
+       /|\         |
+        |         \|/
+        |  /---------------------\    +----------------------+
+        | < Linuxize repo Utility >-->| old linuxized acpica |--+
+        |  \---------------------/    +----------------------+  |
+        |                                                       |
+     /---------\                                                |
+    < git reset >                                                \
+     \---------/                                                  \
+       /|\                                                        /+-+
+        |                                                        /   |
+    +-----------------------------+                             |    |
+    | acpica / master (+) commits |                             |    |
+    +-----------------------------+                             |    |
+                   |                                            |    |
+                  \|/                                           |    |
+         /-----------------------\    +----------------------+  |    |
+        < Linuxize repo Utilities >-->| new linuxized acpica |--+    |
+         \-----------------------/    +----------------------+       |
+                                                                    \|/
+    +--------------------------+                  /----------------------\
+    | Linuxized ACPICA Patches |<----------------< Linuxize patch Utility >
+    +--------------------------+                  \----------------------/
+                   |
+                  \|/
+     /---------------------------\
+    < Linux ACPI Community Review >
+     \---------------------------/
+                   |
+                  \|/
+    +-----------------------+   /------------------\    +----------------+
+    | linux-pm / linux-next |--< Linux Merge Window >-->| linux / master |
+    +-----------------------+   \------------------/    +----------------+
+
+                Figure 2. ACPICA -> Linux Upstream Process
+
+   NOTE:
+    A. Linuxize Utilities - Provided by ACPICA repository, including a
+       utility located in source/tools/acpisrc folder and a bunch of
+       scripts located in generate/linux folder.
+    B. acpica / master - "master" branch of the git repository at
+       <https://github.com/acpica/acpica.git>.
+    C. linux-pm / linux-next - "linux-next" branch of the git repository at
+       <http://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git>.
+    D. linux / master - "master" branch of the git repository at
+       <http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git>.
+    E. Linux ACPI Community - Reached at
+       <mailto:linux-acpi@vger.kernel.org>.
+
+   Before the linuxized ACPICA patches are sent to the Linux ACPI community
+   for review, there is a quality ensurance build test process performed to
+   reduce porting issues. Currently this build process only cares about the
+   following kernel configurations:
+   CONFIG_ACPI/CONFIG_ACPI_DEBUG/CONFIG_ACPI_DEBUGGER
+
+3. ACPICA Divergence
+
+   It's ideal that if all of the ACPICA commits are upstreamed
+   automatically without modifications, the "linux / master" tree should
+   contain the ACPICA code that is exactly same as the ACPICA code
+   contained in "new linuxized acpica" tree and the release process thus
+   could be performed automatically.
+
+   But the facts are there are always source code differences between
+   Linux and ACPICA, such differences are known as "ACPICA Divergences".
+
+   The various sources of the ACPICA divergences include:
+   1. The existing divergences - Before the ACPICA release cycle is
+      fully running, there has already been divergences between Linux and
+      ACPICA. Over the past several years, the divergences are greatly
+      reduced, but there are still several ones and it takes time to figure
+      out the root cause of these old divergences.
+   2. Manual modifications - Any manual modification (ex., coding style
+      fixes) done directly in the Linux side obviously hurts the ACPICA
+      release automation.  Thus it is recommended to fix issues in the
+      ACPICA upstream and generate the linuxized fix using ACPICA release
+      utilities (please refer to the Chapter 4 for the details).
+   3. Linux specific features - Sometime, it's impossible to use the
+      current ACPICA APIs to implement the features required by the Linux
+      kernel, the Linux developers have to hack ACPICA code directly.  Such
+      hacks may not be acceptable from ACPICA's point of view, then they
+      are left as committed ACPICA divergences unless ACPICA side can
+      implement new mechanisms as replacements to cleanup these hacks.
+   4. ACPICA release fixups - ACPICA only tests commits using a set of the
+      user space simulation utilies, thus the linuxized ACPICA patches may
+      break the Linux kernel, leaving us build/boot failures.  In order not
+      to break the Linux bisection process, fixes are done directly to the
+      linuxized ACPICA patches during the release process.  When the
+      release fixups are back ported to the ACPICA upstream, they must
+      follow the ACPICA upstream rules and thus risk further modifications.
+      These can all result in the new divergences.
+   5. Quick path of ACPICA commits - Some ACPICA commits are good stable
+      materials, they are upstreamed prior than the ACPICA release process.
+      If such commits are reverted or rebased in ACPICA side in order to
+      offer better solutions, new ACPICA divergences are generated.
+
+4. ACPICA Development
+
+   This paragraph guides the Linux developers to use the ACPICA upstream
+   release utilities to obtain the ACPICA upstream commits before they
+   are released by the ACPICA release process.
+
+   1. Cherry-pick an ACPICA commit:
+   First you need to git clone the ACPICA repository and the ACPICA changes
+   you want to cherry pick must be committed into the local repository.
+   Then the gen-patch.sh command can help to cherry-pick an ACPICA commit
+   from the ACPICA upstream.
+   # git clone https://github.com/acpica/acpica
+   # cd acpica
+   # generate/linux/gen-patch.sh -u [commit ID]
+   Here the commit ID is the ACPICA upstream commit ID you want to cherry
+   pick.  It can be omitted if the commit is "HEAD".
+
+   2. Cherry-pick the recent ACPICA commits:
+   Sometimes you need to rebase your code on top of the recent ACPICA
+   changes that haven't been upstreamed to Linux yet.  You can generate
+   the ACPICA release series on your own and rebase your code on top of
+   the generated ACPICA release patches.
+   # git clone https://github.com/acpica/acpica
+   # cd acpica
+   # generate/linux/make-patches.sh -u [commit ID]
+   The commit ID should be the last ACPICA commit accepted by Linux.
+   Normally it is a commit modifying ACPI_CA_VERSION.  The modification can
+   be achieved by executing "git log source/include/acpixf.h" in the local
+   copy of the ACPICA repository.
+
+   3. Confirm the current divergences:
+   If you have local copies of both Linux and ACPICA, you can generate a
+   diff file indicating the state of the current divergences:
+   # git clone https://github.com/acpica/acpica
+   # git clone http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
+   # cd acpica
+   # generate/linux/divergences.sh -s ../linux
-- 
1.7.10

[PATCH 2/5] ACPI / debugger: Fix regressions that AML debugger stops working

[Lv Zheng]

The FIFO unlocking mechanism in acpi_dbg has been messed up by the
following commit:
  Commit: 287980e49ffc0f6d911601e7e352a812ed27768e
  Subject: remove lots of IS_ERR_VALUE abuses
It converts !IS_ERR_VALUE(ret) into !ret. This patch fixes the
regression.

Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 drivers/acpi/acpi_dbg.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
index 1f41284..ebc8d18 100644
--- a/drivers/acpi/acpi_dbg.c
+++ b/drivers/acpi/acpi_dbg.c
@@ -602,7 +602,8 @@ static int acpi_aml_read_user(char __user *buf, int len)
 	crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
 	ret = n;
 out:
-	acpi_aml_unlock_fifo(ACPI_AML_OUT_USER, !ret);
+	acpi_aml_unlock_fifo(ACPI_AML_OUT_USER,
+			     ret < 0 ? false : true);
 	return ret;
 }
 
@@ -672,7 +673,8 @@ static int acpi_aml_write_user(const char __user *buf, int len)
 	crc->head = (crc->head + n) & (ACPI_AML_BUF_SIZE - 1);
 	ret = n;
 out:
-	acpi_aml_unlock_fifo(ACPI_AML_IN_USER, !ret);
+	acpi_aml_unlock_fifo(ACPI_AML_IN_USER,
+			     ret < 0 ? false : true);
 	return n;
 }
 
-- 
1.7.10

[PATCH 3/5] ACPI / debugger: Add AML debugger documentation

[Lv Zheng]

This patch adds AML debugger documentation.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 Documentation/acpi/aml-debugger.txt |   56 +++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/acpi/aml-debugger.txt

diff --git a/Documentation/acpi/aml-debugger.txt b/Documentation/acpi/aml-debugger.txt
new file mode 100644
index 0000000..0789332
--- /dev/null
+++ b/Documentation/acpi/aml-debugger.txt
@@ -0,0 +1,56 @@
+The AML Debugger
+
+Copyright (C) 2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+This document describes the usage of the AML debugger embedded in the Linux
+kernel.
+
+1. Build the debugger
+
+   The following kernel configuration items are required to enable the AML
+   debugger interface from the Linux kernel:
+   CONFIG_ACPI_DEBUGGER=y
+   CONFIG_ACPI_DEBUGGER_USER=m
+   The userspace utlities can be built from the kernel source tree using
+   the following commands:
+   # cd tools
+   # make acpi
+   The built userspace tool can be found at:
+     tools/acpi/power/acpi/acpidbg/acpidbg
+   You can install it to the system directories using the following
+   command:
+   # sudo make install
+
+2. Start the userspace debugger interface
+
+   After booting the kernel with the debugger built-in, developers can
+   start the debugger with the following commands:
+   # sudo mount -t debugfs none /sys/kernel/debug
+   # sudo modprobe acpi_dbg
+   # sudo tools/acpi/power/acpi/acpidbg/acpidbg
+   Now you have entered the interactive AML debugger environment, where
+   you can execute the debugger functionalities by typing the debugger
+   commands.
+   You can start to use it from typing "help" command and can download
+   <ACPICA Overview and Programmer Reference> from the following site:
+     https://acpica.org/documentation
+   And find detailed command reference in "Chapter 12. ACPICA Debugger
+   Reference".
+
+3. Stop the userspace debugger interface
+
+   You can type the Ctrl+C, quit, exit to end the interactive debugger
+   interface. And unload the module with the following commands:
+   # sudo rmmod acpi_dbg
+   The module unloading may fail if there is an acpidbg instance running.
+
+4. Run the debugger in a script
+
+   It will be very useful to have the ability to run the AML debugger in
+   a test script. "acpidbg" supports this in a special "batch" mode. For
+   example, the following command outputs the entire ACPI namespace:
+   # sudo acpidbg -b "namespace"
-- 
1.7.10

[PATCH 4/5] ACPI / button: Add SW_ACPI_LID for new usage model

[Lv Zheng]

There are many AML tables reporting wrong initial lid state, and some of
them never reports lid state. As a proxy layer acting between, ACPI button
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.

Link: https://lkml.org/2016/3/7/460
Link: https://github.com/systemd/systemd/issues/2087
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cc: linux-input@vger.kernel.org
---
 Documentation/acpi/acpi-lid.txt        |   42 ++++++++++++++++++++++++++++++++
 drivers/acpi/button.c                  |   20 ++++++++++-----
 include/linux/mod_devicetable.h        |    2 +-
 include/uapi/linux/input-event-codes.h |    3 ++-
 4 files changed, 59 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/acpi/acpi-lid.txt

diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
new file mode 100644
index 0000000..cba200d
--- /dev/null
+++ b/Documentation/acpi/acpi-lid.txt
@@ -0,0 +1,42 @@
+Restrictions of ACPI Control Method LID Device
+
+1. Expections of _LID control method's returning value
+
+The _LID control method is described to return the "current" lid state.
+However the word of "current" has ambiguity, many BIOSen return the lid
+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
+control method is evaluated during the runtime, the problem is its initial
+returning value. When the BIOSen implement this control method with cached
+value, the initial returning value is likely not reliable. There are simply
+so many examples always retuning "close" as initial lid state.
+
+2. Expections on lid events
+
+There are many BIOSen tables never notifying the lid open event. But it is
+ensured that there is always lid close events reported when the lid is
+closed. This is normally used to trigger system power saving operations on
+Windows, thus it is fully tested and functions correctly.
+
+3. Linux ACPI Control Method LID Device Users
+
+The userspace should stop relying on /proc/acpi/button/lid/LID0/state to
+obtain the lid state. This file is only used for the testing purpose.
+
+New userspace should rely on the lid close event to trigger power saving
+operations and may stop taking actions according to the lid open event. A
+new input event SW_ACPI_LID is prepared for the new userspace to implement
+the ACPI control method lid device specific logics.
+
+During the period the userspace hasn't been switched to use the new
+SW_ACPI_LID event, Linux users can use the following boot parameter to
+handle possible issues:
+  button.lid_init_state=method:
+   This is the default behavior, Linux kernel reports initial lid state
+   using _LID control method's returning value.
+   This may fixes some platforms if the _LID control method's returning
+   value is reliable.
+  button.lid_init_state=open:
+   Linux kernel always reports an initial lid state as "open".
+   This may fixes some platforms if the _LID control method's returning
+   value is not reliable.
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 148f4e5..4ef94d2 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device)
 	return lid_state ? 1 : 0;
 }
 
-static int acpi_lid_notify_state(struct acpi_device *device, int state)
+static int acpi_lid_notify_state(struct acpi_device *device,
+				 int state, bool notify_acpi)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 	int ret;
@@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
+	if (notify_acpi) {
+		input_report_switch(button->input,
+				    SW_ACPI_LID, !state);
+		input_sync(button->input);
+	}
 
 	if (state)
 		pm_wakeup_event(&device->dev, 0);
@@ -279,7 +285,8 @@ int acpi_lid_open(void)
 }
 EXPORT_SYMBOL(acpi_lid_open);
 
-static int acpi_lid_update_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device,
+				 bool notify_acpi)
 {
 	int state;
 
@@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device)
 	if (state < 0)
 		return state;
 
-	return acpi_lid_notify_state(device, state);
+	return acpi_lid_notify_state(device, state, notify_acpi);
 }
 
 static void acpi_lid_initialize_state(struct acpi_device *device)
 {
 	switch (lid_init_state) {
 	case ACPI_BUTTON_LID_INIT_OPEN:
-		(void)acpi_lid_notify_state(device, 1);
+		(void)acpi_lid_notify_state(device, 1, false);
 		break;
 	case ACPI_BUTTON_LID_INIT_METHOD:
-		(void)acpi_lid_update_state(device);
+		(void)acpi_lid_update_state(device, false);
 		break;
 	case ACPI_BUTTON_LID_INIT_IGNORE:
 	default:
@@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 	case ACPI_BUTTON_NOTIFY_STATUS:
 		input = button->input;
 		if (button->type == ACPI_BUTTON_TYPE_LID) {
-			acpi_lid_update_state(device);
+			acpi_lid_update_state(device, true);
 		} else {
 			int keycode;
 
@@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device)
 
 	case ACPI_BUTTON_TYPE_LID:
 		input_set_capability(input, EV_SW, SW_LID);
+		input_set_capability(input, EV_SW, SW_ACPI_LID);
 		break;
 	}
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 6e4c645..1014968 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -291,7 +291,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
-#define INPUT_DEVICE_ID_SW_MAX		0x0f
+#define INPUT_DEVICE_ID_SW_MAX		0x10
 
 #define INPUT_DEVICE_ID_MATCH_BUS	1
 #define INPUT_DEVICE_ID_MATCH_VENDOR	2
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 737fa32..81c344c 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -780,7 +780,8 @@
 #define SW_ROTATE_LOCK		0x0c  /* set = rotate locked/disabled */
 #define SW_LINEIN_INSERT	0x0d  /* set = inserted */
 #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
-#define SW_MAX			0x0f
+#define SW_ACPI_LID		0x0f  /* set = lid shut */
+#define SW_MAX			0x10
 #define SW_CNT			(SW_MAX+1)
 
 /*
-- 
1.7.10

[PATCH 5/5] ACPI: Add configuration item to configure ACPICA error logs out

[Lv Zheng]

Sometimes, we need to disable ACPICA error logs to leave only ACPICA
debug logs enabled for debugging purpose. This is useful when ACPICA error
logs become a flood.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=114201
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/Kconfig            |    7 +++++++
 include/acpi/platform/aclinux.h |    4 ++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 04af18f..939d235 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -324,6 +324,13 @@ config ACPI_TABLE_UPGRADE
 	  initrd, therefore it's safe to say Y.
 	  See Documentation/acpi/initrd_table_override.txt for details
 
+config ACPI_NO_ERROR_MESSAGES
+	bool "Disable error messages"
+	default n
+	help
+	  The ACPI subsystem can produce error messages. Saying Y disables
+	  this output.
+
 config ACPI_DEBUG
 	bool "Debug Statements"
 	default n
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 93b61b1..ed27b52 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -77,6 +77,10 @@
 #define ACPI_MUTEX_DEBUG
 #endif
 
+#ifdef CONFIG_ACPI_NO_ERROR_MESSAGES
+#define ACPI_NO_ERROR_MESSAGES
+#endif
+
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/ctype.h>
-- 
1.7.10

Re: [PATCH 2/5] ACPI / debugger: Fix regressions that AML debugger stops working

[Rafael J. Wysocki]

On Tue, Jul 5, 2016 at 1:18 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> The FIFO unlocking mechanism in acpi_dbg has been messed up by the
> following commit:
>   Commit: 287980e49ffc0f6d911601e7e352a812ed27768e
>   Subject: remove lots of IS_ERR_VALUE abuses
> It converts !IS_ERR_VALUE(ret) into !ret. This patch fixes the
> regression.
>
> Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Cc: Arnd Bergmann <arnd@arndb.de>

OK, but ->

> ---
>  drivers/acpi/acpi_dbg.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> index 1f41284..ebc8d18 100644
> --- a/drivers/acpi/acpi_dbg.c
> +++ b/drivers/acpi/acpi_dbg.c
> @@ -602,7 +602,8 @@ static int acpi_aml_read_user(char __user *buf, int len)
>         crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
>         ret = n;
>  out:
> -       acpi_aml_unlock_fifo(ACPI_AML_OUT_USER, !ret);
> +       acpi_aml_unlock_fifo(ACPI_AML_OUT_USER,
> +                            ret < 0 ? false : true);

-> The ternary operation is not necessary here, because the result of
(ret < 0) is already boolean.  So this can be written as

acpi_aml_unlock_fifo(ACPI_AML_OUT_USER, ret >= 0);

and analogously below.

I've made that change, please check the result in bleeding-edge.

Thanks,
Rafael

Re: [PATCH 5/5] ACPI: Add configuration item to configure ACPICA error logs out

[Rafael J. Wysocki]

On Tue, Jul 5, 2016 at 1:18 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> Sometimes, we need to disable ACPICA error logs to leave only ACPICA
> debug logs enabled for debugging purpose. This is useful when ACPICA error
> logs become a flood.
>
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=114201
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>

I seem to remember seeing this change once before and ISTR I said I
wouldn't apply it then.

Why would I say something different this time?

> ---
>  drivers/acpi/Kconfig            |    7 +++++++
>  include/acpi/platform/aclinux.h |    4 ++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 04af18f..939d235 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -324,6 +324,13 @@ config ACPI_TABLE_UPGRADE
>           initrd, therefore it's safe to say Y.
>           See Documentation/acpi/initrd_table_override.txt for details
>
> +config ACPI_NO_ERROR_MESSAGES
> +       bool "Disable error messages"
> +       default n
> +       help
> +         The ACPI subsystem can produce error messages. Saying Y disables
> +         this output.
> +
>  config ACPI_DEBUG
>         bool "Debug Statements"
>         default n
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index 93b61b1..ed27b52 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -77,6 +77,10 @@
>  #define ACPI_MUTEX_DEBUG
>  #endif
>
> +#ifdef CONFIG_ACPI_NO_ERROR_MESSAGES
> +#define ACPI_NO_ERROR_MESSAGES
> +#endif
> +
>  #include <linux/string.h>
>  #include <linux/kernel.h>
>  #include <linux/ctype.h>
> --

RE: [PATCH 5/5] ACPI: Add configuration item to configure ACPICA error logs out

[Zheng, Lv]

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Subject: Re: [PATCH 5/5] ACPI: Add configuration item to configure ACPICA
> error logs out
> 
> On Tue, Jul 5, 2016 at 1:18 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> > Sometimes, we need to disable ACPICA error logs to leave only ACPICA
> > debug logs enabled for debugging purpose. This is useful when ACPICA
> error
> > logs become a flood.
> >
> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=114201
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> I seem to remember seeing this change once before and ISTR I said I
> wouldn't apply it then.
> 
> Why would I say something different this time?
[Lv Zheng] 
I missed that.
Now that I deleted this patch from my local queue.

Thanks and best regards
-Lv

> 
> > ---
> >  drivers/acpi/Kconfig            |    7 +++++++
> >  include/acpi/platform/aclinux.h |    4 ++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 04af18f..939d235 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -324,6 +324,13 @@ config ACPI_TABLE_UPGRADE
> >           initrd, therefore it's safe to say Y.
> >           See Documentation/acpi/initrd_table_override.txt for details
> >
> > +config ACPI_NO_ERROR_MESSAGES
> > +       bool "Disable error messages"
> > +       default n
> > +       help
> > +         The ACPI subsystem can produce error messages. Saying Y disables
> > +         this output.
> > +
> >  config ACPI_DEBUG
> >         bool "Debug Statements"
> >         default n
> > diff --git a/include/acpi/platform/aclinux.h
> b/include/acpi/platform/aclinux.h
> > index 93b61b1..ed27b52 100644
> > --- a/include/acpi/platform/aclinux.h
> > +++ b/include/acpi/platform/aclinux.h
> > @@ -77,6 +77,10 @@
> >  #define ACPI_MUTEX_DEBUG
> >  #endif
> >
> > +#ifdef CONFIG_ACPI_NO_ERROR_MESSAGES
> > +#define ACPI_NO_ERROR_MESSAGES
> > +#endif
> > +
> >  #include <linux/string.h>
> >  #include <linux/kernel.h>
> >  #include <linux/ctype.h>
> > --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/5] ACPI / debugger: Fix regressions that AML debugger stops working

[Zheng, Lv]

Hi, Rafael

> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Subject: Re: [PATCH 2/5] ACPI / debugger: Fix regressions that AML
> debugger stops working
> 
> On Tue, Jul 5, 2016 at 1:18 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> > The FIFO unlocking mechanism in acpi_dbg has been messed up by the
> > following commit:
> >   Commit: 287980e49ffc0f6d911601e7e352a812ed27768e
> >   Subject: remove lots of IS_ERR_VALUE abuses
> > It converts !IS_ERR_VALUE(ret) into !ret. This patch fixes the
> > regression.
> >
> > Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> 
> OK, but ->
> 
> > ---
> >  drivers/acpi/acpi_dbg.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> > index 1f41284..ebc8d18 100644
> > --- a/drivers/acpi/acpi_dbg.c
> > +++ b/drivers/acpi/acpi_dbg.c
> > @@ -602,7 +602,8 @@ static int acpi_aml_read_user(char __user *buf,
> int len)
> >         crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
> >         ret = n;
> >  out:
> > -       acpi_aml_unlock_fifo(ACPI_AML_OUT_USER, !ret);
> > +       acpi_aml_unlock_fifo(ACPI_AML_OUT_USER,
> > +                            ret < 0 ? false : true);
> 
> -> The ternary operation is not necessary here, because the result of
> (ret < 0) is already boolean.  So this can be written as
> 
> acpi_aml_unlock_fifo(ACPI_AML_OUT_USER, ret >= 0);
> 
> and analogously below.
> 
> I've made that change, please check the result in bleeding-edge.

[Lv Zheng] 
Confirmed to be working.
Thanks for the simplification.

Thanks and best regards
-Lv

[PATCH v2 0/4] ACPI: ACPI documentation

[Lv Zheng]

This patch adds new ACPI documents related to:
1. ACPICA release process
2. AML debugger
3. ACPI control method lid device
This patch also contains an update of the ACPI LID input event in order to
support the new usage model defined by the documentation change.

History:
1. Remove the upstreamed trivial fixes;
2. Split lid device code and documents;
3. Fix grammar issues in the documents.

Lv Zheng (4):
  ACPI: Add documentation describing ACPICA release automation
  ACPI / debugger: Add AML debugger documentation
  ACPI / button: Add SW_ACPI_LID for new usage model
  ACPI / button: Add document for ACPI control method lid device
    restrictions

 Documentation/acpi/acpi-lid.txt         |   62 ++++++++
 Documentation/acpi/aml-debugger.txt     |   56 +++++++
 Documentation/acpi/linuxized-acpica.txt |  253 +++++++++++++++++++++++++++++++
 drivers/acpi/button.c                   |   20 ++-
 include/linux/mod_devicetable.h         |    2 +-
 include/uapi/linux/input-event-codes.h  |    3 +-
 6 files changed, 388 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/acpi/acpi-lid.txt
 create mode 100644 Documentation/acpi/aml-debugger.txt
 create mode 100644 Documentation/acpi/linuxized-acpica.txt

-- 
1.7.10

[PATCH v2 1/4] ACPI: Add documentation describing ACPICA release automation

[Lv Zheng]

This patch adds documentation on ACPICA release automation into the kernel
Documentation/acpi folder.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 Documentation/acpi/linuxized-acpica.txt |  253 +++++++++++++++++++++++++++++++
 1 file changed, 253 insertions(+)
 create mode 100644 Documentation/acpi/linuxized-acpica.txt

diff --git a/Documentation/acpi/linuxized-acpica.txt b/Documentation/acpi/linuxized-acpica.txt
new file mode 100644
index 0000000..74103c1
--- /dev/null
+++ b/Documentation/acpi/linuxized-acpica.txt
@@ -0,0 +1,253 @@
+Linuxized ACPICA - Introduction of ACPICA Release Automation
+
+Copyright (C) 2013-2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+This document describes the ACPICA project and the relationship between
+ACPICA and Linux.  It also includes the descriptions on how ACPICA code is
+automatically released to the following Linux subdirectories:
+drivers/acpi/acpica, include/acpi and tools/power/acpi.
+
+
+1. ACPICA Project
+
+   The ACPI Component Architecture (ACPICA) project provides an operating
+   system (OS)-independent reference implementation of the Advanced
+   Configuration and Power Interface Specification (ACPI).  It has been
+   adapted by various host OSes.  By directly integrating ACPICA, Linux can
+   also benefit from the application experiences of ACPICA from other host
+   OSes.
+
+   The homepage of ACPICA project is: www.acpica.org, it is maintained and
+   supported by Intel Corporation.
+
+   The following figure dipicts the Linux ACPI subystem where the ACPICA
+   adaption is included:
+
+      +---------------------------------------------------------+
+      |                                                         |
+      |   +---------------------------------------------------+ |
+      |   | +------------------+                              | |
+      |   | | Table Management |                              | |
+      |   | +------------------+                              | |
+      |   | +----------------------+                          | |
+      |   | | Namespace Management |                          | |
+      |   | +----------------------+                          | |
+      |   | +------------------+       ACPICA Components      | |
+      |   | | Event Management |                              | |
+      |   | +------------------+                              | |
+      |   | +---------------------+                           | |
+      |   | | Resource Management |                           | |
+      |   | +---------------------+                           | |
+      |   | +---------------------+                           | |
+      |   | | Hardware Management |                           | |
+      |   | +---------------------+                           | |
+      | +---------------------------------------------------+ | |
+      | | |                            +------------------+ | | |
+      | | |                            | OS Service Layer | | | |
+      | | |                            +------------------+ | | |
+      | | +-------------------------------------------------|-+ |
+      | |   +--------------------+                          |   |
+      | |   | Device Enumeration |                          |   |
+      | |   +--------------------+                          |   |
+      | |   +------------------+                            |   |
+      | |   | Power Management |                            |   |
+      | |   +------------------+     Linux/ACPI Components  |   |
+      | |   +--------------------+                          |   |
+      | |   | Thermal Management |                          |   |
+      | |   +--------------------+                          |   |
+      | |   +--------------------------+                    |   |
+      | |   | Drivers for ACPI Devices |                    |   |
+      | |   +--------------------------+                    |   |
+      | |   +--------+                                      |   |
+      | |   | ...... |                                      |   |
+      | |   +--------+                                      |   |
+      | +---------------------------------------------------+   |
+      |                                                         |
+      +---------------------------------------------------------+
+
+                 Figure 1. Linux ACPI Software Components
+
+   NOTE:
+    A. OS Service Layer - Provided by Linux to offer OS dependent
+       implementation of the predefined ACPICA interfaces (acpi_os_*).
+         include/acpi/acpiosxf.h
+         drivers/acpi/osl.c
+         include/acpi/platform
+         include/asm/acenv.h
+    B. ACPICA Functionalities - Released from ACPICA code base to offer
+       OS independent implementation of the ACPICA interfaces (acpi_*).
+         drivers/acpi/acpica
+         include/acpi/ac*.h
+         tools/power/acpi
+    C. Linux/ACPI Functionalities - Providing Linux specific ACPI
+       functionalities to the other Linux kernel subsystems and the user
+       space programs.
+         drivers/acpi
+         include/linux/acpi.h
+         include/linux/acpi*.h
+         include/acpi
+         tools/power/acpi
+    D. Architecture Specific ACPICA/ACPI Functionalities - Provided by the
+       ACPI subsystem to offer architecture specific implementation of the
+       ACPI interfaces.  They are Linux specific components, and is out of
+       the scope of this document.
+         include/asm/acpi.h
+         include/asm/acpi*.h
+         arch/*/acpi
+
+2. ACPICA Release
+
+   ACPICA project maintains a code base at the following repository URL:
+   https://github.com/acpica/acpica.git.  It is released once a month.
+
+   As the coding style adopted by the ACPICA project is not acceptable by
+   Linux, there is a release process to convert the ACPICA GIT commits into
+   the Linux acceptable patches.  The patches generated by this process are
+   known as "Linuxized ACPICA Patches".  The release process is performed
+   against a local copy of the ACPICA git repository.  Each commit in the
+   monthly release is converted into the linuxized ACPICA patch.  They form
+   a montly ACPICA release patchset for Linux ACPI community.  The
+   following figure dipicts the Linux upstream process of the ACPICA
+   commits:
+
+    +-----------------------------+
+    | acpica / master (-) commits |
+    +-----------------------------+
+       /|\         |
+        |         \|/
+        |  /---------------------\    +----------------------+
+        | < Linuxize repo Utility >-->| old linuxized acpica |--+
+        |  \---------------------/    +----------------------+  |
+        |                                                       |
+     /---------\                                                |
+    < git reset >                                                \
+     \---------/                                                  \
+       /|\                                                        /+-+
+        |                                                        /   |
+    +-----------------------------+                             |    |
+    | acpica / master (+) commits |                             |    |
+    +-----------------------------+                             |    |
+                   |                                            |    |
+                  \|/                                           |    |
+         /-----------------------\    +----------------------+  |    |
+        < Linuxize repo Utilities >-->| new linuxized acpica |--+    |
+         \-----------------------/    +----------------------+       |
+                                                                    \|/
+    +--------------------------+                  /----------------------\
+    | Linuxized ACPICA Patches |<----------------< Linuxize patch Utility >
+    +--------------------------+                  \----------------------/
+                   |
+                  \|/
+     /---------------------------\
+    < Linux ACPI Community Review >
+     \---------------------------/
+                   |
+                  \|/
+    +-----------------------+   /------------------\    +----------------+
+    | linux-pm / linux-next |--< Linux Merge Window >-->| linux / master |
+    +-----------------------+   \------------------/    +----------------+
+
+                Figure 2. ACPICA -> Linux Upstream Process
+
+   NOTE:
+    A. Linuxize Utilities - Provided by the ACPICA repository, including a
+       utility located in source/tools/acpisrc folder and a bunch of
+       scripts located in generate/linux folder.
+    B. acpica / master - "master" branch of the git repository at
+       <https://github.com/acpica/acpica.git>.
+    C. linux-pm / linux-next - "linux-next" branch of the git repository at
+       <http://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git>.
+    D. linux / master - "master" branch of the git repository at
+       <http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git>.
+    E. Linux ACPI Community - Reached at
+       <mailto:linux-acpi@vger.kernel.org>.
+
+   Before the linuxized ACPICA patches are sent to the Linux ACPI community
+   for review, there is a quality ensurance build test process performed to
+   reduce porting issues.  Currently this build process only cares about
+   the following kernel configurations:
+   CONFIG_ACPI/CONFIG_ACPI_DEBUG/CONFIG_ACPI_DEBUGGER
+
+3. ACPICA Divergence
+
+   It's ideal that if all of the ACPICA commits are upstreamed
+   automatically without modifications, the "linux / master" tree should
+   contain the ACPICA code that is exactly same as the ACPICA code
+   contained in "new linuxized acpica" tree and the release process thus
+   could be performed automatically.
+
+   But the facts are there are always source code differences between
+   Linux and ACPICA, such differences are known as "ACPICA Divergences".
+
+   The various sources of the ACPICA divergences include:
+   1. The existing divergences - Before the ACPICA release cycle is
+      fully running, there has already been divergences between Linux and
+      ACPICA. Over the past several years, the divergences are greatly
+      reduced, but there are still several ones and it takes time to figure
+      out the root cause of these old divergences.
+   2. Manual modifications - Any manual modification (ex., coding style
+      fixes) done directly in the Linux side obviously hurts the ACPICA
+      release automation.  Thus it is recommended to fix such issues in the
+      ACPICA upstream and generate the linuxized fix using the ACPICA
+      release utilities (please refer to the Chapter 4 for the details).
+   3. Linux specific features - Sometime, it's impossible to use the
+      current ACPICA APIs to implement the features required by the Linux
+      kernel, the Linux developers have to hack ACPICA code directly.  Such
+      hacks may not be acceptable from ACPICA's point of view, then they
+      are left as committed ACPICA divergences unless ACPICA side can
+      implement new mechanisms as replacements to clean these hacks up.
+   4. ACPICA release fixups - ACPICA only tests commits using a set of the
+      user space simulation utilies, thus the linuxized ACPICA patches may
+      break the Linux kernel, leaving us build/boot failures.  In order not
+      to break the Linux bisection process, fixes are done directly to the
+      linuxized ACPICA patches during the release process.  When the
+      release fixups are back ported to the ACPICA upstream, they must
+      follow the ACPICA upstream rules and thus risk further modifications.
+      These can all result in the new divergences.
+   5. Quick path of the ACPICA commits - Some ACPICA commits are good
+      stable materials, they are upstreamed prior than the ACPICA release
+      process.  If such commits are reverted or rebased in the ACPICA side
+      in order to offer better solutions, new ACPICA divergences are
+      generated.
+
+4. ACPICA Development
+
+   This paragraph guides the Linux developers to use the ACPICA upstream
+   release utilities to obtain the ACPICA upstream commits before they
+   are released by the ACPICA release process.
+
+   1. Cherry-pick an ACPICA commit:
+   First you need to git clone the ACPICA repository and the ACPICA changes
+   you want to cherry pick must be committed into the local repository.
+   Then the gen-patch.sh command can help to cherry-pick an ACPICA commit
+   from the ACPICA local repository.
+   # git clone https://github.com/acpica/acpica
+   # cd acpica
+   # generate/linux/gen-patch.sh -u [commit ID]
+   Here the commit ID is the ACPICA local repository commit ID you want to
+   cherry pick.  It can be omitted if the commit is "HEAD".
+
+   2. Cherry-pick the recent ACPICA commits:
+   Sometimes you need to rebase your code on top of the recent ACPICA
+   changes that haven't been upstreamed to Linux yet.  You can generate
+   the ACPICA release series on your own and rebase your code on top of
+   the generated ACPICA release patches.
+   # git clone https://github.com/acpica/acpica
+   # cd acpica
+   # generate/linux/make-patches.sh -u [commit ID]
+   The commit ID should be the last ACPICA commit accepted by Linux.
+   Normally it is a commit modifying ACPI_CA_VERSION.  The modification can
+   be achieved by executing "git blame source/include/acpixf.h" and
+   referencing line that contains "ACPI_CA_VERSION".
+
+   3. Confirm the current divergences:
+   If you have local copies of both Linux and ACPICA, you can generate a
+   diff file indicating the state of the current divergences:
+   # git clone https://github.com/acpica/acpica
+   # git clone http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
+   # cd acpica
+   # generate/linux/divergences.sh -s ../linux
-- 
1.7.10

[PATCH v2 2/4] ACPI / debugger: Add AML debugger documentation

[Lv Zheng]

This patch adds AML debugger documentation.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 Documentation/acpi/aml-debugger.txt |   56 +++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/acpi/aml-debugger.txt

diff --git a/Documentation/acpi/aml-debugger.txt b/Documentation/acpi/aml-debugger.txt
new file mode 100644
index 0000000..2f1c184
--- /dev/null
+++ b/Documentation/acpi/aml-debugger.txt
@@ -0,0 +1,56 @@
+The AML Debugger
+
+Copyright (C) 2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+This document describes the usage of the AML debugger embedded in the Linux
+kernel.
+
+1. Build the debugger
+
+   The following kernel configuration items are required to enable the AML
+   debugger interface from the Linux kernel:
+   CONFIG_ACPI_DEBUGGER=y
+   CONFIG_ACPI_DEBUGGER_USER=m
+   The userspace utlities can be built from the kernel source tree using
+   the following commands:
+   # cd tools
+   # make acpi
+   The built userspace tool can be found at:
+     tools/acpi/power/acpi/acpidbg/acpidbg
+   You can install it to the system directories using the following
+   command:
+   # make install
+
+2. Start the userspace debugger interface
+
+   After booting the kernel with the debugger built-in, developers can
+   start the debugger with the following commands:
+   # mount -t debugfs none /sys/kernel/debug
+   # modprobe acpi_dbg
+   # tools/acpi/power/acpi/acpidbg/acpidbg
+   Now you have entered the interactive AML debugger environment, where
+   you can execute the debugger functionalities by typing the debugger
+   commands.
+   You can start to use it from typing "help" command and can download
+   <ACPICA Overview and Programmer Reference> from the following site:
+     https://acpica.org/documentation
+   And find detailed command reference in "Chapter 12. ACPICA Debugger
+   Reference".
+
+3. Stop the userspace debugger interface
+
+   You can type the Ctrl+C, quit, exit to end the interactive debugger
+   interface. And unload the module with the following commands:
+   # rmmod acpi_dbg
+   The module unloading may fail if there is an acpidbg instance running.
+
+4. Run the debugger in a script
+
+   It will be very useful to have the ability to run the AML debugger in
+   a test script. "acpidbg" supports this in a special "batch" mode. For
+   example, the following command outputs the entire ACPI namespace:
+   # acpidbg -b "namespace"
-- 
1.7.10

[PATCH v2 3/4] ACPI / button: Add SW_ACPI_LID for new usage model

[Lv Zheng]

There are many AML tables reporting wrong initial lid state, and some of
them never reports lid state. As a proxy layer acting between, ACPI button
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 a new input key event so that new userspace programs can
use it to handle this usage model correctly. And in the meanwhile, no old
programs will be broken by the userspace changes.

Link: https://lkml.org/2016/3/7/460
Link: https://github.com/systemd/systemd/issues/2087
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cc: linux-input@vger.kernel.org
---
 drivers/acpi/button.c                  |   20 ++++++++++++++------
 include/linux/mod_devicetable.h        |    2 +-
 include/uapi/linux/input-event-codes.h |    3 ++-
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 148f4e5..4ef94d2 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device)
 	return lid_state ? 1 : 0;
 }
 
-static int acpi_lid_notify_state(struct acpi_device *device, int state)
+static int acpi_lid_notify_state(struct acpi_device *device,
+				 int state, bool notify_acpi)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 	int ret;
@@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
+	if (notify_acpi) {
+		input_report_switch(button->input,
+				    SW_ACPI_LID, !state);
+		input_sync(button->input);
+	}
 
 	if (state)
 		pm_wakeup_event(&device->dev, 0);
@@ -279,7 +285,8 @@ int acpi_lid_open(void)
 }
 EXPORT_SYMBOL(acpi_lid_open);
 
-static int acpi_lid_update_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device,
+				 bool notify_acpi)
 {
 	int state;
 
@@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device)
 	if (state < 0)
 		return state;
 
-	return acpi_lid_notify_state(device, state);
+	return acpi_lid_notify_state(device, state, notify_acpi);
 }
 
 static void acpi_lid_initialize_state(struct acpi_device *device)
 {
 	switch (lid_init_state) {
 	case ACPI_BUTTON_LID_INIT_OPEN:
-		(void)acpi_lid_notify_state(device, 1);
+		(void)acpi_lid_notify_state(device, 1, false);
 		break;
 	case ACPI_BUTTON_LID_INIT_METHOD:
-		(void)acpi_lid_update_state(device);
+		(void)acpi_lid_update_state(device, false);
 		break;
 	case ACPI_BUTTON_LID_INIT_IGNORE:
 	default:
@@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 	case ACPI_BUTTON_NOTIFY_STATUS:
 		input = button->input;
 		if (button->type == ACPI_BUTTON_TYPE_LID) {
-			acpi_lid_update_state(device);
+			acpi_lid_update_state(device, true);
 		} else {
 			int keycode;
 
@@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device)
 
 	case ACPI_BUTTON_TYPE_LID:
 		input_set_capability(input, EV_SW, SW_LID);
+		input_set_capability(input, EV_SW, SW_ACPI_LID);
 		break;
 	}
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 6e4c645..1014968 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -291,7 +291,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
-#define INPUT_DEVICE_ID_SW_MAX		0x0f
+#define INPUT_DEVICE_ID_SW_MAX		0x10
 
 #define INPUT_DEVICE_ID_MATCH_BUS	1
 #define INPUT_DEVICE_ID_MATCH_VENDOR	2
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 737fa32..81c344c 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -780,7 +780,8 @@
 #define SW_ROTATE_LOCK		0x0c  /* set = rotate locked/disabled */
 #define SW_LINEIN_INSERT	0x0d  /* set = inserted */
 #define SW_MUTE_DEVICE		0x0e  /* set = device disabled */
-#define SW_MAX			0x0f
+#define SW_ACPI_LID		0x0f  /* set = lid shut */
+#define SW_MAX			0x10
 #define SW_CNT			(SW_MAX+1)
 
 /*
-- 
1.7.10

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

[Lv Zheng]

There are many AML tables reporting wrong initial lid state, and some of
them never reports lid state. As a proxy layer acting between, ACPI button
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 <lv.zheng@intel.com>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cc: linux-input@vger.kernel.org
---
 Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
 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-lid.txt
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 a
+control method lid device. To implement this, the AML tables issue
+Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
+changed. The _LID control method for the lid device must be implemented to
+report the "current" state of the lid as either "opened" or "closed".
+
+This document describes the restrictions and the expections of the Linux
+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.
+However the word of "current" has ambiguity, many AML tables return the lid
+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
+control method is evaluated during the runtime, the problem is its initial
+returning value. When the AML tables implement this control method with
+cached value, the initial returning value is likely not reliable. There are
+simply so many examples always retuning "closed" as initial lid state.
+
+2. Restrictions of the lid state change notifications
+
+There are many AML tables never notifying when the lid device state is
+changed to "opened". But it is ensured that the AML tables always notify
+"closed" when the lid state is changed to "closed". This is normally used
+to trigger some system power saving operations on Windows. Since it is
+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
+used for the validation purpose.
+
+New userspace programs should rely on the lid "closed" notification to
+trigger some power saving operations and may stop taking actions according
+to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
+prepared for the new userspace to implement this ACPI control method lid
+device specific logics.
+
+During the period the userspace hasn't been switched to use the new
+SW_ACPI_LID event, Linux users can use the following boot parameter to
+handle possible issues:
+  button.lid_init_state=method:
+   This is the default behavior of the Linux ACPI lid driver, Linux kernel
+   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
+   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
+   method is not reliable.
-- 
1.7.10

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

[Benjamin Tissoires]

Hi,

On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> There are many AML tables reporting wrong initial lid state, and some of
> them never reports lid state. As a proxy layer acting between, ACPI button
> 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 <lv.zheng@intel.com>
> Cc: Bastien Nocera: <hadess@hadess.net>
> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Cc: linux-input@vger.kernel.org
> ---
>  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
>  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-lid.txt
> 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 a
> +control method lid device. To implement this, the AML tables issue
> +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> +changed. The _LID control method for the lid device must be implemented to
> +report the "current" state of the lid as either "opened" or "closed".
> +
> +This document describes the restrictions and the expections of the Linux
> +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.
> +However the word of "current" has ambiguity, many AML tables return the lid
> +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
> +control method is evaluated during the runtime, the problem is its initial
> +returning value. When the AML tables implement this control method with
> +cached value, the initial returning value is likely not reliable. There are
> +simply so many examples always retuning "closed" as initial lid state.
> +
> +2. Restrictions of the lid state change notifications
> +
> +There are many AML tables never notifying when the lid device state is
> +changed to "opened". But it is ensured that the AML tables always notify
> +"closed" when the lid state is changed to "closed". This is normally used
> +to trigger some system power saving operations on Windows. Since it is
> +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
> +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.

> +
> +New userspace programs should rely on the lid "closed" notification to
> +trigger some power saving operations and may stop taking actions according
> +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
> +prepared for the new userspace to implement this ACPI control method lid
> +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
how to handle.

> +
> +During the period the userspace hasn't been switched to use the new
> +SW_ACPI_LID event, Linux users can use the following boot parameter to
> +handle possible issues:
> +  button.lid_init_state=method:
> +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> +   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
> +   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
> +   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.

Cheers,
Benjamin

> --
> 1.7.10
>

Re: [PATCH v2 3/4] ACPI / button: Add SW_ACPI_LID for new usage model

[Benjamin Tissoires]

On Thu, Jul 7, 2016 at 9:10 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> There are many AML tables reporting wrong initial lid state, and some of
> them never reports lid state. As a proxy layer acting between, ACPI button
> 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 a new input key event so that new userspace programs can
> use it to handle this usage model correctly. And in the meanwhile, no old
> programs will be broken by the userspace changes.
>
> Link: https://lkml.org/2016/3/7/460
> Link: https://github.com/systemd/systemd/issues/2087
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Cc: Bastien Nocera: <hadess@hadess.net>
> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Cc: linux-input@vger.kernel.org
> ---
>  drivers/acpi/button.c                  |   20 ++++++++++++++------
>  include/linux/mod_devicetable.h        |    2 +-
>  include/uapi/linux/input-event-codes.h |    3 ++-
>  3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 148f4e5..4ef94d2 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device)
>         return lid_state ? 1 : 0;
>  }
>
> -static int acpi_lid_notify_state(struct acpi_device *device, int state)
> +static int acpi_lid_notify_state(struct acpi_device *device,
> +                                int state, bool notify_acpi)
>  {
>         struct acpi_button *button = acpi_driver_data(device);
>         int ret;
> @@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
>         /* input layer checks if event is redundant */
>         input_report_switch(button->input, SW_LID, !state);
>         input_sync(button->input);
> +       if (notify_acpi) {
> +               input_report_switch(button->input,
> +                                   SW_ACPI_LID, !state);
> +               input_sync(button->input);

If you use a switch, you'll never send subsequent open state if you
doesn't close it yourself.
See my comments in 5/5 and please use a KEY event instead.

> +       }
>
>         if (state)
>                 pm_wakeup_event(&device->dev, 0);
> @@ -279,7 +285,8 @@ int acpi_lid_open(void)
>  }
>  EXPORT_SYMBOL(acpi_lid_open);
>
> -static int acpi_lid_update_state(struct acpi_device *device)
> +static int acpi_lid_update_state(struct acpi_device *device,
> +                                bool notify_acpi)
>  {
>         int state;
>
> @@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device)
>         if (state < 0)
>                 return state;
>
> -       return acpi_lid_notify_state(device, state);
> +       return acpi_lid_notify_state(device, state, notify_acpi);
>  }
>
>  static void acpi_lid_initialize_state(struct acpi_device *device)
>  {
>         switch (lid_init_state) {
>         case ACPI_BUTTON_LID_INIT_OPEN:
> -               (void)acpi_lid_notify_state(device, 1);
> +               (void)acpi_lid_notify_state(device, 1, false);
>                 break;
>         case ACPI_BUTTON_LID_INIT_METHOD:
> -               (void)acpi_lid_update_state(device);
> +               (void)acpi_lid_update_state(device, false);
>                 break;
>         case ACPI_BUTTON_LID_INIT_IGNORE:
>         default:
> @@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>         case ACPI_BUTTON_NOTIFY_STATUS:
>                 input = button->input;
>                 if (button->type == ACPI_BUTTON_TYPE_LID) {
> -                       acpi_lid_update_state(device);
> +                       acpi_lid_update_state(device, true);
>                 } else {
>                         int keycode;
>
> @@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device)
>
>         case ACPI_BUTTON_TYPE_LID:
>                 input_set_capability(input, EV_SW, SW_LID);
> +               input_set_capability(input, EV_SW, SW_ACPI_LID);

Can't we export this new event only if the _LID function is not
reliable? This could check for the module parameter lid_init_state and
only enable it for ACPI_BUTTON_LID_INIT_OPEN.

I really hope we will be able to find a reliable way to determine
whether or not the platform support reliable LID state. If not, there
might be a need to have a db of reliable switch platforms. This can be
set in the kernel or with a hwdb entry in userspace.

Cheers,
Benjamin

>                 break;
>         }
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 6e4c645..1014968 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -291,7 +291,7 @@ struct pcmcia_device_id {
>  #define INPUT_DEVICE_ID_LED_MAX                0x0f
>  #define INPUT_DEVICE_ID_SND_MAX                0x07
>  #define INPUT_DEVICE_ID_FF_MAX         0x7f
> -#define INPUT_DEVICE_ID_SW_MAX         0x0f
> +#define INPUT_DEVICE_ID_SW_MAX         0x10
>
>  #define INPUT_DEVICE_ID_MATCH_BUS      1
>  #define INPUT_DEVICE_ID_MATCH_VENDOR   2
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 737fa32..81c344c 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -780,7 +780,8 @@
>  #define SW_ROTATE_LOCK         0x0c  /* set = rotate locked/disabled */
>  #define SW_LINEIN_INSERT       0x0d  /* set = inserted */
>  #define SW_MUTE_DEVICE         0x0e  /* set = device disabled */
> -#define SW_MAX                 0x0f
> +#define SW_ACPI_LID            0x0f  /* set = lid shut */
> +#define SW_MAX                 0x10
>  #define SW_CNT                 (SW_MAX+1)
>
>  /*
> --
> 1.7.10
>

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

[Dmitry Torokhov]

On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> Hi,
> 
> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > There are many AML tables reporting wrong initial lid state, and some of
> > them never reports lid state. As a proxy layer acting between, ACPI button
> > 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 <lv.zheng@intel.com>
> > Cc: Bastien Nocera: <hadess@hadess.net>
> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
> >  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-lid.txt
> > 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 a
> > +control method lid device. To implement this, the AML tables issue
> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> > +changed. The _LID control method for the lid device must be implemented to
> > +report the "current" state of the lid as either "opened" or "closed".
> > +
> > +This document describes the restrictions and the expections of the Linux
> > +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.
> > +However the word of "current" has ambiguity, many AML tables return the lid
> > +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
> > +control method is evaluated during the runtime, the problem is its initial
> > +returning value. When the AML tables implement this control method with
> > +cached value, the initial returning value is likely not reliable. There are
> > +simply so many examples always retuning "closed" as initial lid state.
> > +
> > +2. Restrictions of the lid state change notifications
> > +
> > +There are many AML tables never notifying when the lid device state is
> > +changed to "opened". But it is ensured that the AML tables always notify
> > +"closed" when the lid state is changed to "closed". This is normally used
> > +to trigger some system power saving operations on Windows. Since it is
> > +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
> > +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.
> 
> > +
> > +New userspace programs should rely on the lid "closed" notification to
> > +trigger some power saving operations and may stop taking actions according
> > +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
> > +prepared for the new userspace to implement this ACPI control method lid
> > +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
> how to handle.
> 
> > +
> > +During the period the userspace hasn't been switched to use the new
> > +SW_ACPI_LID event, Linux users can use the following boot parameter to
> > +handle possible issues:
> > +  button.lid_init_state=method:
> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> > +   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
> > +   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
> > +   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.

How about we leave the kernel alone and userspace (which would have to
cope with the new KEY_LID_OPEN anyway) would just have to know that if
switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then it
can't trust the events and it needs additional heuristics.

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/4] ACPI / button: Add SW_ACPI_LID for new usage model

[Dmitry Torokhov]

On Fri, Jul 08, 2016 at 11:27:23AM +0200, Benjamin Tissoires wrote:
> On Thu, Jul 7, 2016 at 9:10 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > There are many AML tables reporting wrong initial lid state, and some of
> > them never reports lid state. As a proxy layer acting between, ACPI button
> > 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 a new input key event so that new userspace programs can
> > use it to handle this usage model correctly. And in the meanwhile, no old
> > programs will be broken by the userspace changes.
> >
> > Link: https://lkml.org/2016/3/7/460

Does not work for me.

> > Link: https://github.com/systemd/systemd/issues/2087

Gives me info about pull "Basic DNSSEC support, and unrelated fixes"

> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Cc: Bastien Nocera: <hadess@hadess.net>
> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  drivers/acpi/button.c                  |   20 ++++++++++++++------
> >  include/linux/mod_devicetable.h        |    2 +-
> >  include/uapi/linux/input-event-codes.h |    3 ++-
> >  3 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 148f4e5..4ef94d2 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -130,7 +130,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device)
> >         return lid_state ? 1 : 0;
> >  }
> >
> > -static int acpi_lid_notify_state(struct acpi_device *device, int state)
> > +static int acpi_lid_notify_state(struct acpi_device *device,
> > +                                int state, bool notify_acpi)
> >  {
> >         struct acpi_button *button = acpi_driver_data(device);
> >         int ret;
> > @@ -138,6 +139,11 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
> >         /* input layer checks if event is redundant */
> >         input_report_switch(button->input, SW_LID, !state);
> >         input_sync(button->input);
> > +       if (notify_acpi) {
> > +               input_report_switch(button->input,
> > +                                   SW_ACPI_LID, !state);
> > +               input_sync(button->input);
> 
> If you use a switch, you'll never send subsequent open state if you
> doesn't close it yourself.
> See my comments in 5/5 and please use a KEY event instead.
> 
> > +       }
> >
> >         if (state)
> >                 pm_wakeup_event(&device->dev, 0);
> > @@ -279,7 +285,8 @@ int acpi_lid_open(void)
> >  }
> >  EXPORT_SYMBOL(acpi_lid_open);
> >
> > -static int acpi_lid_update_state(struct acpi_device *device)
> > +static int acpi_lid_update_state(struct acpi_device *device,
> > +                                bool notify_acpi)
> >  {
> >         int state;
> >
> > @@ -287,17 +294,17 @@ static int acpi_lid_update_state(struct acpi_device *device)
> >         if (state < 0)
> >                 return state;
> >
> > -       return acpi_lid_notify_state(device, state);
> > +       return acpi_lid_notify_state(device, state, notify_acpi);
> >  }
> >
> >  static void acpi_lid_initialize_state(struct acpi_device *device)
> >  {
> >         switch (lid_init_state) {
> >         case ACPI_BUTTON_LID_INIT_OPEN:
> > -               (void)acpi_lid_notify_state(device, 1);
> > +               (void)acpi_lid_notify_state(device, 1, false);
> >                 break;
> >         case ACPI_BUTTON_LID_INIT_METHOD:
> > -               (void)acpi_lid_update_state(device);
> > +               (void)acpi_lid_update_state(device, false);
> >                 break;
> >         case ACPI_BUTTON_LID_INIT_IGNORE:
> >         default:
> > @@ -317,7 +324,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
> >         case ACPI_BUTTON_NOTIFY_STATUS:
> >                 input = button->input;
> >                 if (button->type == ACPI_BUTTON_TYPE_LID) {
> > -                       acpi_lid_update_state(device);
> > +                       acpi_lid_update_state(device, true);
> >                 } else {
> >                         int keycode;
> >
> > @@ -436,6 +443,7 @@ static int acpi_button_add(struct acpi_device *device)
> >
> >         case ACPI_BUTTON_TYPE_LID:
> >                 input_set_capability(input, EV_SW, SW_LID);
> > +               input_set_capability(input, EV_SW, SW_ACPI_LID);
> 
> Can't we export this new event only if the _LID function is not
> reliable? This could check for the module parameter lid_init_state and
> only enable it for ACPI_BUTTON_LID_INIT_OPEN.
> 
> I really hope we will be able to find a reliable way to determine
> whether or not the platform support reliable LID state. If not, there
> might be a need to have a db of reliable switch platforms. This can be
> set in the kernel or with a hwdb entry in userspace.
> 
> Cheers,
> Benjamin
> 
> >                 break;
> >         }
> >
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 6e4c645..1014968 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -291,7 +291,7 @@ struct pcmcia_device_id {
> >  #define INPUT_DEVICE_ID_LED_MAX                0x0f
> >  #define INPUT_DEVICE_ID_SND_MAX                0x07
> >  #define INPUT_DEVICE_ID_FF_MAX         0x7f
> > -#define INPUT_DEVICE_ID_SW_MAX         0x0f
> > +#define INPUT_DEVICE_ID_SW_MAX         0x10
> >
> >  #define INPUT_DEVICE_ID_MATCH_BUS      1
> >  #define INPUT_DEVICE_ID_MATCH_VENDOR   2
> > diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> > index 737fa32..81c344c 100644
> > --- a/include/uapi/linux/input-event-codes.h
> > +++ b/include/uapi/linux/input-event-codes.h
> > @@ -780,7 +780,8 @@
> >  #define SW_ROTATE_LOCK         0x0c  /* set = rotate locked/disabled */
> >  #define SW_LINEIN_INSERT       0x0d  /* set = inserted */
> >  #define SW_MUTE_DEVICE         0x0e  /* set = device disabled */
> > -#define SW_MAX                 0x0f
> > +#define SW_ACPI_LID            0x0f  /* set = lid shut */

0x0f is busy already.

> > +#define SW_MAX                 0x10
> >  #define SW_CNT                 (SW_MAX+1)
> >
> >  /*
> > --
> > 1.7.10
> >

-- 
Dmitry

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

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> Hi,
> 
> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > There are many AML tables reporting wrong initial lid state, and some of
> > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > 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 <lv.zheng@intel.com>
> > Cc: Bastien Nocera: <hadess@hadess.net>
> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> >  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-
> lid.txt
> > 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
> a
> > +control method lid device. To implement this, the AML tables issue
> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> > +changed. The _LID control method for the lid device must be
> implemented to
> > +report the "current" state of the lid as either "opened" or "closed".
> > +
> > +This document describes the restrictions and the expections of the
> Linux
> > +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.
> > +However the word of "current" has ambiguity, many AML tables return
> the lid
> > +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
> > +control method is evaluated during the runtime, the problem is its
> initial
> > +returning value. When the AML tables implement this control method
> with
> > +cached value, the initial returning value is likely not reliable. There are
> > +simply so many examples always retuning "closed" as initial lid state.
> > +
> > +2. Restrictions of the lid state change notifications
> > +
> > +There are many AML tables never notifying when the lid device state is
> > +changed to "opened". But it is ensured that the AML tables always
> notify
> > +"closed" when the lid state is changed to "closed". This is normally used
> > +to trigger some system power saving operations on Windows. Since it is
> > +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
> > +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.

> 
> > +
> > +New userspace programs should rely on the lid "closed" notification to
> > +trigger some power saving operations and may stop taking actions
> according
> > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > +prepared for the new userspace to implement this ACPI control method
> lid
> > +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
> how to handle.
[Lv Zheng] 
It should be KEY_LID_CLOSE.
However I checked the KEY code definitions.
It seems their values are highly dependent on the HID specification.
I'm not sure which key code value should I use for this.
Could you point me out?

> 
> > +
> > +During the period the userspace hasn't been switched to use the new
> > +SW_ACPI_LID event, Linux users can use the following boot parameter
> to
> > +handle possible issues:
> > +  button.lid_init_state=method:
> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> > +   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
> > +   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
> > +   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".

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.

Cheers,
-Lv

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

[Bastien Nocera]

On Mon, 2016-07-11 at 03:20 +0000, Zheng, Lv wrote:
> 
<snip>
> > 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".
> 
> 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.

For the affected devices? Sure, but I don't think that's a reasonable
thing to do for "all" the devices. We have a majority of laptops where
this isn't a problem, and it's not even a problem any more on one of
the devices that triggered this discussion (there's a patch for make
the LID status match reality for the Surface 3).

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

[Benjamin Tissoires]

On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
>> Hi,
>>
>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > There are many AML tables reporting wrong initial lid state, and some of
>> > them never reports lid state. As a proxy layer acting between, ACPI button
>> > 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 <lv.zheng@intel.com>
>> > Cc: Bastien Nocera: <hadess@hadess.net>
>> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> > Cc: linux-input@vger.kernel.org
>> > ---
>> >  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
>> >  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-lid.txt
>> > 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 a
>> > +control method lid device. To implement this, the AML tables issue
>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
>> > +changed. The _LID control method for the lid device must be implemented to
>> > +report the "current" state of the lid as either "opened" or "closed".
>> > +
>> > +This document describes the restrictions and the expections of the Linux
>> > +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.
>> > +However the word of "current" has ambiguity, many AML tables return the lid
>> > +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
>> > +control method is evaluated during the runtime, the problem is its initial
>> > +returning value. When the AML tables implement this control method with
>> > +cached value, the initial returning value is likely not reliable. There are
>> > +simply so many examples always retuning "closed" as initial lid state.
>> > +
>> > +2. Restrictions of the lid state change notifications
>> > +
>> > +There are many AML tables never notifying when the lid device state is
>> > +changed to "opened". But it is ensured that the AML tables always notify
>> > +"closed" when the lid state is changed to "closed". This is normally used
>> > +to trigger some system power saving operations on Windows. Since it is
>> > +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
>> > +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.
>>
>> > +
>> > +New userspace programs should rely on the lid "closed" notification to
>> > +trigger some power saving operations and may stop taking actions according
>> > +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
>> > +prepared for the new userspace to implement this ACPI control method lid
>> > +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
>> how to handle.
>>
>> > +
>> > +During the period the userspace hasn't been switched to use the new
>> > +SW_ACPI_LID event, Linux users can use the following boot parameter to
>> > +handle possible issues:
>> > +  button.lid_init_state=method:
>> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>> > +   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
>> > +   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
>> > +   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.
>
> How about we leave the kernel alone and userspace (which would have to
> cope with the new KEY_LID_OPEN anyway) would just have to know that if
> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then it
> can't trust the events and it needs additional heuristics.
>

I really wished we could leave the kernel alone, but some platform
need fixes: we are using an EV_SW, and on those platform, we only get
the close event, which means it gets ignored by the input layer.
Those platform have a form factor which makes the situation acceptable
(tablet with cover, or very low cost laptops).
However, switching away from a different EV_SW and tell userspace to
ignore it would break systems when you are using a docking station.
The acpi would provide fake values for the LID switch and this will
screw the session if you are working on a docking station with an
external monitor and the LID closed.

My initial suggestion was:
- detect in the kernel whether the ACPI LID information was judged as reliable
- if reliable keep things as it
- if not reliable add an extra KEY_LID_CLOSE to notify userspace that
the SW_LID is not reliable (it will emulate the LID open events) and
that it will only get true close events.

After further thoughts, I think we can simply add the extra key, and
have an hwdb entry in logind to enumerate the devices only relying on
the close event. If the userspace is not updated, we can tell user to
use the button.lid_init_state=open quirk to simulate the behavior
logind should expect.

That means only adding one extra key (and its events) in the kernel
and let userspace decide what to do.

Cheers,
Benjamin

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

[Benjamin Tissoires]

On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>> method lid device restrictions
>>
>> Hi,
>>
>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > There are many AML tables reporting wrong initial lid state, and some of
>> > them never reports lid state. As a proxy layer acting between, ACPI
>> button
>> > 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 <lv.zheng@intel.com>
>> > Cc: Bastien Nocera: <hadess@hadess.net>
>> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> > Cc: linux-input@vger.kernel.org
>> > ---
>> >  Documentation/acpi/acpi-lid.txt |   62
>> +++++++++++++++++++++++++++++++++++++++
>> >  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-
>> lid.txt
>> > 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
>> a
>> > +control method lid device. To implement this, the AML tables issue
>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
>> > +changed. The _LID control method for the lid device must be
>> implemented to
>> > +report the "current" state of the lid as either "opened" or "closed".
>> > +
>> > +This document describes the restrictions and the expections of the
>> Linux
>> > +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.
>> > +However the word of "current" has ambiguity, many AML tables return
>> the lid
>> > +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
>> > +control method is evaluated during the runtime, the problem is its
>> initial
>> > +returning value. When the AML tables implement this control method
>> with
>> > +cached value, the initial returning value is likely not reliable. There are
>> > +simply so many examples always retuning "closed" as initial lid state.
>> > +
>> > +2. Restrictions of the lid state change notifications
>> > +
>> > +There are many AML tables never notifying when the lid device state is
>> > +changed to "opened". But it is ensured that the AML tables always
>> notify
>> > +"closed" when the lid state is changed to "closed". This is normally used
>> > +to trigger some system power saving operations on Windows. Since it is
>> > +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
>> > +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.
>
>>
>> > +
>> > +New userspace programs should rely on the lid "closed" notification to
>> > +trigger some power saving operations and may stop taking actions
>> according
>> > +to the lid "opened" notification. A new input switch event -
>> SW_ACPI_LID is
>> > +prepared for the new userspace to implement this ACPI control method
>> lid
>> > +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
>> how to handle.
> [Lv Zheng]
> It should be KEY_LID_CLOSE.

yep, sorry.

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

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


>>
>> > +
>> > +During the period the userspace hasn't been switched to use the new
>> > +SW_ACPI_LID event, Linux users can use the following boot parameter
>> to
>> > +handle possible issues:
>> > +  button.lid_init_state=method:
>> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>> > +   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
>> > +   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
>> > +   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".
>
> 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.
>
> Cheers,
> -Lv

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

[Benjamin Tissoires]

[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
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> Hi,
>>
>>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
>>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>>> method lid device restrictions
>>>
>>> Hi,
>>>
>>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>>> > There are many AML tables reporting wrong initial lid state, and some of
>>> > them never reports lid state. As a proxy layer acting between, ACPI
>>> button
>>> > 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 <lv.zheng@intel.com>
>>> > Cc: Bastien Nocera: <hadess@hadess.net>
>>> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>>> > Cc: linux-input@vger.kernel.org
>>> > ---
>>> >  Documentation/acpi/acpi-lid.txt |   62
>>> +++++++++++++++++++++++++++++++++++++++
>>> >  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-
>>> lid.txt
>>> > 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
>>> a
>>> > +control method lid device. To implement this, the AML tables issue
>>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
>>> > +changed. The _LID control method for the lid device must be
>>> implemented to
>>> > +report the "current" state of the lid as either "opened" or "closed".
>>> > +
>>> > +This document describes the restrictions and the expections of the
>>> Linux
>>> > +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.
>>> > +However the word of "current" has ambiguity, many AML tables return
>>> the lid
>>> > +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
>>> > +control method is evaluated during the runtime, the problem is its
>>> initial
>>> > +returning value. When the AML tables implement this control method
>>> with
>>> > +cached value, the initial returning value is likely not reliable. There are
>>> > +simply so many examples always retuning "closed" as initial lid state.
>>> > +
>>> > +2. Restrictions of the lid state change notifications
>>> > +
>>> > +There are many AML tables never notifying when the lid device state is
>>> > +changed to "opened". But it is ensured that the AML tables always
>>> notify
>>> > +"closed" when the lid state is changed to "closed". This is normally used
>>> > +to trigger some system power saving operations on Windows. Since it is
>>> > +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
>>> > +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.
>>
>>>
>>> > +
>>> > +New userspace programs should rely on the lid "closed" notification to
>>> > +trigger some power saving operations and may stop taking actions
>>> according
>>> > +to the lid "opened" notification. A new input switch event -
>>> SW_ACPI_LID is
>>> > +prepared for the new userspace to implement this ACPI control method
>>> lid
>>> > +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
>>> how to handle.
>> [Lv Zheng]
>> It should be KEY_LID_CLOSE.
>
> yep, sorry.
>
>> 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 :)
>
>> 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.

>
>>>
>>> > +
>>> > +During the period the userspace hasn't been switched to use the new
>>> > +SW_ACPI_LID event, Linux users can use the following boot parameter
>>> to
>>> > +handle possible issues:
>>> > +  button.lid_init_state=method:
>>> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>>> > +   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
>>> > +   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
>>> > +   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).

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

Cheers,
Benjamin

>>
>> Cheers,
>> -Lv

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

[Dmitry Torokhov]

On Mon, Jul 11, 2016 at 01:34:08PM +0200, Benjamin Tissoires wrote:
> On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> >> Hi,
> >>
> >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> >> > There are many AML tables reporting wrong initial lid state, and some of
> >> > them never reports lid state. As a proxy layer acting between, ACPI button
> >> > 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 <lv.zheng@intel.com>
> >> > Cc: Bastien Nocera: <hadess@hadess.net>
> >> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >> > Cc: linux-input@vger.kernel.org
> >> > ---
> >> >  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
> >> >  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-lid.txt
> >> > 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 a
> >> > +control method lid device. To implement this, the AML tables issue
> >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> >> > +changed. The _LID control method for the lid device must be implemented to
> >> > +report the "current" state of the lid as either "opened" or "closed".
> >> > +
> >> > +This document describes the restrictions and the expections of the Linux
> >> > +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.
> >> > +However the word of "current" has ambiguity, many AML tables return the lid
> >> > +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
> >> > +control method is evaluated during the runtime, the problem is its initial
> >> > +returning value. When the AML tables implement this control method with
> >> > +cached value, the initial returning value is likely not reliable. There are
> >> > +simply so many examples always retuning "closed" as initial lid state.
> >> > +
> >> > +2. Restrictions of the lid state change notifications
> >> > +
> >> > +There are many AML tables never notifying when the lid device state is
> >> > +changed to "opened". But it is ensured that the AML tables always notify
> >> > +"closed" when the lid state is changed to "closed". This is normally used
> >> > +to trigger some system power saving operations on Windows. Since it is
> >> > +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
> >> > +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.
> >>
> >> > +
> >> > +New userspace programs should rely on the lid "closed" notification to
> >> > +trigger some power saving operations and may stop taking actions according
> >> > +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
> >> > +prepared for the new userspace to implement this ACPI control method lid
> >> > +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
> >> how to handle.
> >>
> >> > +
> >> > +During the period the userspace hasn't been switched to use the new
> >> > +SW_ACPI_LID event, Linux users can use the following boot parameter to
> >> > +handle possible issues:
> >> > +  button.lid_init_state=method:
> >> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> >> > +   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
> >> > +   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
> >> > +   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.
> >
> > How about we leave the kernel alone and userspace (which would have to
> > cope with the new KEY_LID_OPEN anyway) would just have to know that if
> > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then it
> > can't trust the events and it needs additional heuristics.
> >
> 
> I really wished we could leave the kernel alone, but some platform
> need fixes: we are using an EV_SW, and on those platform, we only get
> the close event, which means it gets ignored by the input layer.

OK. Can we then emit missing "open" if we get "close" and the state is
already closed?

Thanks.

-- 
Dmitry

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

[Zheng, Lv]

Hi,

> From: Bastien Nocera [mailto:hadess@hadess.net]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Mon, 2016-07-11 at 03:20 +0000, Zheng, Lv wrote:
> >
> <snip>
> > > 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".
> >
> > 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.
> 
> For the affected devices? Sure, but I don't think that's a reasonable
> thing to do for "all" the devices. We have a majority of laptops where
> this isn't a problem, and it's not even a problem any more on one of
> the devices that triggered this discussion (there's a patch for make
> the LID status match reality for the Surface 3).
[Lv Zheng] 
It looks, even with this fixed, there are tables never generating "lid open" event.
Thus the lid notification is definitely not a "switch event".

Thanks and best regards
-Lv

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

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> >> Hi,
> >>
> >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> >> > There are many AML tables reporting wrong initial lid state, and some
> of
> >> > them never reports lid state. As a proxy layer acting between, ACPI
> button
> >> > 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 <lv.zheng@intel.com>
> >> > Cc: Bastien Nocera: <hadess@hadess.net>
> >> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >> > Cc: linux-input@vger.kernel.org
> >> > ---
> >> >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> >> >  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-lid.txt
> >> > 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 a
> >> > +control method lid device. To implement this, the AML tables issue
> >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state
> has
> >> > +changed. The _LID control method for the lid device must be
> implemented to
> >> > +report the "current" state of the lid as either "opened" or "closed".
> >> > +
> >> > +This document describes the restrictions and the expections of the
> Linux
> >> > +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.
> >> > +However the word of "current" has ambiguity, many AML tables
> return the lid
> >> > +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
> >> > +control method is evaluated during the runtime, the problem is its
> initial
> >> > +returning value. When the AML tables implement this control
> method with
> >> > +cached value, the initial returning value is likely not reliable. There
> are
> >> > +simply so many examples always retuning "closed" as initial lid state.
> >> > +
> >> > +2. Restrictions of the lid state change notifications
> >> > +
> >> > +There are many AML tables never notifying when the lid device state
> is
> >> > +changed to "opened". But it is ensured that the AML tables always
> notify
> >> > +"closed" when the lid state is changed to "closed". This is normally
> used
> >> > +to trigger some system power saving operations on Windows. Since
> it is
> >> > +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
> >> > +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.
> >>
> >> > +
> >> > +New userspace programs should rely on the lid "closed" notification
> to
> >> > +trigger some power saving operations and may stop taking actions
> according
> >> > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> >> > +prepared for the new userspace to implement this ACPI control
> method lid
> >> > +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
> >> how to handle.
> >>
> >> > +
> >> > +During the period the userspace hasn't been switched to use the
> new
> >> > +SW_ACPI_LID event, Linux users can use the following boot
> parameter to
> >> > +handle possible issues:
> >> > +  button.lid_init_state=method:
> >> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> >> > +   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
> >> > +   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
> >> > +   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.
> >
> > How about we leave the kernel alone and userspace (which would have
> to
> > cope with the new KEY_LID_OPEN anyway) would just have to know that
> if
> > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then
> it
> > can't trust the events and it needs additional heuristics.
> >
> 
> I really wished we could leave the kernel alone, but some platform
> need fixes: we are using an EV_SW, and on those platform, we only get
> the close event, which means it gets ignored by the input layer.
> Those platform have a form factor which makes the situation acceptable
> (tablet with cover, or very low cost laptops).
> However, switching away from a different EV_SW and tell userspace to
> ignore it would break systems when you are using a docking station.
> The acpi would provide fake values for the LID switch and this will
> screw the session if you are working on a docking station with an
> external monitor and the LID closed.
> 
> My initial suggestion was:
> - detect in the kernel whether the ACPI LID information was judged as
> reliable
> - if reliable keep things as it
> - if not reliable add an extra KEY_LID_CLOSE to notify userspace that
> the SW_LID is not reliable (it will emulate the LID open events) and
> that it will only get true close events.
[Lv Zheng] 
The problem is:
ACPI subsystem has no idea if it is reliable.

The only possible mean to get this awareness is:
1. Waiting for the user report,
2. Check the AML tables and confirm if this is the problem,
3. Add a quirk in the kernel.
As there could be many such kind of "unreliable tables", the quirk list will be endless.
That's the motivation of this discussion:
We need to find a way, end the need of introducing such kind of endless quirk list into the kernel.

The kernel parameter "button.lid_init_state" is a solution to eliminate the need of writing such an endless quirk table.
However, it doesn't fix anything.
Some unwanted power consumptions are caused by the userspace behavior.
And it is not fixable in the kernel. That's the motivation for us to discuss here.

> 
> After further thoughts, I think we can simply add the extra key, and
> have an hwdb entry in logind to enumerate the devices only relying on
> the close event. If the userspace is not updated, we can tell user to
> use the button.lid_init_state=open quirk to simulate the behavior
> logind should expect.
> 
> That means only adding one extra key (and its events) in the kernel
> and let userspace decide what to do.

[Lv Zheng] 
Yes. All sounds reasonable.

But TBH, I really do not know how can ACPI subsystem determine the following stuffs automatically:
1. if _LID initial state is reliable or not,
2. will open event be sent by the platform or not.

So my choice will be:
Leaving this to be determined by the users.
We can have a kernel parameter, switching acpi button driver between SW_LID and KEY_LID_CLOSE.
IMO, this should be sufficient for the vendors.

Thanks and best regards
-Lv

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

[Zheng, Lv]

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
> <benjamin.tissoires@gmail.com> wrote:
> > On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> >> Hi,
> >>
> >>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> >>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI
> control
> >>> method lid device restrictions
> >>>
> >>> Hi,
> >>>
> >>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> >>> > There are many AML tables reporting wrong initial lid state, and
> some of
> >>> > them never reports lid state. As a proxy layer acting between, ACPI
> >>> button
> >>> > 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 <lv.zheng@intel.com>
> >>> > Cc: Bastien Nocera: <hadess@hadess.net>
> >>> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >>> > Cc: linux-input@vger.kernel.org
> >>> > ---
> >>> >  Documentation/acpi/acpi-lid.txt |   62
> >>> +++++++++++++++++++++++++++++++++++++++
> >>> >  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-
> >>> lid.txt
> >>> > 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
> >>> a
> >>> > +control method lid device. To implement this, the AML tables issue
> >>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state
> has
> >>> > +changed. The _LID control method for the lid device must be
> >>> implemented to
> >>> > +report the "current" state of the lid as either "opened" or "closed".
> >>> > +
> >>> > +This document describes the restrictions and the expections of the
> >>> Linux
> >>> > +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.
> >>> > +However the word of "current" has ambiguity, many AML tables
> return
> >>> the lid
> >>> > +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
> >>> > +control method is evaluated during the runtime, the problem is its
> >>> initial
> >>> > +returning value. When the AML tables implement this control
> method
> >>> with
> >>> > +cached value, the initial returning value is likely not reliable. There
> are
> >>> > +simply so many examples always retuning "closed" as initial lid
> state.
> >>> > +
> >>> > +2. Restrictions of the lid state change notifications
> >>> > +
> >>> > +There are many AML tables never notifying when the lid device
> state is
> >>> > +changed to "opened". But it is ensured that the AML tables always
> >>> notify
> >>> > +"closed" when the lid state is changed to "closed". This is normally
> used
> >>> > +to trigger some system power saving operations on Windows.
> Since it is
> >>> > +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
> >>> > +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.
> >>
> >>>
> >>> > +
> >>> > +New userspace programs should rely on the lid "closed"
> notification to
> >>> > +trigger some power saving operations and may stop taking actions
> >>> according
> >>> > +to the lid "opened" notification. A new input switch event -
> >>> SW_ACPI_LID is
> >>> > +prepared for the new userspace to implement this ACPI control
> method
> >>> lid
> >>> > +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
> >>> how to handle.
> >> [Lv Zheng]
> >> It should be KEY_LID_CLOSE.
> >
> > yep, sorry.
> >
> >> 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 :)
> >
> >> 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.

> 
> >
> >>>
> >>> > +
> >>> > +During the period the userspace hasn't been switched to use the
> new
> >>> > +SW_ACPI_LID event, Linux users can use the following boot
> parameter
> >>> to
> >>> > +handle possible issues:
> >>> > +  button.lid_init_state=method:
> >>> > +   This is the default behavior of the Linux ACPI lid driver, Linux
> kernel
> >>> > +   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
> >>> > +   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
> >>> > +   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.

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

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

[Zheng, Lv]

Hi, Dmitry

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Mon, Jul 11, 2016 at 01:34:08PM +0200, Benjamin Tissoires wrote:
> > On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> > >> Hi,
> > >>
> > >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
> wrote:
> > >> > There are many AML tables reporting wrong initial lid state, and
> some of
> > >> > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > >> > 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 <lv.zheng@intel.com>
> > >> > Cc: Bastien Nocera: <hadess@hadess.net>
> > >> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > >> > Cc: linux-input@vger.kernel.org
> > >> > ---
> > >> >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> > >> >  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-lid.txt
> > >> > 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 a
> > >> > +control method lid device. To implement this, the AML tables issue
> > >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
> state has
> > >> > +changed. The _LID control method for the lid device must be
> implemented to
> > >> > +report the "current" state of the lid as either "opened" or "closed".
> > >> > +
> > >> > +This document describes the restrictions and the expections of the
> Linux
> > >> > +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.
> > >> > +However the word of "current" has ambiguity, many AML tables
> return the lid
> > >> > +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
> > >> > +control method is evaluated during the runtime, the problem is its
> initial
> > >> > +returning value. When the AML tables implement this control
> method with
> > >> > +cached value, the initial returning value is likely not reliable. There
> are
> > >> > +simply so many examples always retuning "closed" as initial lid
> state.
> > >> > +
> > >> > +2. Restrictions of the lid state change notifications
> > >> > +
> > >> > +There are many AML tables never notifying when the lid device
> state is
> > >> > +changed to "opened". But it is ensured that the AML tables always
> notify
> > >> > +"closed" when the lid state is changed to "closed". This is normally
> used
> > >> > +to trigger some system power saving operations on Windows.
> Since it is
> > >> > +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
> > >> > +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.
> > >>
> > >> > +
> > >> > +New userspace programs should rely on the lid "closed"
> notification to
> > >> > +trigger some power saving operations and may stop taking actions
> according
> > >> > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > >> > +prepared for the new userspace to implement this ACPI control
> method lid
> > >> > +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
> > >> how to handle.
> > >>
> > >> > +
> > >> > +During the period the userspace hasn't been switched to use the
> new
> > >> > +SW_ACPI_LID event, Linux users can use the following boot
> parameter to
> > >> > +handle possible issues:
> > >> > +  button.lid_init_state=method:
> > >> > +   This is the default behavior of the Linux ACPI lid driver, Linux
> kernel
> > >> > +   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
> > >> > +   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
> > >> > +   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.
> > >
> > > How about we leave the kernel alone and userspace (which would have
> to
> > > cope with the new KEY_LID_OPEN anyway) would just have to know
> that if
> > > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
> then it
> > > can't trust the events and it needs additional heuristics.
> > >
> >
> > I really wished we could leave the kernel alone, but some platform
> > need fixes: we are using an EV_SW, and on those platform, we only get
> > the close event, which means it gets ignored by the input layer.
> 
> OK. Can we then emit missing "open" if we get "close" and the state is
> already closed?
[Lv Zheng] 
The problem is systemd/logind thinks SW_LID is a switch event, thus it must be paired.
While the real world is: there are platforms, SW_LID may not be paired for ACPI control method lid device.

As the events are provided by the BIOS tables, the Linux ACPI subsystem actually has no idea on how to get the missing "open" back.
Unless we hire people to wait for the bug reports and fix the BIOS tables.

Thanks and best regards
-Lv

[PATCH v3 1/2] ACPI / button: Add KEY_LID_CLOSE for new usage model

[Lv Zheng]

There are many AML tables reporting wrong initial lid state, and some of
them never reports lid open state. As a proxy layer acting between, ACPI
button 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 an 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 a new input key event so that the new userspace programs
can use it to handle this usage model correctly. And in the meanwhile, no
old programs will be broken by the userspace changes.
This patch also adds a button.lid_event_type parameter to allow the users
to switch between the 2 event types.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: linux-input@vger.kernel.org
---
 drivers/acpi/button.c                  |  109 +++++++++++++++++++++++++-------
 include/uapi/linux/input-event-codes.h |    6 ++
 2 files changed, 91 insertions(+), 24 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 148f4e5..1298ef8 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -57,6 +57,9 @@
 #define ACPI_BUTTON_LID_INIT_OPEN	0x01
 #define ACPI_BUTTON_LID_INIT_METHOD	0x02
 
+#define ACPI_BUTTON_LID_EVENT_KEY	0x00
+#define ACPI_BUTTON_LID_EVENT_SWITCH	0x01
+
 #define _COMPONENT		ACPI_BUTTON_COMPONENT
 ACPI_MODULE_NAME("button");
 
@@ -110,6 +113,7 @@ struct acpi_button {
 static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
 static struct acpi_device *lid_device;
 static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
+static u8 lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH;
 
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
@@ -136,8 +140,17 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
 	int ret;
 
 	/* input layer checks if event is redundant */
-	input_report_switch(button->input, SW_LID, !state);
-	input_sync(button->input);
+	if (lid_event_type == ACPI_BUTTON_LID_EVENT_SWITCH) {
+		input_report_switch(button->input, SW_LID, !state);
+		input_sync(button->input);
+	}
+	if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY &&
+	   !state) {
+		input_report_key(button->input, KEY_LID_CLOSE, 1);
+		input_sync(button->input);
+		input_report_key(button->input, KEY_LID_CLOSE, 0);
+		input_sync(button->input);
+	}
 
 	if (state)
 		pm_wakeup_event(&device->dev, 0);
@@ -292,6 +305,9 @@ static int acpi_lid_update_state(struct acpi_device *device)
 
 static void acpi_lid_initialize_state(struct acpi_device *device)
 {
+	if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY)
+		return;
+
 	switch (lid_init_state) {
 	case ACPI_BUTTON_LID_INIT_OPEN:
 		(void)acpi_lid_notify_state(device, 1);
@@ -436,6 +452,7 @@ static int acpi_button_add(struct acpi_device *device)
 
 	case ACPI_BUTTON_TYPE_LID:
 		input_set_capability(input, EV_SW, SW_LID);
+		input_set_capability(input, EV_KEY, KEY_LID_CLOSE);
 		break;
 	}
 
@@ -475,35 +492,49 @@ static int acpi_button_remove(struct acpi_device *device)
 
 static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
 {
-	int result = 0;
-
-	if (!strncmp(val, "open", sizeof("open") - 1)) {
-		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
-		pr_info("Notify initial lid state as open\n");
-	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
-		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
-		pr_info("Notify initial lid state with _LID return value\n");
-	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
-		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
-		pr_info("Do not notify initial lid state\n");
-	} else
-		result = -EINVAL;
+	int result = -EINVAL;
+
+	switch (lid_event_type) {
+	case ACPI_BUTTON_LID_EVENT_KEY:
+		if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
+			lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
+			pr_info("Do not notify initial lid state\n");
+		}
+		break;
+	case ACPI_BUTTON_LID_EVENT_SWITCH:
+		if (!strncmp(val, "open", sizeof("open") - 1)) {
+			lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
+			pr_info("Notify initial lid state as open\n");
+		} else if (!strncmp(val, "method", sizeof("method") - 1)) {
+			lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
+			pr_info("Notify initial lid state"
+				" with _LID return value\n");
+		} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
+			lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
+			pr_info("Do not notify initial lid state\n");
+		}
+		break;
+	}
 	return result;
 }
 
 static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
 {
-	switch (lid_init_state) {
-	case ACPI_BUTTON_LID_INIT_OPEN:
-		return sprintf(buffer, "open");
-	case ACPI_BUTTON_LID_INIT_METHOD:
-		return sprintf(buffer, "method");
-	case ACPI_BUTTON_LID_INIT_IGNORE:
+	switch (lid_event_type) {
+	case ACPI_BUTTON_LID_EVENT_KEY:
 		return sprintf(buffer, "ignore");
-	default:
-		return sprintf(buffer, "invalid");
+	case ACPI_BUTTON_LID_EVENT_SWITCH:
+		switch (lid_init_state) {
+		case ACPI_BUTTON_LID_INIT_OPEN:
+			return sprintf(buffer, "open");
+		case ACPI_BUTTON_LID_INIT_METHOD:
+			return sprintf(buffer, "method");
+		case ACPI_BUTTON_LID_INIT_IGNORE:
+			return sprintf(buffer, "ignore");
+		}
+		break;
 	}
-	return 0;
+	return sprintf(buffer, "invalid");
 }
 
 module_param_call(lid_init_state,
@@ -511,4 +542,34 @@ module_param_call(lid_init_state,
 		  NULL, 0644);
 MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
 
+static int param_set_lid_event_type(const char *val, struct kernel_param *kp)
+{
+	int result = -EINVAL;
+
+	if (!strncmp(val, "key", sizeof("key") - 1)) {
+		lid_event_type = ACPI_BUTTON_LID_EVENT_KEY;
+		pr_info("Notify lid state using key event\n");
+	} else if (!strncmp(val, "switch", sizeof("switch") - 1)) {
+		lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH;
+		pr_info("Notify lid state using switch event\n");
+	}
+	return result;
+}
+
+static int param_get_lid_event_type(char *buffer, struct kernel_param *kp)
+{
+	switch (lid_event_type) {
+	case ACPI_BUTTON_LID_EVENT_KEY:
+		return sprintf(buffer, "key");
+	case ACPI_BUTTON_LID_EVENT_SWITCH:
+		return sprintf(buffer, "switch");
+	}
+	return sprintf(buffer, "invalid");
+}
+
+module_param_call(lid_event_type,
+		  param_set_lid_event_type, param_get_lid_event_type,
+		  NULL, 0644);
+MODULE_PARM_DESC(lid_event_type, "Event type for reporting LID state");
+
 module_acpi_driver(acpi_button_driver);
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 737fa32..df7c0c0 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -641,6 +641,12 @@
  * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.)
  */
 #define KEY_DATA			0x275
+/*
+ * Special event sent by the lid drivers. The drivers may not be able to
+ * issue "open" event, in which case, they send KEY_LID_CLOSE instead of
+ * SW_LID.
+ */
+#define KEY_LID_CLOSE			0x278
 
 #define BTN_TRIGGER_HAPPY		0x2c0
 #define BTN_TRIGGER_HAPPY1		0x2c0
-- 
1.7.10

[PATCH v3 2/2] ACPI / button: Add document for ACPI control method lid device restrictions

[Lv Zheng]

There are many AML tables reporting wrong initial lid state, and some of
them never reports lid open state. As a proxy layer acting between, ACPI
button 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 an 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.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: linux-input@vger.kernel.org
---
 Documentation/acpi/acpi-lid.txt |   89 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/acpi/acpi-lid.txt

diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
new file mode 100644
index 0000000..5cf587c
--- /dev/null
+++ b/Documentation/acpi/acpi-lid.txt
@@ -0,0 +1,89 @@
+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 a
+control method lid device. To implement this, the AML tables issue
+Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
+changed. The _LID control method for the lid device must be implemented to
+report the "current" state of the lid as either "opened" or "closed".
+
+This document describes the restrictions and the expections of the Linux
+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.
+However the word of "current" has ambiguity, many AML tables return the lid
+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
+control method is evaluated during the runtime, the problem is its initial
+returning value. When the AML tables implement this control method with
+cached value, the initial returning value is likely not reliable. There are
+simply so many examples always retuning "closed" as initial lid state.
+
+2. Restrictions of the lid state change notifications
+
+There are many AML tables never notifying when the lid device state is
+changed to "opened". Thus the "opened" notification is not guaranteed.
+
+But it is guaranteed that the AML tables always notify "closed" when the
+lid state is changed to "closed". The "closed" notification is normally
+used to trigger some system power saving operations on Windows. Since it is
+fully tested, the "closed" notification is reliable for all AML tables.
+
+3. Expections for the userspace users of the ACPI lid device driver
+
+The ACPI button driver exports the lid state to the userspace via the
+following file:
+  /proc/acpi/button/lid/LID0/state
+This file actually calls the _LID control method described above. And given
+the previous explanation, it is not reliable enough on some platforms. So
+it is advised for the userspace program to not to solely rely on this file
+to determine the actual lid state.
+
+Linux users can switch the ACPI button driver behavior via the following
+kernel parameters:
+A. button.lid_event_type=switch:
+   When the lid state/event is reliable, the users can specify this option
+   (or nothing as this is the default option) to load the ACPI button
+   driver. The ACPI button driver will send the traditional "SW_LID" event
+   to the userspace.
+B. button.lid_event_type=key:
+   When the lid state/event is not reliable, the users can specify this
+   option to load the ACPI button driver. The ACPI button driver will send
+   the "KEY_LID_CLOSE" event instead of the "SW_LID" to the userspace.
+
+If the userspace hasn't been prepared to handle the new KEY_LID_CLOSE
+event, Linux users can use the following kernel parameters to handle the
+possible issues triggered because of the unreliable lid state/event:
+C. button.lid_init_state=method:
+   This option is only effective when the event type is "switch". When this
+   option is specified, the ACPI button driver reports the initial lid
+   state using the returning value of the _LID control method.
+   This option can be used to fix some platforms where the _LID control
+   method's returning value is reliable but the initial lid state
+   notification is missing.
+D. button.lid_init_state=open:
+   This option is only effective when the event type is "switch". When this
+   option is specified, the ACPI button driver always reports the initial
+   lid state as "opened".
+   This may fix some platforms where the returning value of the _LID
+   control method is not reliable and the initial lid state notification is
+   missing.
+
+If the userspace has been prepared to handle the new KEY_LID_CLOSE event,
+Linux users should always use the following kernel parameter:
+E. button.lid_init_state=ignore:
+   This option allows the users to switch the "event type" between the
+   "switch" and the "key". When this option is specified, the ACPI button
+   driver never reports the initial lid state. However, the platform may
+   automatically report a correct initial lid state and there is no "open"
+   event missing. When this is the case (everything is correctly
+   implemented by the platform firmware), the "event type" should be
+   "switch", otherwise, the "event type" should be "key".
-- 
1.7.10

Re: [PATCH v3 1/2] ACPI / button: Add KEY_LID_CLOSE for new usage model

[Benjamin Tissoires]

Hi,

On Tue, Jul 12, 2016 at 12:17 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> There are many AML tables reporting wrong initial lid state, and some of
> them never reports lid open state. As a proxy layer acting between, ACPI
> button 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 an 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 a new input key event so that the new userspace programs
> can use it to handle this usage model correctly. And in the meanwhile, no
> old programs will be broken by the userspace changes.
> This patch also adds a button.lid_event_type parameter to allow the users
> to switch between the 2 event types.

I think we are getting closer to what would be acceptable for
user-space, but I still have a few comments:

>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> Cc: Bastien Nocera: <hadess@hadess.net>
> Cc: linux-input@vger.kernel.org
> ---
>  drivers/acpi/button.c                  |  109 +++++++++++++++++++++++++-------
>  include/uapi/linux/input-event-codes.h |    6 ++
>  2 files changed, 91 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 148f4e5..1298ef8 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -57,6 +57,9 @@
>  #define ACPI_BUTTON_LID_INIT_OPEN      0x01
>  #define ACPI_BUTTON_LID_INIT_METHOD    0x02
>
> +#define ACPI_BUTTON_LID_EVENT_KEY      0x00
> +#define ACPI_BUTTON_LID_EVENT_SWITCH   0x01
> +
>  #define _COMPONENT             ACPI_BUTTON_COMPONENT
>  ACPI_MODULE_NAME("button");
>
> @@ -110,6 +113,7 @@ struct acpi_button {
>  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
>  static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> +static u8 lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH;
>
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
> @@ -136,8 +140,17 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
>         int ret;
>
>         /* input layer checks if event is redundant */
> -       input_report_switch(button->input, SW_LID, !state);
> -       input_sync(button->input);
> +       if (lid_event_type == ACPI_BUTTON_LID_EVENT_SWITCH) {
> +               input_report_switch(button->input, SW_LID, !state);
> +               input_sync(button->input);
> +       }
> +       if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY &&
> +          !state) {
> +               input_report_key(button->input, KEY_LID_CLOSE, 1);
> +               input_sync(button->input);
> +               input_report_key(button->input, KEY_LID_CLOSE, 0);
> +               input_sync(button->input);
> +       }
>
>         if (state)
>                 pm_wakeup_event(&device->dev, 0);
> @@ -292,6 +305,9 @@ static int acpi_lid_update_state(struct acpi_device *device)
>
>  static void acpi_lid_initialize_state(struct acpi_device *device)
>  {
> +       if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY)
> +               return;
> +
>         switch (lid_init_state) {
>         case ACPI_BUTTON_LID_INIT_OPEN:
>                 (void)acpi_lid_notify_state(device, 1);
> @@ -436,6 +452,7 @@ static int acpi_button_add(struct acpi_device *device)
>
>         case ACPI_BUTTON_TYPE_LID:
>                 input_set_capability(input, EV_SW, SW_LID);
> +               input_set_capability(input, EV_KEY, KEY_LID_CLOSE);

Here you are basically exporting the 2 input events but only update
one of the 2. It will confuse userspace and it is generally better not
to export unused input codes.

However, as I'll say below, I think we should keep the code that way here.


>                 break;
>         }
>
> @@ -475,35 +492,49 @@ static int acpi_button_remove(struct acpi_device *device)
>
>  static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
>  {
> -       int result = 0;
> -
> -       if (!strncmp(val, "open", sizeof("open") - 1)) {
> -               lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> -               pr_info("Notify initial lid state as open\n");
> -       } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> -               lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> -               pr_info("Notify initial lid state with _LID return value\n");
> -       } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> -               lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> -               pr_info("Do not notify initial lid state\n");
> -       } else
> -               result = -EINVAL;
> +       int result = -EINVAL;
> +
> +       switch (lid_event_type) {
> +       case ACPI_BUTTON_LID_EVENT_KEY:
> +               if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> +                       lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> +                       pr_info("Do not notify initial lid state\n");
> +               }
> +               break;
> +       case ACPI_BUTTON_LID_EVENT_SWITCH:
> +               if (!strncmp(val, "open", sizeof("open") - 1)) {
> +                       lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> +                       pr_info("Notify initial lid state as open\n");
> +               } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> +                       lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> +                       pr_info("Notify initial lid state"
> +                               " with _LID return value\n");
> +               } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> +                       lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> +                       pr_info("Do not notify initial lid state\n");
> +               }
> +               break;
> +       }
>         return result;
>  }
>
>  static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
>  {
> -       switch (lid_init_state) {
> -       case ACPI_BUTTON_LID_INIT_OPEN:
> -               return sprintf(buffer, "open");
> -       case ACPI_BUTTON_LID_INIT_METHOD:
> -               return sprintf(buffer, "method");
> -       case ACPI_BUTTON_LID_INIT_IGNORE:
> +       switch (lid_event_type) {
> +       case ACPI_BUTTON_LID_EVENT_KEY:
>                 return sprintf(buffer, "ignore");
> -       default:
> -               return sprintf(buffer, "invalid");
> +       case ACPI_BUTTON_LID_EVENT_SWITCH:
> +               switch (lid_init_state) {
> +               case ACPI_BUTTON_LID_INIT_OPEN:
> +                       return sprintf(buffer, "open");
> +               case ACPI_BUTTON_LID_INIT_METHOD:
> +                       return sprintf(buffer, "method");
> +               case ACPI_BUTTON_LID_INIT_IGNORE:
> +                       return sprintf(buffer, "ignore");
> +               }
> +               break;
>         }
> -       return 0;
> +       return sprintf(buffer, "invalid");
>  }
>
>  module_param_call(lid_init_state,
> @@ -511,4 +542,34 @@ module_param_call(lid_init_state,
>                   NULL, 0644);
>  MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
>
> +static int param_set_lid_event_type(const char *val, struct kernel_param *kp)
> +{
> +       int result = -EINVAL;
> +
> +       if (!strncmp(val, "key", sizeof("key") - 1)) {
> +               lid_event_type = ACPI_BUTTON_LID_EVENT_KEY;
> +               pr_info("Notify lid state using key event\n");
> +       } else if (!strncmp(val, "switch", sizeof("switch") - 1)) {
> +               lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH;
> +               pr_info("Notify lid state using switch event\n");
> +       }
> +       return result;
> +}
> +
> +static int param_get_lid_event_type(char *buffer, struct kernel_param *kp)
> +{
> +       switch (lid_event_type) {
> +       case ACPI_BUTTON_LID_EVENT_KEY:
> +               return sprintf(buffer, "key");
> +       case ACPI_BUTTON_LID_EVENT_SWITCH:
> +               return sprintf(buffer, "switch");
> +       }
> +       return sprintf(buffer, "invalid");
> +}
> +
> +module_param_call(lid_event_type,
> +                 param_set_lid_event_type, param_get_lid_event_type,
> +                 NULL, 0644);
> +MODULE_PARM_DESC(lid_event_type, "Event type for reporting LID state");

I don't think this is a good solution to have a kernel parameter. I
thought the final decision were to have userspace decide which event
was valid, and so we just need to export and emit both of events.

_If_ you export a kernel parameter, it makes sense to have a dmi
blacklist to have a good default experience, which is what you wanted
to avoid.
So if you just export and use both events at the same time, you will have:
- correct ACPI machines will just have an extra KEY_LID_CLOSE event
emitted, which will not harm logind
- wrong ACPI machines will not have their SW_LID input event updated
because it will be kept closed. But given that logind will ignore it,
there is no harm either

As Dmitry said, we could also have a KEY_LID_OPEN emitted for
symmetrical purposes, but I am not entirely sure if this will confuse
userspace or not. On the other hand, there are few users of these
states, and we can teach them how to properly use them.

So in the end, I think you should just get rid of the kernel
parameter, export SW_LID, KEY_LID_CLOSE, KEY_LID_OPEN in the event
node, and only add the KEY_LID_CLOSE|OPEN events on an actual acpi
notification.

Then a small hwdb entry set will teach logind/powerd if they need to
ignore the SW_LID event or not.

Cheers,
Benjamin

> +
>  module_acpi_driver(acpi_button_driver);
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 737fa32..df7c0c0 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -641,6 +641,12 @@
>   * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.)
>   */
>  #define KEY_DATA                       0x275
> +/*
> + * Special event sent by the lid drivers. The drivers may not be able to
> + * issue "open" event, in which case, they send KEY_LID_CLOSE instead of
> + * SW_LID.
> + */
> +#define KEY_LID_CLOSE                  0x278
>
>  #define BTN_TRIGGER_HAPPY              0x2c0
>  #define BTN_TRIGGER_HAPPY1             0x2c0
> --
> 1.7.10
>

Re: [PATCH v3 1/2] ACPI / button: Add KEY_LID_CLOSE for new usage model

[Bastien Nocera]

On Mon, 2016-07-18 at 09:53 +0200, Benjamin Tissoires wrote:
> 
<snip>
> I don't think this is a good solution to have a kernel parameter. I
> thought the final decision were to have userspace decide which event
> was valid, and so we just need to export and emit both of events.
> 
> _If_ you export a kernel parameter, it makes sense to have a dmi
> blacklist to have a good default experience, which is what you wanted
> to avoid.
> So if you just export and use both events at the same time, you will
> have:
> - correct ACPI machines will just have an extra KEY_LID_CLOSE event
> emitted, which will not harm logind
> - wrong ACPI machines will not have their SW_LID input event updated
> because it will be kept closed. But given that logind will ignore it,
> there is no harm either
> 
> As Dmitry said, we could also have a KEY_LID_OPEN emitted for
> symmetrical purposes, but I am not entirely sure if this will confuse
> userspace or not. On the other hand, there are few users of these
> states, and we can teach them how to properly use them.
> 
> So in the end, I think you should just get rid of the kernel
> parameter, export SW_LID, KEY_LID_CLOSE, KEY_LID_OPEN in the event
> node, and only add the KEY_LID_CLOSE|OPEN events on an actual acpi
> notification.
> 
> Then a small hwdb entry set will teach logind/powerd if they need to
> ignore the SW_LID event or not.

So user-space would have its own blacklist (likely in udev through an
hwdb), instead of having it in the kernel? That seems like a fine idea
to me, and one of the first consumers, logind, would have all the
necessary data straight away.

RE: [PATCH v3 1/2] ACPI / button: Add KEY_LID_CLOSE for new usage model

[Zheng, Lv]

Hi, Benjamin

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v3 1/2] ACPI / button: Add KEY_LID_CLOSE for new
> usage model
> 
> Hi,
> 
> On Tue, Jul 12, 2016 at 12:17 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> > There are many AML tables reporting wrong initial lid state, and some of
> > them never reports lid open state. As a proxy layer acting between, ACPI
> > button 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 an 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 a new input key event so that the new userspace
> programs
> > can use it to handle this usage model correctly. And in the meanwhile, no
> > old programs will be broken by the userspace changes.
> > This patch also adds a button.lid_event_type parameter to allow the
> users
> > to switch between the 2 event types.
> 
> I think we are getting closer to what would be acceptable for
> user-space, but I still have a few comments:
[Lv Zheng] 
OK.

> 
> >
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > Cc: Bastien Nocera: <hadess@hadess.net>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  drivers/acpi/button.c                  |  109 +++++++++++++++++++++++++--
> -----
> >  include/uapi/linux/input-event-codes.h |    6 ++
> >  2 files changed, 91 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 148f4e5..1298ef8 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -57,6 +57,9 @@
> >  #define ACPI_BUTTON_LID_INIT_OPEN      0x01
> >  #define ACPI_BUTTON_LID_INIT_METHOD    0x02
> >
> > +#define ACPI_BUTTON_LID_EVENT_KEY      0x00
> > +#define ACPI_BUTTON_LID_EVENT_SWITCH   0x01
> > +
> >  #define _COMPONENT             ACPI_BUTTON_COMPONENT
> >  ACPI_MODULE_NAME("button");
> >
> > @@ -110,6 +113,7 @@ struct acpi_button {
> >  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> >  static struct acpi_device *lid_device;
> >  static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > +static u8 lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH;
> >
> >  /* --------------------------------------------------------------------------
> >                                FS Interface (/proc)
> > @@ -136,8 +140,17 @@ static int acpi_lid_notify_state(struct
> acpi_device *device, int state)
> >         int ret;
> >
> >         /* input layer checks if event is redundant */
> > -       input_report_switch(button->input, SW_LID, !state);
> > -       input_sync(button->input);
> > +       if (lid_event_type == ACPI_BUTTON_LID_EVENT_SWITCH) {
> > +               input_report_switch(button->input, SW_LID, !state);
> > +               input_sync(button->input);
> > +       }
> > +       if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY &&
> > +          !state) {
> > +               input_report_key(button->input, KEY_LID_CLOSE, 1);
> > +               input_sync(button->input);
> > +               input_report_key(button->input, KEY_LID_CLOSE, 0);
> > +               input_sync(button->input);
> > +       }
> >
> >         if (state)
> >                 pm_wakeup_event(&device->dev, 0);
> > @@ -292,6 +305,9 @@ static int acpi_lid_update_state(struct
> acpi_device *device)
> >
> >  static void acpi_lid_initialize_state(struct acpi_device *device)
> >  {
> > +       if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY)
> > +               return;
> > +
> >         switch (lid_init_state) {
> >         case ACPI_BUTTON_LID_INIT_OPEN:
> >                 (void)acpi_lid_notify_state(device, 1);
> > @@ -436,6 +452,7 @@ static int acpi_button_add(struct acpi_device
> *device)
> >
> >         case ACPI_BUTTON_TYPE_LID:
> >                 input_set_capability(input, EV_SW, SW_LID);
> > +               input_set_capability(input, EV_KEY, KEY_LID_CLOSE);
> 
> Here you are basically exporting the 2 input events but only update
> one of the 2. It will confuse userspace and it is generally better not
> to export unused input codes.
[Lv Zheng] 
I just do not know if it is proper to clear the capability during runtime via module parameters.


> 
> However, as I'll say below, I think we should keep the code that way here.
[Lv Zheng] 
Great. So we needn't think about input_clear_capability().

> 
> 
> >                 break;
> >         }
> >
> > @@ -475,35 +492,49 @@ static int acpi_button_remove(struct
> acpi_device *device)
> >
> >  static int param_set_lid_init_state(const char *val, struct kernel_param
> *kp)
> >  {
> > -       int result = 0;
> > -
> > -       if (!strncmp(val, "open", sizeof("open") - 1)) {
> > -               lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > -               pr_info("Notify initial lid state as open\n");
> > -       } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> > -               lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > -               pr_info("Notify initial lid state with _LID return value\n");
> > -       } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> > -               lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> > -               pr_info("Do not notify initial lid state\n");
> > -       } else
> > -               result = -EINVAL;
> > +       int result = -EINVAL;
> > +
> > +       switch (lid_event_type) {
> > +       case ACPI_BUTTON_LID_EVENT_KEY:
> > +               if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> > +                       lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> > +                       pr_info("Do not notify initial lid state\n");
> > +               }
> > +               break;
> > +       case ACPI_BUTTON_LID_EVENT_SWITCH:
> > +               if (!strncmp(val, "open", sizeof("open") - 1)) {
> > +                       lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > +                       pr_info("Notify initial lid state as open\n");
> > +               } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> > +                       lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > +                       pr_info("Notify initial lid state"
> > +                               " with _LID return value\n");
> > +               } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> > +                       lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> > +                       pr_info("Do not notify initial lid state\n");
> > +               }
> > +               break;
> > +       }
> >         return result;
> >  }
> >
> >  static int param_get_lid_init_state(char *buffer, struct kernel_param
> *kp)
> >  {
> > -       switch (lid_init_state) {
> > -       case ACPI_BUTTON_LID_INIT_OPEN:
> > -               return sprintf(buffer, "open");
> > -       case ACPI_BUTTON_LID_INIT_METHOD:
> > -               return sprintf(buffer, "method");
> > -       case ACPI_BUTTON_LID_INIT_IGNORE:
> > +       switch (lid_event_type) {
> > +       case ACPI_BUTTON_LID_EVENT_KEY:
> >                 return sprintf(buffer, "ignore");
> > -       default:
> > -               return sprintf(buffer, "invalid");
> > +       case ACPI_BUTTON_LID_EVENT_SWITCH:
> > +               switch (lid_init_state) {
> > +               case ACPI_BUTTON_LID_INIT_OPEN:
> > +                       return sprintf(buffer, "open");
> > +               case ACPI_BUTTON_LID_INIT_METHOD:
> > +                       return sprintf(buffer, "method");
> > +               case ACPI_BUTTON_LID_INIT_IGNORE:
> > +                       return sprintf(buffer, "ignore");
> > +               }
> > +               break;
> >         }
> > -       return 0;
> > +       return sprintf(buffer, "invalid");
> >  }
> >
> >  module_param_call(lid_init_state,
> > @@ -511,4 +542,34 @@ module_param_call(lid_init_state,
> >                   NULL, 0644);
> >  MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial
> state");
> >
> > +static int param_set_lid_event_type(const char *val, struct
> kernel_param *kp)
> > +{
> > +       int result = -EINVAL;
> > +
> > +       if (!strncmp(val, "key", sizeof("key") - 1)) {
> > +               lid_event_type = ACPI_BUTTON_LID_EVENT_KEY;
> > +               pr_info("Notify lid state using key event\n");
> > +       } else if (!strncmp(val, "switch", sizeof("switch") - 1)) {
> > +               lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH;
> > +               pr_info("Notify lid state using switch event\n");
> > +       }
> > +       return result;
> > +}
> > +
> > +static int param_get_lid_event_type(char *buffer, struct kernel_param
> *kp)
> > +{
> > +       switch (lid_event_type) {
> > +       case ACPI_BUTTON_LID_EVENT_KEY:
> > +               return sprintf(buffer, "key");
> > +       case ACPI_BUTTON_LID_EVENT_SWITCH:
> > +               return sprintf(buffer, "switch");
> > +       }
> > +       return sprintf(buffer, "invalid");
> > +}
> > +
> > +module_param_call(lid_event_type,
> > +                 param_set_lid_event_type, param_get_lid_event_type,
> > +                 NULL, 0644);
> > +MODULE_PARM_DESC(lid_event_type, "Event type for reporting LID
> state");
> 
> I don't think this is a good solution to have a kernel parameter. I
> thought the final decision were to have userspace decide which event
> was valid, and so we just need to export and emit both of events.
[Lv Zheng] 
OK.

> 
> _If_ you export a kernel parameter, it makes sense to have a dmi
> blacklist to have a good default experience, which is what you wanted
> to avoid.
[Lv Zheng] 
I was thinking that the distribution vendors can configure a default boot parameters for a specific hardware.

> So if you just export and use both events at the same time, you will have:
> - correct ACPI machines will just have an extra KEY_LID_CLOSE event
> emitted, which will not harm logind
> - wrong ACPI machines will not have their SW_LID input event updated
> because it will be kept closed. But given that logind will ignore it,
> there is no harm either
[Lv Zheng] 
I see.

> 
> As Dmitry said, we could also have a KEY_LID_OPEN emitted for
> symmetrical purposes, but I am not entirely sure if this will confuse
> userspace or not. On the other hand, there are few users of these
> states, and we can teach them how to properly use them.
> 
> So in the end, I think you should just get rid of the kernel
> parameter, export SW_LID, KEY_LID_CLOSE, KEY_LID_OPEN in the event
> node, and only add the KEY_LID_CLOSE|OPEN events on an actual acpi
> notification.
[Lv Zheng] 
I'm sorry I didn't notice the KEY_LID_OPEN comment.
Thanks for the reminder.

> 
> Then a small hwdb entry set will teach logind/powerd if they need to
> ignore the SW_LID event or not.
[Lv Zheng] 
OK, I'll address what you've suggested in this email.

Thanks and best regards
-Lv

> 
> Cheers,
> Benjamin
> 
> > +
> >  module_acpi_driver(acpi_button_driver);
> > diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> > index 737fa32..df7c0c0 100644
> > --- a/include/uapi/linux/input-event-codes.h
> > +++ b/include/uapi/linux/input-event-codes.h
> > @@ -641,6 +641,12 @@
> >   * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.)
> >   */
> >  #define KEY_DATA                       0x275
> > +/*
> > + * Special event sent by the lid drivers. The drivers may not be able to
> > + * issue "open" event, in which case, they send KEY_LID_CLOSE instead
> of
> > + * SW_LID.
> > + */
> > +#define KEY_LID_CLOSE                  0x278
> >
> >  #define BTN_TRIGGER_HAPPY              0x2c0
> >  #define BTN_TRIGGER_HAPPY1             0x2c0
> > --
> > 1.7.10
> >

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

[Zheng, Lv]

Hi, Dmitry

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> > Hi,
> >
> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > > There are many AML tables reporting wrong initial lid state, and some
> of
> > > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > > 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 <lv.zheng@intel.com>
> > > Cc: Bastien Nocera: <hadess@hadess.net>
> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > > Cc: linux-input@vger.kernel.org
> > > ---
> > >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> > >  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-lid.txt
> > > 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 a
> > > +control method lid device. To implement this, the AML tables issue
> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state
> has
> > > +changed. The _LID control method for the lid device must be
> implemented to
> > > +report the "current" state of the lid as either "opened" or "closed".
> > > +
> > > +This document describes the restrictions and the expections of the
> Linux
> > > +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.
> > > +However the word of "current" has ambiguity, many AML tables
> return the lid
> > > +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
> > > +control method is evaluated during the runtime, the problem is its
> initial
> > > +returning value. When the AML tables implement this control method
> with
> > > +cached value, the initial returning value is likely not reliable. There are
> > > +simply so many examples always retuning "closed" as initial lid state.
> > > +
> > > +2. Restrictions of the lid state change notifications
> > > +
> > > +There are many AML tables never notifying when the lid device state
> is
> > > +changed to "opened". But it is ensured that the AML tables always
> notify
> > > +"closed" when the lid state is changed to "closed". This is normally
> used
> > > +to trigger some system power saving operations on Windows. Since it
> is
> > > +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
> > > +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.
> >
> > > +
> > > +New userspace programs should rely on the lid "closed" notification
> to
> > > +trigger some power saving operations and may stop taking actions
> according
> > > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > > +prepared for the new userspace to implement this ACPI control
> method lid
> > > +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
> > how to handle.
> >
> > > +
> > > +During the period the userspace hasn't been switched to use the new
> > > +SW_ACPI_LID event, Linux users can use the following boot parameter
> to
> > > +handle possible issues:
> > > +  button.lid_init_state=method:
> > > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> > > +   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
> > > +   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
> > > +   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.
> 
> How about we leave the kernel alone and userspace (which would have to
> cope with the new KEY_LID_OPEN anyway) would just have to know that if
> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then
> it
> can't trust the events and it needs additional heuristics.

[Lv Zheng] 
I found a problem with the key event approach.
And need your suggestions.

Some AML tables invoke Notify(lid_device, ...) in several different places.
It may be invoked from different functions.

Finally, it's not guaranteed that one "lid close" action can only trigger one key close notification.
If we use EV_KEY, then there should be many platforms triggering multiple "lid close" events to the user space.

Original switch event based design can automatically eliminate the redundant events.

Does input layer has an event type that can handle such situation?
Or shall ACPI button driver handle this?

Thanks,
-Lv

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

[Benjamin Tissoires]

On Tue, Jul 19, 2016 at 9:17 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Dmitry
>
>> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>> method lid device restrictions
>>
>> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
>> > Hi,
>> >
>> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > > There are many AML tables reporting wrong initial lid state, and some
>> of
>> > > them never reports lid state. As a proxy layer acting between, ACPI
>> button
>> > > 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 <lv.zheng@intel.com>
>> > > Cc: Bastien Nocera: <hadess@hadess.net>
>> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> > > Cc: linux-input@vger.kernel.org
>> > > ---
>> > >  Documentation/acpi/acpi-lid.txt |   62
>> +++++++++++++++++++++++++++++++++++++++
>> > >  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-lid.txt
>> > > 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 a
>> > > +control method lid device. To implement this, the AML tables issue
>> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state
>> has
>> > > +changed. The _LID control method for the lid device must be
>> implemented to
>> > > +report the "current" state of the lid as either "opened" or "closed".
>> > > +
>> > > +This document describes the restrictions and the expections of the
>> Linux
>> > > +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.
>> > > +However the word of "current" has ambiguity, many AML tables
>> return the lid
>> > > +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
>> > > +control method is evaluated during the runtime, the problem is its
>> initial
>> > > +returning value. When the AML tables implement this control method
>> with
>> > > +cached value, the initial returning value is likely not reliable. There are
>> > > +simply so many examples always retuning "closed" as initial lid state.
>> > > +
>> > > +2. Restrictions of the lid state change notifications
>> > > +
>> > > +There are many AML tables never notifying when the lid device state
>> is
>> > > +changed to "opened". But it is ensured that the AML tables always
>> notify
>> > > +"closed" when the lid state is changed to "closed". This is normally
>> used
>> > > +to trigger some system power saving operations on Windows. Since it
>> is
>> > > +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
>> > > +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.
>> >
>> > > +
>> > > +New userspace programs should rely on the lid "closed" notification
>> to
>> > > +trigger some power saving operations and may stop taking actions
>> according
>> > > +to the lid "opened" notification. A new input switch event -
>> SW_ACPI_LID is
>> > > +prepared for the new userspace to implement this ACPI control
>> method lid
>> > > +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
>> > how to handle.
>> >
>> > > +
>> > > +During the period the userspace hasn't been switched to use the new
>> > > +SW_ACPI_LID event, Linux users can use the following boot parameter
>> to
>> > > +handle possible issues:
>> > > +  button.lid_init_state=method:
>> > > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>> > > +   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
>> > > +   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
>> > > +   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.
>>
>> How about we leave the kernel alone and userspace (which would have to
>> cope with the new KEY_LID_OPEN anyway) would just have to know that if
>> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then
>> it
>> can't trust the events and it needs additional heuristics.
>
> [Lv Zheng]
> I found a problem with the key event approach.
> And need your suggestions.
>
> Some AML tables invoke Notify(lid_device, ...) in several different places.
> It may be invoked from different functions.
>
> Finally, it's not guaranteed that one "lid close" action can only trigger one key close notification.
> If we use EV_KEY, then there should be many platforms triggering multiple "lid close" events to the user space.
>
> Original switch event based design can automatically eliminate the redundant events.
>
> Does input layer has an event type that can handle such situation?
> Or shall ACPI button driver handle this?
>

Keys also have some redundant event elimination, but it's as long as
you are holding the key in the press (or released) position. So in
your case, that would mean sending input_key(1), wait a little while
other notifications are processed, and then sending input_key(0)
(assuming each notification comes in with its own thread). Not sure
you will gain anything from the new implementation you just sent with
the rate-limit.

Cheers,
Benjamin

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

[Zheng, Lv]

Hi, Benjamin

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Tue, Jul 19, 2016 at 9:17 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi, Dmitry
> >
> >> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> >> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI
> control
> >> method lid device restrictions
> >>
> >> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> >> > Hi,
> >> >
> >> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
> wrote:
> >> > > There are many AML tables reporting wrong initial lid state, and
> some
> >> of
> >> > > them never reports lid state. As a proxy layer acting between, ACPI
> >> button
> >> > > 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 <lv.zheng@intel.com>
> >> > > Cc: Bastien Nocera: <hadess@hadess.net>
> >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >> > > Cc: linux-input@vger.kernel.org
> >> > > ---
> >> > >  Documentation/acpi/acpi-lid.txt |   62
> >> +++++++++++++++++++++++++++++++++++++++
> >> > >  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-lid.txt
> >> > > 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 a
> >> > > +control method lid device. To implement this, the AML tables issue
> >> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
> state
> >> has
> >> > > +changed. The _LID control method for the lid device must be
> >> implemented to
> >> > > +report the "current" state of the lid as either "opened" or "closed".
> >> > > +
> >> > > +This document describes the restrictions and the expections of the
> >> Linux
> >> > > +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.
> >> > > +However the word of "current" has ambiguity, many AML tables
> >> return the lid
> >> > > +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
> >> > > +control method is evaluated during the runtime, the problem is its
> >> initial
> >> > > +returning value. When the AML tables implement this control
> method
> >> with
> >> > > +cached value, the initial returning value is likely not reliable. There
> are
> >> > > +simply so many examples always retuning "closed" as initial lid
> state.
> >> > > +
> >> > > +2. Restrictions of the lid state change notifications
> >> > > +
> >> > > +There are many AML tables never notifying when the lid device
> state
> >> is
> >> > > +changed to "opened". But it is ensured that the AML tables always
> >> notify
> >> > > +"closed" when the lid state is changed to "closed". This is normally
> >> used
> >> > > +to trigger some system power saving operations on Windows.
> Since it
> >> is
> >> > > +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
> >> > > +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.
> >> >
> >> > > +
> >> > > +New userspace programs should rely on the lid "closed"
> notification
> >> to
> >> > > +trigger some power saving operations and may stop taking actions
> >> according
> >> > > +to the lid "opened" notification. A new input switch event -
> >> SW_ACPI_LID is
> >> > > +prepared for the new userspace to implement this ACPI control
> >> method lid
> >> > > +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
> >> > how to handle.
> >> >
> >> > > +
> >> > > +During the period the userspace hasn't been switched to use the
> new
> >> > > +SW_ACPI_LID event, Linux users can use the following boot
> parameter
> >> to
> >> > > +handle possible issues:
> >> > > +  button.lid_init_state=method:
> >> > > +   This is the default behavior of the Linux ACPI lid driver, Linux
> kernel
> >> > > +   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
> >> > > +   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
> >> > > +   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.
> >>
> >> How about we leave the kernel alone and userspace (which would have
> to
> >> cope with the new KEY_LID_OPEN anyway) would just have to know
> that if
> >> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
> then
> >> it
> >> can't trust the events and it needs additional heuristics.
> >
> > [Lv Zheng]
> > I found a problem with the key event approach.
> > And need your suggestions.
> >
> > Some AML tables invoke Notify(lid_device, ...) in several different places.
> > It may be invoked from different functions.
> >
> > Finally, it's not guaranteed that one "lid close" action can only trigger
> one key close notification.
> > If we use EV_KEY, then there should be many platforms triggering
> multiple "lid close" events to the user space.
> >
> > Original switch event based design can automatically eliminate the
> redundant events.
> >
> > Does input layer has an event type that can handle such situation?
> > Or shall ACPI button driver handle this?
> >
> 
> Keys also have some redundant event elimination, but it's as long as
> you are holding the key in the press (or released) position. So in
> your case, that would mean sending input_key(1), wait a little while
> other notifications are processed, and then sending input_key(0)
> (assuming each notification comes in with its own thread). Not sure
> you will gain anything from the new implementation you just sent with
> the rate-limit.

[Lv Zheng] 
However this key is virtual.
The multiple notifications are just triggered by the AML code.
The Notify(lid_device, xxx) may be invoked in a function.
And this function may be invoked multiple times by other control methods.
So I do not know when it is "released".

Using the feature of the keys, it sounds like that I should setup a timer.
When the state is changed, I should report input_key(1) and prepare the timer.
Then report the input_key(0) when the timer times out.
The side effect is the input_key(0) will be deferred.

I just refreshed the patch as v4 with an ACPI button driver internal workaround.
By adding a time_after() check which looks more lightweight than setting up a timer.
Could you also help to check if that solution is OK?
Thanks in advance.

Best regards
-Lv

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

[Benjamin Tissoires]

On Tue, Jul 19, 2016 at 10:57 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Benjamin
>
>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>> method lid device restrictions
>>
>> On Tue, Jul 19, 2016 at 9:17 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> > Hi, Dmitry
>> >
>> >> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> >> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI
>> control
>> >> method lid device restrictions
>> >>
>> >> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
>> >> > Hi,
>> >> >
>> >> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
>> wrote:
>> >> > > There are many AML tables reporting wrong initial lid state, and
>> some
>> >> of
>> >> > > them never reports lid state. As a proxy layer acting between, ACPI
>> >> button
>> >> > > 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 <lv.zheng@intel.com>
>> >> > > Cc: Bastien Nocera: <hadess@hadess.net>
>> >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> >> > > Cc: linux-input@vger.kernel.org
>> >> > > ---
>> >> > >  Documentation/acpi/acpi-lid.txt |   62
>> >> +++++++++++++++++++++++++++++++++++++++
>> >> > >  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-lid.txt
>> >> > > 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 a
>> >> > > +control method lid device. To implement this, the AML tables issue
>> >> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
>> state
>> >> has
>> >> > > +changed. The _LID control method for the lid device must be
>> >> implemented to
>> >> > > +report the "current" state of the lid as either "opened" or "closed".
>> >> > > +
>> >> > > +This document describes the restrictions and the expections of the
>> >> Linux
>> >> > > +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.
>> >> > > +However the word of "current" has ambiguity, many AML tables
>> >> return the lid
>> >> > > +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
>> >> > > +control method is evaluated during the runtime, the problem is its
>> >> initial
>> >> > > +returning value. When the AML tables implement this control
>> method
>> >> with
>> >> > > +cached value, the initial returning value is likely not reliable. There
>> are
>> >> > > +simply so many examples always retuning "closed" as initial lid
>> state.
>> >> > > +
>> >> > > +2. Restrictions of the lid state change notifications
>> >> > > +
>> >> > > +There are many AML tables never notifying when the lid device
>> state
>> >> is
>> >> > > +changed to "opened". But it is ensured that the AML tables always
>> >> notify
>> >> > > +"closed" when the lid state is changed to "closed". This is normally
>> >> used
>> >> > > +to trigger some system power saving operations on Windows.
>> Since it
>> >> is
>> >> > > +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
>> >> > > +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.
>> >> >
>> >> > > +
>> >> > > +New userspace programs should rely on the lid "closed"
>> notification
>> >> to
>> >> > > +trigger some power saving operations and may stop taking actions
>> >> according
>> >> > > +to the lid "opened" notification. A new input switch event -
>> >> SW_ACPI_LID is
>> >> > > +prepared for the new userspace to implement this ACPI control
>> >> method lid
>> >> > > +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
>> >> > how to handle.
>> >> >
>> >> > > +
>> >> > > +During the period the userspace hasn't been switched to use the
>> new
>> >> > > +SW_ACPI_LID event, Linux users can use the following boot
>> parameter
>> >> to
>> >> > > +handle possible issues:
>> >> > > +  button.lid_init_state=method:
>> >> > > +   This is the default behavior of the Linux ACPI lid driver, Linux
>> kernel
>> >> > > +   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
>> >> > > +   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
>> >> > > +   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.
>> >>
>> >> How about we leave the kernel alone and userspace (which would have
>> to
>> >> cope with the new KEY_LID_OPEN anyway) would just have to know
>> that if
>> >> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
>> then
>> >> it
>> >> can't trust the events and it needs additional heuristics.
>> >
>> > [Lv Zheng]
>> > I found a problem with the key event approach.
>> > And need your suggestions.
>> >
>> > Some AML tables invoke Notify(lid_device, ...) in several different places.
>> > It may be invoked from different functions.
>> >
>> > Finally, it's not guaranteed that one "lid close" action can only trigger
>> one key close notification.
>> > If we use EV_KEY, then there should be many platforms triggering
>> multiple "lid close" events to the user space.
>> >
>> > Original switch event based design can automatically eliminate the
>> redundant events.
>> >
>> > Does input layer has an event type that can handle such situation?
>> > Or shall ACPI button driver handle this?
>> >
>>
>> Keys also have some redundant event elimination, but it's as long as
>> you are holding the key in the press (or released) position. So in
>> your case, that would mean sending input_key(1), wait a little while
>> other notifications are processed, and then sending input_key(0)
>> (assuming each notification comes in with its own thread). Not sure
>> you will gain anything from the new implementation you just sent with
>> the rate-limit.
>
> [Lv Zheng]
> However this key is virtual.
> The multiple notifications are just triggered by the AML code.
> The Notify(lid_device, xxx) may be invoked in a function.
> And this function may be invoked multiple times by other control methods.

Yes, I understood. In this particular case, when I sad "pressed" or
"released" please understand logical 1 or logical 0.

> So I do not know when it is "released".

Yep, that's the problem.

>
> Using the feature of the keys, it sounds like that I should setup a timer.
> When the state is changed, I should report input_key(1) and prepare the timer.
> Then report the input_key(0) when the timer times out.
> The side effect is the input_key(0) will be deferred.

That's not an issue. If it were an issue, logind can just get
triggered when it receives the key(1), and ignore the key(0). But
given that the action will likely be power suspend, I don't think even
a 500ms delay will be an issue.

>
> I just refreshed the patch as v4 with an ACPI button driver internal workaround.
> By adding a time_after() check which looks more lightweight than setting up a timer.
> Could you also help to check if that solution is OK?

Yes, that's fine by me. I already sent my rev-by.

Cheers,
Benjamin

> Thanks in advance.
>
> Best regards
> -Lv

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

[Zheng, Lv]

Hi, Dmitry

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Mon, Jul 11, 2016 at 01:34:08PM +0200, Benjamin Tissoires wrote:
> > On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> > >> Hi,
> > >>
> > >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
> wrote:
> > >> > There are many AML tables reporting wrong initial lid state, and
> some of
> > >> > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > >> > 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 <lv.zheng@intel.com>
> > >> > Cc: Bastien Nocera: <hadess@hadess.net>
> > >> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > >> > Cc: linux-input@vger.kernel.org
> > >> > ---
> > >> >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> > >> >  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-lid.txt
> > >> > 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 a
> > >> > +control method lid device. To implement this, the AML tables issue
> > >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
> state has
> > >> > +changed. The _LID control method for the lid device must be
> implemented to
> > >> > +report the "current" state of the lid as either "opened" or "closed".
> > >> > +
> > >> > +This document describes the restrictions and the expections of the
> Linux
> > >> > +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.
> > >> > +However the word of "current" has ambiguity, many AML tables
> return the lid
> > >> > +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
> > >> > +control method is evaluated during the runtime, the problem is its
> initial
> > >> > +returning value. When the AML tables implement this control
> method with
> > >> > +cached value, the initial returning value is likely not reliable. There
> are
> > >> > +simply so many examples always retuning "closed" as initial lid
> state.
> > >> > +
> > >> > +2. Restrictions of the lid state change notifications
> > >> > +
> > >> > +There are many AML tables never notifying when the lid device
> state is
> > >> > +changed to "opened". But it is ensured that the AML tables always
> notify
> > >> > +"closed" when the lid state is changed to "closed". This is normally
> used
> > >> > +to trigger some system power saving operations on Windows.
> Since it is
> > >> > +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
> > >> > +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.
> > >>
> > >> > +
> > >> > +New userspace programs should rely on the lid "closed"
> notification to
> > >> > +trigger some power saving operations and may stop taking actions
> according
> > >> > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > >> > +prepared for the new userspace to implement this ACPI control
> method lid
> > >> > +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
> > >> how to handle.
> > >>
> > >> > +
> > >> > +During the period the userspace hasn't been switched to use the
> new
> > >> > +SW_ACPI_LID event, Linux users can use the following boot
> parameter to
> > >> > +handle possible issues:
> > >> > +  button.lid_init_state=method:
> > >> > +   This is the default behavior of the Linux ACPI lid driver, Linux
> kernel
> > >> > +   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
> > >> > +   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
> > >> > +   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.
> > >
> > > How about we leave the kernel alone and userspace (which would have
> to
> > > cope with the new KEY_LID_OPEN anyway) would just have to know
> that if
> > > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
> then it
> > > can't trust the events and it needs additional heuristics.
> > >
> >
> > I really wished we could leave the kernel alone, but some platform
> > need fixes: we are using an EV_SW, and on those platform, we only get
> > the close event, which means it gets ignored by the input layer.
> 
> OK. Can we then emit missing "open" if we get "close" and the state is
> already closed?
[Lv Zheng] 
I've been considering this again.
I think you may mean we can improve SW_LID by reporting SW_LID(open) when a new close event is arrived.
So that we may be able to fix old programs.

Possibly the code should be looked like:

If (!!state != last_state && time_after(jiffies, last_jiffies)) {
	input_report_switch(..., state);
}
input_report_switch(..., !state);

However, there are tables never reporting "open".
If we do things in this way, there will be a very long period between the last close and the next close.

Which means there will be a too long period between the close and the open switch events.
IMO, this cannot fix old user space programs.

Currently, logind has a timeout mechanism.
If it cannot receive "open" within this period, it will suspend the system again right after resuming.
Thus we can still see that suspending right after resuming could be resulted.

However the new key events allow the new user space to fix the issue without changing the timeout logic.

For the old user space programs, I have no idea how we can fix them on such platforms (no open events).

Thanks and best regards
-Lv