[systemd-devel] [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI

[Benjamin Tissoires]

Hi,

Sending this as a WIP as it still need a few changes, but it mostly works as
expected (still not fully compliant yet).

So this is based on Lennart's comment in [1]: if the LID state is not reliable,
the kernel should not export the LID switch device as long as we are not sure
about its state.

That is the basic idea, and here are some more general comments:
Lv described the 5 cases in "RFC PATCH v3" regarding the LID switch.
Let me rewrite them here (they are in patch 2):

1. Some platforms send "open" ACPI notification to the OS and the event
   arrive before the button driver is resumed;
2. Some platforms send "open" ACPI notification to the OS, but the event
   arrives after the button driver is resumed, ex., Samsung N210+;
3. Some platforms never send an "open" ACPI notification to the OS, but
   update the cached _LID return value to "open", and this update arrives
   before the button driver is resumed;
4. Some platforms never send an "open" ACPI notification to the OS, but
   update the cached _LID return value to "open", but this update arrives
   after the button driver is resumed, ex., Surface Pro 3;
5. Some platforms never send an "open" ACPI notification to the OS, and
   _LID ACPI method returns a value which stays to "close", ex.,
   Surface Pro 1.

We we consider that we can mark the LID switch as unreliable and make it
disappear when we are not certain of the state, we can consider cases 1, 2, 3
are solved: cases 1 and 3 are solved when the LID state is reliable (majority
of existing laptops), and case 2 is solved just by marking when the LID is not
reliable. When we go to sleep, we unregister the input node. We wait for
the next ACPI notification to re-export the LID switch input node with the
correct state.

Given that the "close" event is reliable, on platforms where the LID switch is
not reliable for "open", we will get the "close" event when we will start
exporting the switch at the input level.

Note that systemd currently doesn't sync the state when the input node just
appears. This is a systemd bug, and it should not be handled by the kernel
community.

For case 4, we are not aware at the acpi/button.c level when the state is valid.
We can solve this by polling every seconds for let's say 1 min, and if we detect
a change, then we can re-export the input node (this hasn't been implemented
yet). After this delay, we can consider the state as valid and export the input
node with the current reported state in the ACPI.

However, this will conflict with case 5 where the ACPI value reported by
the _LID method can be wrong anytime. We will need to treat this separately
or find some other magic to make cases 4 and 5 compatible.

libinput will help cases 4 and 5 to restore the proper state, but that's
assuming we have exported a wrong state. It might happen in case 5, but
shouldn't in case 4.

Anyway, that is just a WIP which IMO is less hacky than the few other series.
I still need to work on the udev/hwdb rules to have the list of problematic
platforms in hwdb to not have them in the kernel, but that shouldn't be much
of an issue. I also need to work on the polling but I'd like to get some inputs
from Lv, Peter and others before spending too much time on it.

Note: yes, there is a lot of boilerplate for the input handler and for the
reliable state, but I think this simplifies the logic as we are all reliying
on the input stack to filter duplicate events.
One other benefit of this boilerplate is that when libinput changes the LID
state, i915 and nouveau will get notified.

Cheers,
Benjamin


[1] https://github.com/systemd/systemd/issues/2807

Benjamin Tissoires (3):
  ACPI: button: extract input creation/destruction helpers
  ACPI: button: remove the LID input node when the state is unknown
  ACPI: button: Let input filter out the LID events

Lv Zheng (1):
  ACPI: button: Fix lid notification locks

 drivers/acpi/button.c | 453 +++---
 1 file changed, 320 insertions(+), 133 deletions(-)

-- 
2.9.4

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events

[Benjamin Tissoires]

The input stack already filters out the LID events. So instead of
filtering them out at the source, we can hook up after the input
processing and propagate the lid switch events when the input stack
tells us to.

An other benefit is that if userspace (think libinput) "fixes" the lid
switch state by some heuristics, this new state is forwarded to the
listeners in the kernel.

Signed-off-by: Benjamin Tissoires 
---
 drivers/acpi/button.c | 156 --
 1 file changed, 139 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9ad7604..03e5981 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -109,8 +109,6 @@ struct acpi_button {
struct input_dev *input;
char phys[32];  /* for input device */
unsigned long pushed;
-   int last_state;
-   ktime_t last_time;
bool suspended;
 };
 
@@ -184,7 +182,6 @@ static int acpi_lid_evaluate_state(struct acpi_device 
*device)
 static int acpi_lid_notify_state(struct acpi_device *device, int state)
 {
struct acpi_button *button = acpi_driver_data(device);
-   int ret;
 
/* button_input_lock must be held */
 
@@ -205,20 +202,129 @@ static int acpi_lid_notify_state(struct acpi_device 
*device, int state)
if (state)
pm_wakeup_hard_event(>dev);
 
-   ret = blocking_notifier_call_chain(_lid_notifier, state, device);
-   if (ret == NOTIFY_DONE)
-   ret = blocking_notifier_call_chain(_lid_notifier, state,
-  device);
-   if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
-   /*
-* It is also regarded as success if the notifier_chain
-* returns NOTIFY_OK or NOTIFY_DONE.
-*/
-   ret = 0;
+   return 0;
+}
+
+/*
+ * Pass incoming event to all connected clients.
+ */
+static void acpi_button_lid_events(struct input_handle *handle,
+  const struct input_value *vals,
+  unsigned int count)
+{
+   const struct input_value *v;
+   int state = -1;
+   int ret;
+
+   for (v = vals; v != vals + count; v++) {
+   switch (v->type) {
+   case EV_SYN:
+   if (v->code == SYN_REPORT && state >= 0) {
+   ret = 
blocking_notifier_call_chain(_lid_notifier,
+   state,
+   lid_device);
+   if (ret == NOTIFY_DONE)
+   ret = 
blocking_notifier_call_chain(_lid_notifier,
+   state,
+   lid_device);
+   if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
+   /*
+* It is also regarded as success if
+* the notifier_chain returns NOTIFY_OK
+* or NOTIFY_DONE.
+*/
+   ret = 0;
+   }
+   }
+   break;
+   case EV_SW:
+   if (v->code == SW_LID)
+   state = !v->value;
+   break;
+   }
}
-   return ret;
 }
 
+static int acpi_button_lid_connect(struct input_handler *handler,
+  struct input_dev *dev,
+  const struct input_device_id *id)
+{
+   struct input_handle *handle;
+   int error;
+
+   handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
+   if (!handle)
+   return -ENOMEM;
+
+   handle->dev = dev;
+   handle->handler = handler;
+   handle->name = "acpi-button-lid";
+
+   error = input_register_handle(handle);
+   if (error) {
+   dev_err(_device->dev, "Error installing input handle\n");
+   goto err_free;
+   }
+
+   error = input_open_device(handle);
+   if (error) {
+   dev_err(_device->dev, "Failed to open input device\n");
+   goto err_unregister;
+   }
+
+   return 0;
+
+ err_unregister:
+   input_unregister_handle(handle);
+ err_free:
+   kfree(handle);
+   return error;
+}
+
+static void acpi_button_lid_disconnect(struct input_handle *handle)
+{
+   input_close_device(handle);
+   input_unregister_handle(handle);
+   kfree(handle);
+}
+
+bool acpi_button_lid_match(struct input_handler *handler,
+  struct input_dev *dev)
+{
+   struct 

[systemd-devel] [WIP PATCH 4/4] ACPI: button: Fix lid notification locks

[Benjamin Tissoires]

From: Lv Zheng 

acpi/button.c now contains the logic to avoid frequently replayed events
which originally was ensured by using blocking notifier.
On the contrary, using a blocking notifier is wrong as it could keep on
returning NOTIFY_DONE, causing events lost.

This patch thus changes lid notification to raw notifier in order not to
have any events lost.

Signed-off-by: Lv Zheng 
Signed-off-by: Benjamin Tissoires 
---
 drivers/acpi/button.c | 68 ---
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 03e5981..1927b08 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -114,7 +114,7 @@ struct acpi_button {
 
 static DEFINE_MUTEX(button_input_lock);
 
-static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
+static RAW_NOTIFIER_HEAD(acpi_lid_notifier);
 static struct acpi_device *lid_device;
 static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
 
@@ -179,14 +179,12 @@ 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 void acpi_lid_notify_state(struct acpi_device *device, int state)
 {
struct acpi_button *button = acpi_driver_data(device);
 
-   /* button_input_lock must be held */
-
if (!button->input)
-   return 0;
+   return;
 
/*
 * If the lid is unreliable, always send an "open" event before any
@@ -201,8 +199,6 @@ static int acpi_lid_notify_state(struct acpi_device 
*device, int state)
 
if (state)
pm_wakeup_hard_event(>dev);
-
-   return 0;
 }
 
 /*
@@ -214,28 +210,14 @@ static void acpi_button_lid_events(struct input_handle 
*handle,
 {
const struct input_value *v;
int state = -1;
-   int ret;
 
for (v = vals; v != vals + count; v++) {
switch (v->type) {
case EV_SYN:
-   if (v->code == SYN_REPORT && state >= 0) {
-   ret = 
blocking_notifier_call_chain(_lid_notifier,
+   if (v->code == SYN_REPORT && state >= 0)
+   
(void)raw_notifier_call_chain(_lid_notifier,
state,
lid_device);
-   if (ret == NOTIFY_DONE)
-   ret = 
blocking_notifier_call_chain(_lid_notifier,
-   state,
-   lid_device);
-   if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
-   /*
-* It is also regarded as success if
-* the notifier_chain returns NOTIFY_OK
-* or NOTIFY_DONE.
-*/
-   ret = 0;
-   }
-   }
break;
case EV_SW:
if (v->code == SW_LID)
@@ -433,13 +415,25 @@ static int acpi_button_remove_fs(struct acpi_device 
*device)
-- 
*/
 int acpi_lid_notifier_register(struct notifier_block *nb)
 {
-   return blocking_notifier_chain_register(_lid_notifier, nb);
+   return raw_notifier_chain_register(_lid_notifier, nb);
 }
 EXPORT_SYMBOL(acpi_lid_notifier_register);
 
+static inline int __acpi_lid_notifier_unregister(struct notifier_block *nb,
+bool sync)
+{
+   int ret;
+
+   ret = raw_notifier_chain_unregister(_lid_notifier, nb);
+   if (sync)
+   synchronize_rcu();
+
+   return ret;
+}
+
 int acpi_lid_notifier_unregister(struct notifier_block *nb)
 {
-   return blocking_notifier_chain_unregister(_lid_notifier, nb);
+   return __acpi_lid_notifier_unregister(nb, false);
 }
 EXPORT_SYMBOL(acpi_lid_notifier_unregister);
 
@@ -452,40 +446,36 @@ int acpi_lid_open(void)
 }
 EXPORT_SYMBOL(acpi_lid_open);
 
-static int acpi_lid_update_state(struct acpi_device *device)
+static void acpi_lid_update_state(struct acpi_device *device)
 {
int state;
 
state = acpi_lid_evaluate_state(device);
if (state < 0)
-   return state;
+   return;
 
-   return acpi_lid_notify_state(device, state);
+   acpi_lid_notify_state(device, state);
 }
 
-static int acpi_lid_notify(struct acpi_device *device)
+static void acpi_lid_notify(struct acpi_device *device)
 {
struct acpi_button *button = 

[systemd-devel] [WIP PATCH 2/4] ACPI: button: remove the LID input node when the state is unknown

[Benjamin Tissoires]

Because of the variation of firmware implementation, there is a chance
the LID state is unknown:
1. Some platforms send "open" ACPI notification to the OS and the event
   arrive before the button driver is resumed;
2. Some platforms send "open" ACPI notification to the OS, but the event
   arrives after the button driver is resumed, ex., Samsung N210+;
3. Some platforms never send an "open" ACPI notification to the OS, but
   update the cached _LID return value to "open", and this update arrives
   before the button driver is resumed;
4. Some platforms never send an "open" ACPI notification to the OS, but
   update the cached _LID return value to "open", but this update arrives
   after the button driver is resumed, ex., Surface Pro 3;
5. Some platforms never send an "open" ACPI notification to the OS, and
   _LID ACPI method returns a value which stays to "close", ex.,
   Surface Pro 1.

We can mark the unreliable platform (cases 2, 4, 5 above) as such and make
sure we do not export an input node with an unknown state to prevent
suspend loops.

The database of unreliable devices is left to userspace to handle with
a hwdb file and a udev rule.

Note that this patch removes the filtering of duplicate events when
calling blocking_notifier_call_chain(), but this will be addressed in
a following patch.

Signed-off-by: Benjamin Tissoires 
---
 drivers/acpi/button.c | 207 --
 1 file changed, 131 insertions(+), 76 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 48bcdca..9ad7604 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -79,6 +80,8 @@ 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 int acpi_button_add_input(struct acpi_device *device);
+static int acpi_lid_update_reliable(struct acpi_device *device);
 
 #ifdef CONFIG_PM_SLEEP
 static int acpi_button_suspend(struct device *dev);
@@ -111,6 +114,8 @@ struct acpi_button {
bool suspended;
 };
 
+static DEFINE_MUTEX(button_input_lock);
+
 static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
 static struct acpi_device *lid_device;
 static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
@@ -119,6 +124,44 @@ 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 bool lid_reliable = true;
+
+static int param_set_lid_reliable(const char *val,
+ const struct kernel_param *kp)
+{
+   bool prev_lid_reliable = lid_reliable;
+   int ret;
+
+   mutex_lock(_input_lock);
+
+   ret = param_set_bool(val, kp);
+   if (ret) {
+   mutex_unlock(_input_lock);
+   return ret;
+   }
+
+   /*
+* prevent a loop when we show up the device to userspace because
+* of an acpi notification, and userspace immediately removes it
+* by marking it as unreliable when this was already known.
+*/
+   if (lid_device && prev_lid_reliable != lid_reliable) {
+   ret = acpi_lid_update_reliable(lid_device);
+   if (ret)
+   lid_reliable = prev_lid_reliable;
+   }
+
+   mutex_unlock(_input_lock);
+   return ret;
+}
+
+static const struct kernel_param_ops lid_reliable_ops = {
+   .get = param_get_bool,
+   .set = param_set_lid_reliable,
+};
+module_param_cb(lid_reliable, _reliable_ops, _reliable, 0644);
+MODULE_PARM_DESC(lid_reliable, "Is the LID switch reliable (true|false)?");
+
 /* --
   FS Interface (/proc)
-- 
*/
@@ -142,79 +185,22 @@ static int acpi_lid_notify_state(struct acpi_device 
*device, int state)
 {
struct acpi_button *button = acpi_driver_data(device);
int ret;
-   ktime_t next_report;
-   bool do_update;
+
+   /* button_input_lock must be held */
+
+   if (!button->input)
+   return 0;
 
/*
-* In lid_init_state=ignore mode, if user opens/closes lid
-* frequently with "open" missing, and "last_time" is also updated
-* frequently, "close" cannot be delivered to the userspace.
-* So "last_time" is only updated after a timeout or an actual
-* switch.
+* If the lid is unreliable, always send an "open" event before any
+* other. The input layer will filter out the extra open if required,
+* and it will force the close event to be sent.
 */
-   if 

[systemd-devel] [WIP PATCH 1/4] ACPI: button: extract input creation/destruction helpers

[Benjamin Tissoires]

When the LID switch ACPI implementation is unreliable, we might want
to remove the device when we are not sure about the state. This should
prevent any suspend/resume loops given that in that case, there will be
no more LID switch input node.

This patch prepares the dynamic creation/destruction of the input node.

Signed-off-by: Benjamin Tissoires 
---
 drivers/acpi/button.c | 90 ---
 1 file changed, 57 insertions(+), 33 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9ad8cdb..48bcdca 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -448,10 +448,61 @@ static int acpi_button_resume(struct device *dev)
 }
 #endif
 
+static void acpi_button_remove_input(struct acpi_device *device)
+{
+   struct acpi_button *button = acpi_driver_data(device);
+
+   input_unregister_device(button->input);
+   button->input = NULL;
+}
+
+static int acpi_button_add_input(struct acpi_device *device)
+{
+   struct acpi_button *button = acpi_driver_data(device);
+   struct input_dev *input;
+   int error;
+
+   button->input = input = input_allocate_device();
+   if (!input) {
+   error = -ENOMEM;
+   goto err;
+   }
+
+   input->name = acpi_device_name(device);
+   input->phys = button->phys;
+   input->id.bustype = BUS_HOST;
+   input->id.product = button->type;
+   input->dev.parent = >dev;
+
+   switch (button->type) {
+   case ACPI_BUTTON_TYPE_POWER:
+   input_set_capability(input, EV_KEY, KEY_POWER);
+   break;
+
+   case ACPI_BUTTON_TYPE_SLEEP:
+   input_set_capability(input, EV_KEY, KEY_SLEEP);
+   break;
+
+   case ACPI_BUTTON_TYPE_LID:
+   input_set_capability(input, EV_SW, SW_LID);
+   break;
+   }
+
+   error = input_register_device(input);
+   if (error)
+   goto err;
+
+   return 0;
+
+ err:
+   input_free_device(input);
+   button->input = NULL;
+   return error;
+}
+
 static int acpi_button_add(struct acpi_device *device)
 {
struct acpi_button *button;
-   struct input_dev *input;
const char *hid = acpi_device_hid(device);
char *name, *class;
int error;
@@ -462,12 +513,6 @@ static int acpi_button_add(struct acpi_device *device)
 
device->driver_data = button;
 
-   button->input = input = input_allocate_device();
-   if (!input) {
-   error = -ENOMEM;
-   goto err_free_button;
-   }
-
name = acpi_device_name(device);
class = acpi_device_class(device);
 
@@ -493,38 +538,19 @@ static int acpi_button_add(struct acpi_device *device)
} else {
printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
error = -ENODEV;
-   goto err_free_input;
+   goto err_free_button;
}
 
error = acpi_button_add_fs(device);
if (error)
-   goto err_free_input;
+   goto err_free_button;
 
snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid);
 
-   input->name = name;
-   input->phys = button->phys;
-   input->id.bustype = BUS_HOST;
-   input->id.product = button->type;
-   input->dev.parent = >dev;
-
-   switch (button->type) {
-   case ACPI_BUTTON_TYPE_POWER:
-   input_set_capability(input, EV_KEY, KEY_POWER);
-   break;
-
-   case ACPI_BUTTON_TYPE_SLEEP:
-   input_set_capability(input, EV_KEY, KEY_SLEEP);
-   break;
-
-   case ACPI_BUTTON_TYPE_LID:
-   input_set_capability(input, EV_SW, SW_LID);
-   break;
-   }
-
-   error = input_register_device(input);
+   error = acpi_button_add_input(device);
if (error)
goto err_remove_fs;
+
if (button->type == ACPI_BUTTON_TYPE_LID) {
acpi_lid_initialize_state(device);
/*
@@ -540,8 +566,6 @@ static int acpi_button_add(struct acpi_device *device)
 
  err_remove_fs:
acpi_button_remove_fs(device);
- err_free_input:
-   input_free_device(input);
  err_free_button:
kfree(button);
return error;
@@ -552,7 +576,7 @@ static int acpi_button_remove(struct acpi_device *device)
struct acpi_button *button = acpi_driver_data(device);
 
acpi_button_remove_fs(device);
-   input_unregister_device(button->input);
+   acpi_button_remove_input(device);
kfree(button);
return 0;
 }
-- 
2.9.4

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI

[Bastien Nocera]

On Thu, 2017-06-01 at 20:46 +0200, Benjamin Tissoires wrote:
> Hi,
> 
> Sending this as a WIP as it still need a few changes, but it mostly
> works as
> expected (still not fully compliant yet).
> 
> So this is based on Lennart's comment in [1]: if the LID state is not
> reliable,
> the kernel should not export the LID switch device as long as we are
> not sure
> about its state.
> 
> That is the basic idea, and here are some more general comments:
> Lv described the 5 cases in "RFC PATCH v3" regarding the LID switch.
> Let me rewrite them here (they are in patch 2):
> 
> 1. Some platforms send "open" ACPI notification to the OS and the
> event
>arrive before the button driver is resumed;
> 2. Some platforms send "open" ACPI notification to the OS, but the
> event
>arrives after the button driver is resumed, ex., Samsung N210+;
> 3. Some platforms never send an "open" ACPI notification to the OS,
> but
>update the cached _LID return value to "open", and this update
> arrives
>before the button driver is resumed;
> 4. Some platforms never send an "open" ACPI notification to the OS,
> but
>update the cached _LID return value to "open", but this update
> arrives
>after the button driver is resumed, ex., Surface Pro 3;
> 5. Some platforms never send an "open" ACPI notification to the OS,
> and
>_LID ACPI method returns a value which stays to "close", ex.,
>Surface Pro 1.

In which case does the Surface 3 lie? I believe we still needed your
"gpiolib-acpi: make sure we trigger the events at least once" patch to
make that one work.

Cheers
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel