Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1655669 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-06-01 20:50 +0200
    [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-06-01 20:50 +0200
      RE: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID  events "Zheng, Lv" <lv.zheng@intel.com> - 2017-06-05 06:30 +0200
        Re: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-06-06 12:40 +0200
    Re: [WIP PATCH 0/4] Rework the unreliable LID switch exported by  ACPI Bastien Nocera <hadess@hadess.net> - 2017-06-01 23:50 +0200
      Re: [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-06-02 09:30 +0200
    RE: [WIP PATCH 0/4] Rework the unreliable LID switch exported by  ACPI "Zheng, Lv" <lv.zheng@intel.com> - 2017-06-05 04:30 +0200
    Re: [systemd-devel] [WIP PATCH 0/4] Rework the unreliable LID switch  exported by ACPI Lennart Poettering <mzxreary@0pointer.de> - 2017-06-07 10:00 +0200

#1655669 — [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI

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

[toc] | [next] | [standalone]

#1655673 — [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events

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 <benjamin.tissoires@redhat.com>
---
 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(&device->dev);
 
-	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
-	if (ret == NOTIFY_DONE)
-		ret = blocking_notifier_call_chain(&acpi_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(&acpi_lid_notifier,
+								state,
+								lid_device);
+				if (ret == NOTIFY_DONE)
+					ret = blocking_notifier_call_chain(&acpi_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(&lid_device->dev, "Error installing input handle\n");
+		goto err_free;
+	}
+
+	error = input_open_device(handle);
+	if (error) {
+		dev_err(&lid_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 acpi_button *button;
+
+	if (!lid_device)
+		return false;
+
+	button = acpi_driver_data(lid_device);
+
+	if (dev != button->input)
+		return false;
+
+	return true;
+}
+
+static const struct input_device_id acpi_button_lid_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
+		.evbit = { BIT_MASK(EV_SW) },
+		.swbit = { BIT_MASK(SW_LID) },
+	},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(input, acpi_button_lid_ids);
+
+static struct input_handler acpi_button_lid_handler = {
+	.match		= acpi_button_lid_match,
+	.connect	= acpi_button_lid_connect,
+	.disconnect	= acpi_button_lid_disconnect,
+	.events		= acpi_button_lid_events,
+	.name		= "acpi-lid-callchain",
+	.id_table	= acpi_button_lid_ids,
+};
+
+
 static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
 {
 	struct acpi_device *device = seq->private;
@@ -581,8 +687,6 @@ static int acpi_button_add(struct acpi_device *device)
 		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
 		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
-		button->last_state = !!acpi_lid_evaluate_state(device);
-		button->last_time = ktime_get();
 	} else {
 		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
 		error = -ENODEV;
@@ -674,4 +778,22 @@ module_param_call(lid_init_state,
 		  NULL, 0644);
 MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
 
-module_acpi_driver(acpi_button_driver);
+static int __init acpi_button_init(void)
+{
+	int error;
+
+	error = input_register_handler(&acpi_button_lid_handler);
+	if (error)
+		return error;
+
+	return acpi_bus_register_driver(&acpi_button_driver);
+}
+
+static void __exit acpi_button_exit(void)
+{
+	acpi_bus_unregister_driver(&acpi_button_driver);
+	input_unregister_handler(&acpi_button_lid_handler);
+}
+
+module_init(acpi_button_init);
+module_exit(acpi_button_exit);
-- 
2.9.4

[toc] | [prev] | [next] | [standalone]

#1657260 — RE: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events

Hi, Benjamin

> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Subject: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events
> 
> 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.

See my comments to PATCH 4.
IMO, it sounds better that
1. ACPI lid works as a driver of SW_LID, and
2. i915 registers notification (the only user) via input layer.
So it looks i915 rather than button driver should call input_register_handler().
And input layer may help to provide a simplified interface for drivers to register key notifications.

Thanks and best regards
Lv

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  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(&device->dev);
> 
> -	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
> -	if (ret == NOTIFY_DONE)
> -		ret = blocking_notifier_call_chain(&acpi_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(&acpi_lid_notifier,
> +								state,
> +								lid_device);
> +				if (ret == NOTIFY_DONE)
> +					ret = blocking_notifier_call_chain(&acpi_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(&lid_device->dev, "Error installing input handle\n");
> +		goto err_free;
> +	}
> +
> +	error = input_open_device(handle);
> +	if (error) {
> +		dev_err(&lid_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 acpi_button *button;
> +
> +	if (!lid_device)
> +		return false;
> +
> +	button = acpi_driver_data(lid_device);
> +
> +	if (dev != button->input)
> +		return false;
> +
> +	return true;
> +}
> +
> +static const struct input_device_id acpi_button_lid_ids[] = {
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> +		.evbit = { BIT_MASK(EV_SW) },
> +		.swbit = { BIT_MASK(SW_LID) },
> +	},
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(input, acpi_button_lid_ids);
> +
> +static struct input_handler acpi_button_lid_handler = {
> +	.match		= acpi_button_lid_match,
> +	.connect	= acpi_button_lid_connect,
> +	.disconnect	= acpi_button_lid_disconnect,
> +	.events		= acpi_button_lid_events,
> +	.name		= "acpi-lid-callchain",
> +	.id_table	= acpi_button_lid_ids,
> +};
> +
> +
>  static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
>  {
>  	struct acpi_device *device = seq->private;
> @@ -581,8 +687,6 @@ static int acpi_button_add(struct acpi_device *device)
>  		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
>  		sprintf(class, "%s/%s",
>  			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
> -		button->last_state = !!acpi_lid_evaluate_state(device);
> -		button->last_time = ktime_get();
>  	} else {
>  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
>  		error = -ENODEV;
> @@ -674,4 +778,22 @@ module_param_call(lid_init_state,
>  		  NULL, 0644);
>  MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
> 
> -module_acpi_driver(acpi_button_driver);
> +static int __init acpi_button_init(void)
> +{
> +	int error;
> +
> +	error = input_register_handler(&acpi_button_lid_handler);
> +	if (error)
> +		return error;
> +
> +	return acpi_bus_register_driver(&acpi_button_driver);
> +}
> +
> +static void __exit acpi_button_exit(void)
> +{
> +	acpi_bus_unregister_driver(&acpi_button_driver);
> +	input_unregister_handler(&acpi_button_lid_handler);
> +}
> +
> +module_init(acpi_button_init);
> +module_exit(acpi_button_exit);
> --
> 2.9.4

[toc] | [prev] | [next] | [standalone]

#1658596 — Re: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events

On Jun 05 2017 or thereabouts, Zheng, Lv wrote:
> Hi, Benjamin
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > Subject: [WIP PATCH 3/4] ACPI: button: Let input filter out the LID events
> > 
> > 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.
> 
> See my comments to PATCH 4.
> IMO, it sounds better that
> 1. ACPI lid works as a driver of SW_LID, and
> 2. i915 registers notification (the only user) via input layer.
> So it looks i915 rather than button driver should call input_register_handler().
> And input layer may help to provide a simplified interface for drivers to register key notifications.

Sounds good.

Dmitry, would a simplified API for other drivers to listen to input
events be something you would agree on?

Cheers,
Benjamin

> 
> Thanks and best regards
> Lv
> 
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  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(&device->dev);
> > 
> > -	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
> > -	if (ret == NOTIFY_DONE)
> > -		ret = blocking_notifier_call_chain(&acpi_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(&acpi_lid_notifier,
> > +								state,
> > +								lid_device);
> > +				if (ret == NOTIFY_DONE)
> > +					ret = blocking_notifier_call_chain(&acpi_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(&lid_device->dev, "Error installing input handle\n");
> > +		goto err_free;
> > +	}
> > +
> > +	error = input_open_device(handle);
> > +	if (error) {
> > +		dev_err(&lid_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 acpi_button *button;
> > +
> > +	if (!lid_device)
> > +		return false;
> > +
> > +	button = acpi_driver_data(lid_device);
> > +
> > +	if (dev != button->input)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static const struct input_device_id acpi_button_lid_ids[] = {
> > +	{
> > +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> > +		.evbit = { BIT_MASK(EV_SW) },
> > +		.swbit = { BIT_MASK(SW_LID) },
> > +	},
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(input, acpi_button_lid_ids);
> > +
> > +static struct input_handler acpi_button_lid_handler = {
> > +	.match		= acpi_button_lid_match,
> > +	.connect	= acpi_button_lid_connect,
> > +	.disconnect	= acpi_button_lid_disconnect,
> > +	.events		= acpi_button_lid_events,
> > +	.name		= "acpi-lid-callchain",
> > +	.id_table	= acpi_button_lid_ids,
> > +};
> > +
> > +
> >  static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
> >  {
> >  	struct acpi_device *device = seq->private;
> > @@ -581,8 +687,6 @@ static int acpi_button_add(struct acpi_device *device)
> >  		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
> >  		sprintf(class, "%s/%s",
> >  			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
> > -		button->last_state = !!acpi_lid_evaluate_state(device);
> > -		button->last_time = ktime_get();
> >  	} else {
> >  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
> >  		error = -ENODEV;
> > @@ -674,4 +778,22 @@ module_param_call(lid_init_state,
> >  		  NULL, 0644);
> >  MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
> > 
> > -module_acpi_driver(acpi_button_driver);
> > +static int __init acpi_button_init(void)
> > +{
> > +	int error;
> > +
> > +	error = input_register_handler(&acpi_button_lid_handler);
> > +	if (error)
> > +		return error;
> > +
> > +	return acpi_bus_register_driver(&acpi_button_driver);
> > +}
> > +
> > +static void __exit acpi_button_exit(void)
> > +{
> > +	acpi_bus_unregister_driver(&acpi_button_driver);
> > +	input_unregister_handler(&acpi_button_lid_handler);
> > +}
> > +
> > +module_init(acpi_button_init);
> > +module_exit(acpi_button_exit);
> > --
> > 2.9.4
> 

[toc] | [prev] | [next] | [standalone]

#1655785 — Re: [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI

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

[toc] | [prev] | [next] | [standalone]

#1655963

On Jun 01 2017 or thereabouts, Bastien Nocera wrote:
> 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.

Well, the surface 3 is using a different driver for the LID switch
(surface3-wmi). From what I can remember, we don't need this patch when
using this driver (which is why I did not submitted it further). But I
might be wrong...

Cheers,
Benjamin

[toc] | [prev] | [next] | [standalone]

#1657223 — RE: [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI

Hi, Benjamin

> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Subject: [WIP PATCH 0/4] Rework the unreliable LID switch exported by ACPI
> 
> 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:

I have concerns with case 2.

> cases 1 and 3 are solved when the LID state is reliable (majority
> of existing laptops),

Agreed.

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

According to the test, both case 2,4,5 have already been solved in systemd.
So we needn't do anything in kernel.

If you still want to improve in acpi button.
IMO, for case 2, 4, we really have chance to improve.

For example, we could just add a timer right after resume.
And before it is timed out, if we can see the BIOS notify, we delete the timer.
And after it is timed out, we report "lid init value" to input layer.

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

According to the test, systemd should be ok now.
Why do we need to change it again?

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

Looks similar as the timer solution mentioned above.

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

Case 5 is not compliant to SW_LID anyway.
However it works well with latest systemd.
Maybe we should just let it be and wait for further user request.

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

IMO, if we improved case 2,4, libinput should only help to handle case 5.
Which is entirely not SW_LID compliant.

Thanks
Lv

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

[toc] | [prev] | [next] | [standalone]

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

On Thu, 01.06.17 20:46, Benjamin Tissoires (benjamin.tissoires@redhat.com) 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.

Ah nice! I (obviously) like this approach.

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

Uh if this is borked, we should indeed fix this in systemd. Is there
already a systemd github bug about this? If not, please create one,
and we'll look into it!

Thanks for working on this,

Lennart

-- 
Lennart Poettering, Red Hat

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web