[systemd-devel] [RFC PATCH v5 1/2] ACPI: button: Fix issue that button notify cannot report stateful SW_LID state

Lv Zheng lv.zheng at intel.com
Wed Jun 7 09:43:39 UTC 2017


There are platform variations implementing ACPI lid device in different
ways:
1. Some platforms send "open" events to OS and the events arrive before
   button driver is resumed;
2. Some platforms send "open" events to OS, but the events arrive after
   button driver is resumed, ex., Samsung N210+;
3. Some platforms never send "open" events to OS, but send "open" events to
   update the cached _LID return value, and the update events arrive before
   button driver is resumed;
4. Some platforms never send "open" events to OS, but send "open" events to
   update the cached _LID return value, but the update events arrive after
   button driver is resumed, ex., Surface Pro 3;
5. Some platforms never send "open" events, _LID returns value sticks to
   "close", ex., Surface Pro 1.
Currently, only case 1,3 works fine with systemd 229.

Case 2,4 can be treated as an order issue. This patch first fixes this
issue by defer sending initial lid state 10 seconds later after resume,
which ensures acpi_ec_resume() is always invoked before
acpi_button_resume().

However we can see different problems due to systemd bugs:
  systemd won't suspend right after seeing "close" event, it has a timeout,
  within the timeout, user may opens lid again. But even lid
  firmware/driver properly deliver this "open" to user space, when the
  timeout tickes, systemd still suspends the platform.
  Then user has to close/open again to wake the system up. Noticing that
  the first close event will remain in firmware, after resume, user space
  can still see a "close" followed by "open", and nothing can stop systemd
  from suspending again.
This problem can only be fixed by continously updating lid state. Thus
this patch doesn't kill the timer after seeing the BIOS notification, but
continously sending _LID return value to the input layer for
button.lid_init_state=method mode.

The users can configure update interval via button.lid_update_interval.

Cc: <systemd-devel at lists.freedesktop.org>
Cc: Benjamin Tissoires <benjamin.tissoires at redhat.com>
Cc: Peter Hutterer <peter.hutterer at who-t.net>
Signed-off-by: Lv Zheng <lv.zheng at intel.com>
---
 drivers/acpi/button.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index e19f530..fd8eff6 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -28,6 +28,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/input.h>
+#include <linux/timer.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <acpi/button.h>
@@ -79,6 +80,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
 static int acpi_button_add(struct acpi_device *device);
 static int acpi_button_remove(struct acpi_device *device);
 static void acpi_button_notify(struct acpi_device *device, u32 event);
+static void acpi_lid_timeout(ulong arg);
 
 #ifdef CONFIG_PM_SLEEP
 static int acpi_button_suspend(struct device *dev);
@@ -104,6 +106,7 @@ static struct acpi_driver acpi_button_driver = {
 struct acpi_button {
 	unsigned int type;
 	struct input_dev *input;
+	struct timer_list lid_timer;
 	char phys[32];			/* for input device */
 	unsigned long pushed;
 	int last_state;
@@ -119,6 +122,10 @@ static unsigned long lid_report_interval __read_mostly = 500;
 module_param(lid_report_interval, ulong, 0644);
 MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
 
+static unsigned long lid_update_interval __read_mostly = 10 * MSEC_PER_SEC;
+module_param(lid_update_interval, ulong, 0644);
+MODULE_PARM_DESC(lid_update_interval, "Interval (ms) between lid state updates");
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -371,17 +378,25 @@ static int acpi_lid_update_state(struct acpi_device *device)
 	return acpi_lid_notify_state(device, state);
 }
 
-static void acpi_lid_initialize_state(struct acpi_device *device)
+static void acpi_lid_tick(struct acpi_device *device)
+{
+	struct acpi_button *button = acpi_driver_data(device);
+
+	mod_timer(&button->lid_timer,
+		  jiffies + msecs_to_jiffies(lid_update_interval));
+}
+
+static void acpi_lid_timeout(ulong arg)
 {
+	struct acpi_device *device = (struct acpi_device *)arg;
+
 	switch (lid_init_state) {
 	case ACPI_BUTTON_LID_INIT_OPEN:
 		(void)acpi_lid_notify_state(device, 1);
 		break;
 	case ACPI_BUTTON_LID_INIT_METHOD:
-		(void)acpi_lid_update_state(device);
-		break;
-	case ACPI_BUTTON_LID_INIT_IGNORE:
-	default:
+		acpi_lid_update_state(device);
+		acpi_lid_tick(device);
 		break;
 	}
 }
@@ -432,6 +447,8 @@ static int acpi_button_suspend(struct device *dev)
 	struct acpi_device *device = to_acpi_device(dev);
 	struct acpi_button *button = acpi_driver_data(device);
 
+	if (button->type == ACPI_BUTTON_TYPE_LID)
+		del_timer(&button->lid_timer);
 	button->suspended = true;
 	return 0;
 }
@@ -443,7 +460,7 @@ static int acpi_button_resume(struct device *dev)
 
 	button->suspended = false;
 	if (button->type == ACPI_BUTTON_TYPE_LID)
-		acpi_lid_initialize_state(device);
+		acpi_lid_tick(device);
 	return 0;
 }
 #endif
@@ -467,6 +484,7 @@ static int acpi_button_add(struct acpi_device *device)
 		error = -ENOMEM;
 		goto err_free_button;
 	}
+	init_timer(&button->lid_timer);
 
 	name = acpi_device_name(device);
 	class = acpi_device_class(device);
@@ -490,6 +508,8 @@ static int acpi_button_add(struct acpi_device *device)
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
 		button->last_state = !!acpi_lid_evaluate_state(device);
 		button->last_time = ktime_get();
+		setup_timer(&button->lid_timer,
+			    acpi_lid_timeout, (ulong)device);
 	} else {
 		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
 		error = -ENODEV;
@@ -526,7 +546,7 @@ static int acpi_button_add(struct acpi_device *device)
 	if (error)
 		goto err_remove_fs;
 	if (button->type == ACPI_BUTTON_TYPE_LID) {
-		acpi_lid_initialize_state(device);
+		acpi_lid_tick(device);
 		/*
 		 * This assumes there's only one lid device, or if there are
 		 * more we only care about the last one...
@@ -550,6 +570,8 @@ static int acpi_button_remove(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 
+	if (button->type == ACPI_BUTTON_TYPE_LID)
+		del_timer(&button->lid_timer);
 	acpi_button_remove_fs(device);
 	input_unregister_device(button->input);
 	kfree(button);
-- 
2.7.4



More information about the systemd-devel mailing list