All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
@ 2016-02-03 11:04 Chen Yu
  2016-02-15  3:34   ` Zheng, Lv
  0 siblings, 1 reply; 11+ messages in thread
From: Chen Yu @ 2016-02-03 11:04 UTC (permalink / raw)
  To: linux-acpi; +Cc: linux-kernel, rjw, lenb, rui.zhang, bugzilla, cwhuang, Chen Yu

Some platforms such as Surface 3, Surface Pro 1 have broken _LID
that, either _LID returns 'closed' during bootup, or _LID fails
to return the up-to-date lid state to OSPM. This is because
that, on these platforms _LID is implemented by returning a
local variable, which can only be updated by lid events:

Device (LID)
{
	Name (LIDB, Zero)
	Method (_LID, 0, NotSerialized)
	{
		Return (LIDB)
	}
}

Method (_E4C, 0, Serialized)
{
	If ((HELD == One))
	{
		^^LID.LIDB = One
	}
	Else
	{
		^^LID.LIDB = Zero
		Notify (LID, 0x80)
	}
}

After the lid is closed, _E4C updates the LIDB to zero, then system
falls asleep, however when lid is opened, there would be no interrupt
triggered due to hardware design, we have to wake the system up by
pressing power button, as a result, LIDB remains zero after resumed,
thus _LID returns 'close' to systemd daemon(or does not return any
value to systemd), as a result the system suspends again even though
we do nothing.

This patch is to provide a possible workaround for these broken
platforms, by introducing a 'cached' lid state, which is not
based on evaluating _LID, but based on maintaining the lid
state in a event-driven manner:

1. lid cache state is assigned to 'open' explicitly during boot up.
2. lid cache state can only be changed during suspend/resume, or someone
   notifies the lid device.
3. always return lid cache state instead of _LID to sysfs.

Linked: https://bugzilla.kernel.org/show_bug.cgi?id=89211
Reported-and-tested-by: GiH <gih@maier.one>
Reported-by: David J. Goehrig <dave@dloh.org>
Reported-by: Stephen Just <stephenjust@gmail.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/acpi/button.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 5c3b091..ec2a027 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -28,6 +28,7 @@
 #include <linux/input.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <acpi/button.h>
 
 #define PREFIX "ACPI: "
@@ -53,6 +54,9 @@
 #define ACPI_BUTTON_DEVICE_NAME_LID	"Lid Switch"
 #define ACPI_BUTTON_TYPE_LID		0x05
 
+#define ACPI_LID_CACHE_OPEN			1
+#define ACPI_LID_CACHE_CLOSE			0
+
 #define _COMPONENT		ACPI_BUTTON_COMPONENT
 ACPI_MODULE_NAME("button");
 
@@ -101,8 +105,10 @@ struct acpi_button {
 	char phys[32];			/* for input device */
 	unsigned long pushed;
 	bool suspended;
+	unsigned long long cache_state;
 };
 
+static int use_lid_cache_state;
 static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
 static struct acpi_device *lid_device;
 
@@ -118,8 +124,13 @@ static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
 	struct acpi_device *device = seq->private;
 	acpi_status status;
 	unsigned long long state;
+	struct acpi_button *button = acpi_driver_data(device);
 
 	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
+
+	if (use_lid_cache_state)
+		state = button->cache_state;
+
 	seq_printf(seq, "state:      %s\n",
 		   ACPI_FAILURE(status) ? "unsupported" :
 			(state ? "open" : "closed"));
@@ -233,15 +244,23 @@ int acpi_lid_open(void)
 {
 	acpi_status status;
 	unsigned long long state;
+	struct acpi_button *button;
 
 	if (!lid_device)
 		return -ENODEV;
 
+	button = acpi_driver_data(lid_device);
+	if (!button)
+		return -ENODEV;
+
 	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
 				       &state);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
+	if (use_lid_cache_state)
+		state = button->cache_state;
+
 	return !!state;
 }
 EXPORT_SYMBOL(acpi_lid_open);
@@ -257,6 +276,9 @@ static int acpi_lid_send_state(struct acpi_device *device)
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
+	if (use_lid_cache_state)
+		state = button->cache_state;
+
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
@@ -290,6 +312,8 @@ 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) {
+			if (use_lid_cache_state)
+				button->cache_state = !button->cache_state;
 			acpi_lid_send_state(device);
 		} else {
 			int keycode;
@@ -325,6 +349,9 @@ static int acpi_button_suspend(struct device *dev)
 	struct acpi_button *button = acpi_driver_data(device);
 
 	button->suspended = true;
+	if (use_lid_cache_state)
+		button->cache_state = ACPI_LID_CACHE_CLOSE;
+
 	return 0;
 }
 
@@ -334,12 +361,41 @@ static int acpi_button_resume(struct device *dev)
 	struct acpi_button *button = acpi_driver_data(device);
 
 	button->suspended = false;
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (button->type == ACPI_BUTTON_TYPE_LID) {
+		if (use_lid_cache_state)
+			button->cache_state = ACPI_LID_CACHE_OPEN;
 		return acpi_lid_send_state(device);
+	}
 	return 0;
 }
 #endif
 
+static int switch_lid_mode(const struct dmi_system_id *d)
+{
+	use_lid_cache_state = 1;
+	return 0;
+}
+
+static struct dmi_system_id broken_lid_dmi_table[] = {
+	{
+	 .callback = switch_lid_mode,
+	 .ident = "Surface Pro 1",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Surface with Windows 8 Pro"),
+		},
+	},
+	{
+	 .callback = switch_lid_mode,
+	 .ident = "Surface 3",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
+		},
+	},
+	{}
+};
+
 static int acpi_button_add(struct acpi_device *device)
 {
 	struct acpi_button *button;
@@ -380,6 +436,7 @@ 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);
+		dmi_check_system(broken_lid_dmi_table);
 	} else {
 		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
 		error = -ENODEV;
@@ -416,6 +473,8 @@ static int acpi_button_add(struct acpi_device *device)
 	if (error)
 		goto err_remove_fs;
 	if (button->type == ACPI_BUTTON_TYPE_LID) {
+		if (use_lid_cache_state)
+			button->cache_state = ACPI_LID_CACHE_OPEN;
 		acpi_lid_send_state(device);
 		/*
 		 * This assumes there's only one lid device, or if there are
-- 
1.8.4.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-02-03 11:04 [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet Chen Yu
@ 2016-02-15  3:34   ` Zheng, Lv
  0 siblings, 0 replies; 11+ messages in thread
From: Zheng, Lv @ 2016-02-15  3:34 UTC (permalink / raw)
  To: linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, bugzilla, cwhuang, Chen, Yu C

Hi, Yu

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Chen Yu
> Subject: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
> 
[Lv Zheng] 
The word 'broken' is not proper.
This just seems like an indication of a Linux gap.
Possibly just:
1. losses of GPEs;
2. lacking in the correct init value of lid driver state; and
3. lacking in the sense of lid capability

> Some platforms such as Surface 3, Surface Pro 1 have broken _LID
[Lv Zheng] 
Ditto, 'broken' is not correct.

> that, either _LID returns 'closed' during bootup, or _LID fails
> to return the up-to-date lid state to OSPM. This is because
> that, on these platforms _LID is implemented by returning a
> local variable, which can only be updated by lid events:
> 
> Device (LID)
> {
> 	Name (LIDB, Zero)
> 	Method (_LID, 0, NotSerialized)
> 	{
> 		Return (LIDB)
> 	}
> }
> 
> Method (_E4C, 0, Serialized)
> {
> 	If ((HELD == One))
> 	{
> 		^^LID.LIDB = One
> 	}
> 	Else
> 	{
> 		^^LID.LIDB = Zero
> 		Notify (LID, 0x80)
> 	}
> }
> 
> After the lid is closed, _E4C updates the LIDB to zero, then system
> falls asleep, however when lid is opened, there would be no interrupt
> triggered due to hardware design, we have to wake the system up by
> pressing power button, as a result, LIDB remains zero after resumed,
> thus _LID returns 'close' to systemd daemon(or does not return any
> value to systemd), as a result the system suspends again even though
> we do nothing.
> 
> This patch is to provide a possible workaround for these broken
> platforms, by introducing a 'cached' lid state, which is not
> based on evaluating _LID, but based on maintaining the lid
> state in a event-driven manner:
> 
> 1. lid cache state is assigned to 'open' explicitly during boot up.
> 2. lid cache state can only be changed during suspend/resume, or someone
>    notifies the lid device.
> 3. always return lid cache state instead of _LID to sysfs.
[Lv Zheng] 
According to my understanding, returning _LID isn't wrong.

In fact, some platforms rely on this behavior.
I know for sure there is a platform:
1. Only generates LID open notification, and doesn't generate LID close notification; and
2. _LID return correct state as it includes several EC accesses to obtain correct hardware status.
Changing this behavior could break such platform while it is apparently working by returning _LID from sysfs.

OTOH, if BIOS lid state has been correctly updated according to the notification, _LID is ensured to be correct by BIOS.

So could you stop changing this behavior?
I can sense regressions around this change.

> 
> Linked: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> Reported-and-tested-by: GiH <gih@maier.one>
> Reported-by: David J. Goehrig <dave@dloh.org>
> Reported-by: Stephen Just <stephenjust@gmail.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  drivers/acpi/button.c | 61
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 5c3b091..ec2a027 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -28,6 +28,7 @@
>  #include <linux/input.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <acpi/button.h>
> 
>  #define PREFIX "ACPI: "
> @@ -53,6 +54,9 @@
>  #define ACPI_BUTTON_DEVICE_NAME_LID	"Lid Switch"
>  #define ACPI_BUTTON_TYPE_LID		0x05
> 
> +#define ACPI_LID_CACHE_OPEN			1
> +#define ACPI_LID_CACHE_CLOSE			0
> +
>  #define _COMPONENT		ACPI_BUTTON_COMPONENT
>  ACPI_MODULE_NAME("button");
> 
> @@ -101,8 +105,10 @@ struct acpi_button {
>  	char phys[32];			/* for input device */
>  	unsigned long pushed;
>  	bool suspended;
> +	unsigned long long cache_state;
>  };
> 
> +static int use_lid_cache_state;
>  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
> 
> @@ -118,8 +124,13 @@ static int acpi_button_state_seq_show(struct seq_file
> *seq, void *offset)
>  	struct acpi_device *device = seq->private;
>  	acpi_status status;
>  	unsigned long long state;
> +	struct acpi_button *button = acpi_driver_data(device);
> 
>  	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
> +
> +	if (use_lid_cache_state)
> +		state = button->cache_state;
> +
>  	seq_printf(seq, "state:      %s\n",
>  		   ACPI_FAILURE(status) ? "unsupported" :
>  			(state ? "open" : "closed"));
> @@ -233,15 +244,23 @@ int acpi_lid_open(void)
>  {
>  	acpi_status status;
>  	unsigned long long state;
> +	struct acpi_button *button;
> 
>  	if (!lid_device)
>  		return -ENODEV;
> 
> +	button = acpi_driver_data(lid_device);
> +	if (!button)
> +		return -ENODEV;
> +
>  	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
>  				       &state);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> 
> +	if (use_lid_cache_state)
> +		state = button->cache_state;
> +
>  	return !!state;
>  }
>  EXPORT_SYMBOL(acpi_lid_open);
> @@ -257,6 +276,9 @@ static int acpi_lid_send_state(struct acpi_device
> *device)
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> 
> +	if (use_lid_cache_state)
> +		state = button->cache_state;
> +
>  	/* input layer checks if event is redundant */
>  	input_report_switch(button->input, SW_LID, !state);
>  	input_sync(button->input);
> @@ -290,6 +312,8 @@ 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) {
> +			if (use_lid_cache_state)
> +				button->cache_state = !button->cache_state;
>  			acpi_lid_send_state(device);
>  		} else {
>  			int keycode;
> @@ -325,6 +349,9 @@ static int acpi_button_suspend(struct device *dev)
>  	struct acpi_button *button = acpi_driver_data(device);
> 
>  	button->suspended = true;
> +	if (use_lid_cache_state)
> +		button->cache_state = ACPI_LID_CACHE_CLOSE;
> +
>  	return 0;
>  }
> 
> @@ -334,12 +361,41 @@ static int acpi_button_resume(struct device *dev)
>  	struct acpi_button *button = acpi_driver_data(device);
> 
>  	button->suspended = false;
> -	if (button->type == ACPI_BUTTON_TYPE_LID)
> +	if (button->type == ACPI_BUTTON_TYPE_LID) {
> +		if (use_lid_cache_state)
> +			button->cache_state = ACPI_LID_CACHE_OPEN;
>  		return acpi_lid_send_state(device);
> +	}
>  	return 0;
>  }
>  #endif
> 
> +static int switch_lid_mode(const struct dmi_system_id *d)
> +{
> +	use_lid_cache_state = 1;
> +	return 0;
> +}
> +
> +static struct dmi_system_id broken_lid_dmi_table[] = {
[Lv Zheng] 
'lid_dmi_table' could be better.
It is not a good idea to maintain quirks for 'broken' BIOS in the kernel source tree.
Quirks should be prepared by the kernel to work around issues that the kernel hasn't enough knowledge/isn't willing to handle.
For BIOS quirks, it is better to just provide boot parameters for the distribution vendors.

> +	{
> +	 .callback = switch_lid_mode,
> +	 .ident = "Surface Pro 1",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface with Windows 8
> Pro"),
> +		},
> +	},
> +	{
> +	 .callback = switch_lid_mode,
> +	 .ident = "Surface 3",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> +		},
> +	},
> +	{}
> +};
> +
[Lv Zheng] 
They are all "connected standby" platforms.
And we can see there will be more and more such platforms manufactured from then on.
In order not to increase this quirk table unlimitedly, could you try to fix the 'GPE loss on s2i' issues for such kind of platforms?

Thanks and best regards
-Lv

>  static int acpi_button_add(struct acpi_device *device)
>  {
>  	struct acpi_button *button;
> @@ -380,6 +436,7 @@ 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);
> +		dmi_check_system(broken_lid_dmi_table);
>  	} else {
>  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
>  		error = -ENODEV;
> @@ -416,6 +473,8 @@ static int acpi_button_add(struct acpi_device *device)
>  	if (error)
>  		goto err_remove_fs;
>  	if (button->type == ACPI_BUTTON_TYPE_LID) {
> +		if (use_lid_cache_state)
> +			button->cache_state = ACPI_LID_CACHE_OPEN;
>  		acpi_lid_send_state(device);
>  		/*
>  		 * This assumes there's only one lid device, or if there are
> --
> 1.8.4.2
> 
> --
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
@ 2016-02-15  3:34   ` Zheng, Lv
  0 siblings, 0 replies; 11+ messages in thread
From: Zheng, Lv @ 2016-02-15  3:34 UTC (permalink / raw)
  To: Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, bugzilla, cwhuang, Chen, Yu C

Hi, Yu

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Chen Yu
> Subject: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
> 
[Lv Zheng] 
The word 'broken' is not proper.
This just seems like an indication of a Linux gap.
Possibly just:
1. losses of GPEs;
2. lacking in the correct init value of lid driver state; and
3. lacking in the sense of lid capability

> Some platforms such as Surface 3, Surface Pro 1 have broken _LID
[Lv Zheng] 
Ditto, 'broken' is not correct.

> that, either _LID returns 'closed' during bootup, or _LID fails
> to return the up-to-date lid state to OSPM. This is because
> that, on these platforms _LID is implemented by returning a
> local variable, which can only be updated by lid events:
> 
> Device (LID)
> {
> 	Name (LIDB, Zero)
> 	Method (_LID, 0, NotSerialized)
> 	{
> 		Return (LIDB)
> 	}
> }
> 
> Method (_E4C, 0, Serialized)
> {
> 	If ((HELD == One))
> 	{
> 		^^LID.LIDB = One
> 	}
> 	Else
> 	{
> 		^^LID.LIDB = Zero
> 		Notify (LID, 0x80)
> 	}
> }
> 
> After the lid is closed, _E4C updates the LIDB to zero, then system
> falls asleep, however when lid is opened, there would be no interrupt
> triggered due to hardware design, we have to wake the system up by
> pressing power button, as a result, LIDB remains zero after resumed,
> thus _LID returns 'close' to systemd daemon(or does not return any
> value to systemd), as a result the system suspends again even though
> we do nothing.
> 
> This patch is to provide a possible workaround for these broken
> platforms, by introducing a 'cached' lid state, which is not
> based on evaluating _LID, but based on maintaining the lid
> state in a event-driven manner:
> 
> 1. lid cache state is assigned to 'open' explicitly during boot up.
> 2. lid cache state can only be changed during suspend/resume, or someone
>    notifies the lid device.
> 3. always return lid cache state instead of _LID to sysfs.
[Lv Zheng] 
According to my understanding, returning _LID isn't wrong.

In fact, some platforms rely on this behavior.
I know for sure there is a platform:
1. Only generates LID open notification, and doesn't generate LID close notification; and
2. _LID return correct state as it includes several EC accesses to obtain correct hardware status.
Changing this behavior could break such platform while it is apparently working by returning _LID from sysfs.

OTOH, if BIOS lid state has been correctly updated according to the notification, _LID is ensured to be correct by BIOS.

So could you stop changing this behavior?
I can sense regressions around this change.

> 
> Linked: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> Reported-and-tested-by: GiH <gih@maier.one>
> Reported-by: David J. Goehrig <dave@dloh.org>
> Reported-by: Stephen Just <stephenjust@gmail.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  drivers/acpi/button.c | 61
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 5c3b091..ec2a027 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -28,6 +28,7 @@
>  #include <linux/input.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <acpi/button.h>
> 
>  #define PREFIX "ACPI: "
> @@ -53,6 +54,9 @@
>  #define ACPI_BUTTON_DEVICE_NAME_LID	"Lid Switch"
>  #define ACPI_BUTTON_TYPE_LID		0x05
> 
> +#define ACPI_LID_CACHE_OPEN			1
> +#define ACPI_LID_CACHE_CLOSE			0
> +
>  #define _COMPONENT		ACPI_BUTTON_COMPONENT
>  ACPI_MODULE_NAME("button");
> 
> @@ -101,8 +105,10 @@ struct acpi_button {
>  	char phys[32];			/* for input device */
>  	unsigned long pushed;
>  	bool suspended;
> +	unsigned long long cache_state;
>  };
> 
> +static int use_lid_cache_state;
>  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
> 
> @@ -118,8 +124,13 @@ static int acpi_button_state_seq_show(struct seq_file
> *seq, void *offset)
>  	struct acpi_device *device = seq->private;
>  	acpi_status status;
>  	unsigned long long state;
> +	struct acpi_button *button = acpi_driver_data(device);
> 
>  	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
> +
> +	if (use_lid_cache_state)
> +		state = button->cache_state;
> +
>  	seq_printf(seq, "state:      %s\n",
>  		   ACPI_FAILURE(status) ? "unsupported" :
>  			(state ? "open" : "closed"));
> @@ -233,15 +244,23 @@ int acpi_lid_open(void)
>  {
>  	acpi_status status;
>  	unsigned long long state;
> +	struct acpi_button *button;
> 
>  	if (!lid_device)
>  		return -ENODEV;
> 
> +	button = acpi_driver_data(lid_device);
> +	if (!button)
> +		return -ENODEV;
> +
>  	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
>  				       &state);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> 
> +	if (use_lid_cache_state)
> +		state = button->cache_state;
> +
>  	return !!state;
>  }
>  EXPORT_SYMBOL(acpi_lid_open);
> @@ -257,6 +276,9 @@ static int acpi_lid_send_state(struct acpi_device
> *device)
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> 
> +	if (use_lid_cache_state)
> +		state = button->cache_state;
> +
>  	/* input layer checks if event is redundant */
>  	input_report_switch(button->input, SW_LID, !state);
>  	input_sync(button->input);
> @@ -290,6 +312,8 @@ 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) {
> +			if (use_lid_cache_state)
> +				button->cache_state = !button->cache_state;
>  			acpi_lid_send_state(device);
>  		} else {
>  			int keycode;
> @@ -325,6 +349,9 @@ static int acpi_button_suspend(struct device *dev)
>  	struct acpi_button *button = acpi_driver_data(device);
> 
>  	button->suspended = true;
> +	if (use_lid_cache_state)
> +		button->cache_state = ACPI_LID_CACHE_CLOSE;
> +
>  	return 0;
>  }
> 
> @@ -334,12 +361,41 @@ static int acpi_button_resume(struct device *dev)
>  	struct acpi_button *button = acpi_driver_data(device);
> 
>  	button->suspended = false;
> -	if (button->type == ACPI_BUTTON_TYPE_LID)
> +	if (button->type == ACPI_BUTTON_TYPE_LID) {
> +		if (use_lid_cache_state)
> +			button->cache_state = ACPI_LID_CACHE_OPEN;
>  		return acpi_lid_send_state(device);
> +	}
>  	return 0;
>  }
>  #endif
> 
> +static int switch_lid_mode(const struct dmi_system_id *d)
> +{
> +	use_lid_cache_state = 1;
> +	return 0;
> +}
> +
> +static struct dmi_system_id broken_lid_dmi_table[] = {
[Lv Zheng] 
'lid_dmi_table' could be better.
It is not a good idea to maintain quirks for 'broken' BIOS in the kernel source tree.
Quirks should be prepared by the kernel to work around issues that the kernel hasn't enough knowledge/isn't willing to handle.
For BIOS quirks, it is better to just provide boot parameters for the distribution vendors.

> +	{
> +	 .callback = switch_lid_mode,
> +	 .ident = "Surface Pro 1",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface with Windows 8
> Pro"),
> +		},
> +	},
> +	{
> +	 .callback = switch_lid_mode,
> +	 .ident = "Surface 3",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> +		},
> +	},
> +	{}
> +};
> +
[Lv Zheng] 
They are all "connected standby" platforms.
And we can see there will be more and more such platforms manufactured from then on.
In order not to increase this quirk table unlimitedly, could you try to fix the 'GPE loss on s2i' issues for such kind of platforms?

Thanks and best regards
-Lv

>  static int acpi_button_add(struct acpi_device *device)
>  {
>  	struct acpi_button *button;
> @@ -380,6 +436,7 @@ 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);
> +		dmi_check_system(broken_lid_dmi_table);
>  	} else {
>  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
>  		error = -ENODEV;
> @@ -416,6 +473,8 @@ static int acpi_button_add(struct acpi_device *device)
>  	if (error)
>  		goto err_remove_fs;
>  	if (button->type == ACPI_BUTTON_TYPE_LID) {
> +		if (use_lid_cache_state)
> +			button->cache_state = ACPI_LID_CACHE_OPEN;
>  		acpi_lid_send_state(device);
>  		/*
>  		 * This assumes there's only one lid device, or if there are
> --
> 1.8.4.2
> 
> --
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-02-15  3:34   ` Zheng, Lv
  (?)
@ 2016-02-15  5:39   ` Chen, Yu C
  2016-02-22  7:24     ` Zheng, Lv
  -1 siblings, 1 reply; 11+ messages in thread
From: Chen, Yu C @ 2016-02-15  5:39 UTC (permalink / raw)
  To: Zheng, Lv, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, bugzilla, cwhuang

Hi Lv,

> -----Original Message-----
> From: Zheng, Lv
> Sent: Monday, February 15, 2016 11:34 AM
> To: Chen, Yu C; linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org;
> Zhang, Rui; bugzilla@hadess.net; cwhuang@android-x86.org; Chen, Yu C
> Subject: RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface
> tablet
> 
> Hi, Yu
> 
> > From: linux-acpi-owner@vger.kernel.org
> > [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Chen Yu
> > Subject: [PATCH] ACPI / button: Avoid using broken _LID on Surface
> > tablet
> >
> [Lv Zheng]
> The word 'broken' is not proper.
> This just seems like an indication of a Linux gap.
> Possibly just:
> 1. losses of GPEs;
> 2. lacking in the correct init value of lid driver state; and 3. lacking in the sense
> of lid capability
[Yu] I agree, but the major problem here is that, there is no SCI triggered when opening the LID,
and since this problem exist on Windows too(can not wake system up by open lid), 
it looks like either a BIOS problem(does not get the right lid status
by EC eccess) or a hardware issue, I'm not sure if Linux is aware of the open event from lid. 
And even if Linux does not send the lid status after resume(althought wrong status: close) to input layer/netlink,
the daemon systemd would also suspend   the system after 20 seconds(and there is no SCI during 20s), so
it seems systemd(or other) daemon is using a timeout framework, so the quickest way to fix this problem
is to send the 'correct' lid status to daemon after resume, I know this is ugly.. 
> 
> > Some platforms such as Surface 3, Surface Pro 1 have broken _LID
> [Lv Zheng]
> Ditto, 'broken' is not correct.
> 
> > that, either _LID returns 'closed' during bootup, or _LID fails to
> > return the up-to-date lid state to OSPM. This is because that, on
> > these platforms _LID is implemented by returning a local variable,
> > which can only be updated by lid events:
> >
> > Device (LID)
> > {
> > 	Name (LIDB, Zero)
> > 	Method (_LID, 0, NotSerialized)
> > 	{
> > 		Return (LIDB)
> > 	}
> > }
> >
> > Method (_E4C, 0, Serialized)
> > {
> > 	If ((HELD == One))
> > 	{
> > 		^^LID.LIDB = One
> > 	}
> > 	Else
> > 	{
> > 		^^LID.LIDB = Zero
> > 		Notify (LID, 0x80)
> > 	}
> > }
> >
> > After the lid is closed, _E4C updates the LIDB to zero, then system
> > falls asleep, however when lid is opened, there would be no interrupt
> > triggered due to hardware design, we have to wake the system up by
> > pressing power button, as a result, LIDB remains zero after resumed,
> > thus _LID returns 'close' to systemd daemon(or does not return any
> > value to systemd), as a result the system suspends again even though
> > we do nothing.
> >
> > This patch is to provide a possible workaround for these broken
> > platforms, by introducing a 'cached' lid state, which is not based on
> > evaluating _LID, but based on maintaining the lid state in a
> > event-driven manner:
> >
> > 1. lid cache state is assigned to 'open' explicitly during boot up.
> > 2. lid cache state can only be changed during suspend/resume, or someone
> >    notifies the lid device.
> > 3. always return lid cache state instead of _LID to sysfs.
> [Lv Zheng]
> According to my understanding, returning _LID isn't wrong.
> 
> In fact, some platforms rely on this behavior.
> I know for sure there is a platform:
> 1. Only generates LID open notification, and doesn't generate LID close
> notification; and 2. _LID return correct state as it includes several EC accesses
> to obtain correct hardware status.
> Changing this behavior could break such platform while it is apparently
> working by returning _LID from sysfs.
> 
> OTOH, if BIOS lid state has been correctly updated according to the
> notification, _LID is ensured to be correct by BIOS.
> 
> So could you stop changing this behavior?
> I can sense regressions around this change.
> 
> >
> > Linked: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> > Reported-and-tested-by: GiH <gih@maier.one>
> > Reported-by: David J. Goehrig <dave@dloh.org>
> > Reported-by: Stephen Just <stephenjust@gmail.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  drivers/acpi/button.c | 61
> > ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index
> > 5c3b091..ec2a027 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/input.h>
> >  #include <linux/slab.h>
> >  #include <linux/acpi.h>
> > +#include <linux/dmi.h>
> >  #include <acpi/button.h>
> >
> >  #define PREFIX "ACPI: "
> > @@ -53,6 +54,9 @@
> >  #define ACPI_BUTTON_DEVICE_NAME_LID	"Lid Switch"
> >  #define ACPI_BUTTON_TYPE_LID		0x05
> >
> > +#define ACPI_LID_CACHE_OPEN			1
> > +#define ACPI_LID_CACHE_CLOSE			0
> > +
> >  #define _COMPONENT		ACPI_BUTTON_COMPONENT
> >  ACPI_MODULE_NAME("button");
> >
> > @@ -101,8 +105,10 @@ struct acpi_button {
> >  	char phys[32];			/* for input device */
> >  	unsigned long pushed;
> >  	bool suspended;
> > +	unsigned long long cache_state;
> >  };
> >
> > +static int use_lid_cache_state;
> >  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> >  static struct acpi_device *lid_device;
> >
> > @@ -118,8 +124,13 @@ static int acpi_button_state_seq_show(struct
> > seq_file *seq, void *offset)
> >  	struct acpi_device *device = seq->private;
> >  	acpi_status status;
> >  	unsigned long long state;
> > +	struct acpi_button *button = acpi_driver_data(device);
> >
> >  	status = acpi_evaluate_integer(device->handle, "_LID", NULL,
> > &state);
> > +
> > +	if (use_lid_cache_state)
> > +		state = button->cache_state;
> > +
> >  	seq_printf(seq, "state:      %s\n",
> >  		   ACPI_FAILURE(status) ? "unsupported" :
> >  			(state ? "open" : "closed"));
> > @@ -233,15 +244,23 @@ int acpi_lid_open(void)  {
> >  	acpi_status status;
> >  	unsigned long long state;
> > +	struct acpi_button *button;
> >
> >  	if (!lid_device)
> >  		return -ENODEV;
> >
> > +	button = acpi_driver_data(lid_device);
> > +	if (!button)
> > +		return -ENODEV;
> > +
> >  	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
> >  				       &state);
> >  	if (ACPI_FAILURE(status))
> >  		return -ENODEV;
> >
> > +	if (use_lid_cache_state)
> > +		state = button->cache_state;
> > +
> >  	return !!state;
> >  }
> >  EXPORT_SYMBOL(acpi_lid_open);
> > @@ -257,6 +276,9 @@ static int acpi_lid_send_state(struct acpi_device
> > *device)
> >  	if (ACPI_FAILURE(status))
> >  		return -ENODEV;
> >
> > +	if (use_lid_cache_state)
> > +		state = button->cache_state;
> > +
> >  	/* input layer checks if event is redundant */
> >  	input_report_switch(button->input, SW_LID, !state);
> >  	input_sync(button->input);
> > @@ -290,6 +312,8 @@ 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) {
> > +			if (use_lid_cache_state)
> > +				button->cache_state = !button->cache_state;
> >  			acpi_lid_send_state(device);
> >  		} else {
> >  			int keycode;
> > @@ -325,6 +349,9 @@ static int acpi_button_suspend(struct device *dev)
> >  	struct acpi_button *button = acpi_driver_data(device);
> >
> >  	button->suspended = true;
> > +	if (use_lid_cache_state)
> > +		button->cache_state = ACPI_LID_CACHE_CLOSE;
> > +
> >  	return 0;
> >  }
> >
> > @@ -334,12 +361,41 @@ static int acpi_button_resume(struct device *dev)
> >  	struct acpi_button *button = acpi_driver_data(device);
> >
> >  	button->suspended = false;
> > -	if (button->type == ACPI_BUTTON_TYPE_LID)
> > +	if (button->type == ACPI_BUTTON_TYPE_LID) {
> > +		if (use_lid_cache_state)
> > +			button->cache_state = ACPI_LID_CACHE_OPEN;
> >  		return acpi_lid_send_state(device);
> > +	}
> >  	return 0;
> >  }
> >  #endif
> >
> > +static int switch_lid_mode(const struct dmi_system_id *d) {
> > +	use_lid_cache_state = 1;
> > +	return 0;
> > +}
> > +
> > +static struct dmi_system_id broken_lid_dmi_table[] = {
> [Lv Zheng]
> 'lid_dmi_table' could be better.
> It is not a good idea to maintain quirks for 'broken' BIOS in the kernel source
> tree.
> Quirks should be prepared by the kernel to work around issues that the
> kernel hasn't enough knowledge/isn't willing to handle.
> For BIOS quirks, it is better to just provide boot parameters for the
> distribution vendors.
[Yu] Yes, I used to provide a boot parameter as you suggested before, but users
gave feedback they'd like to fix this problem transparently.. 
> 
> > +	{
> > +	 .callback = switch_lid_mode,
> > +	 .ident = "Surface Pro 1",
> > +	 .matches = {
> > +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface with
> Windows 8
> > Pro"),
> > +		},
> > +	},
> > +	{
> > +	 .callback = switch_lid_mode,
> > +	 .ident = "Surface 3",
> > +	 .matches = {
> > +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> > +		},
> > +	},
> > +	{}
> > +};
> > +
> [Lv Zheng]
> They are all "connected standby" platforms.
> And we can see there will be more and more such platforms manufactured
> from then on.
> In order not to increase this quirk table unlimitedly, could you try to fix the
> 'GPE loss on s2i' issues for such kind of platforms?
[Yu] This might be a hardware issue that cause the loss of GPE, there is no SCi triggered
when open the lid.

Thanks,
yu
> 
> Thanks and best regards
> -Lv
> 
> >  static int acpi_button_add(struct acpi_device *device)  {
> >  	struct acpi_button *button;
> > @@ -380,6 +436,7 @@ 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);
> > +		dmi_check_system(broken_lid_dmi_table);
> >  	} else {
> >  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
> >  		error = -ENODEV;
> > @@ -416,6 +473,8 @@ static int acpi_button_add(struct acpi_device
> *device)
> >  	if (error)
> >  		goto err_remove_fs;
> >  	if (button->type == ACPI_BUTTON_TYPE_LID) {
> > +		if (use_lid_cache_state)
> > +			button->cache_state = ACPI_LID_CACHE_OPEN;
> >  		acpi_lid_send_state(device);
> >  		/*
> >  		 * This assumes there's only one lid device, or if there are
> > --
> > 1.8.4.2
> >
> > --
> > 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-02-15  5:39   ` Chen, Yu C
@ 2016-02-22  7:24     ` Zheng, Lv
  2016-02-22 14:14         ` Bastien Nocera
  0 siblings, 1 reply; 11+ messages in thread
From: Zheng, Lv @ 2016-02-22  7:24 UTC (permalink / raw)
  To: Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, bugzilla, cwhuang

Hi, Yu

> From: Chen, Yu C
> Subject: RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
> 
> Hi Lv,
> 
> > From: Zheng, Lv
> > Subject: RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface
> > tablet
> >
> > Hi, Yu
> >
> > > From: linux-acpi-owner@vger.kernel.org
> > > [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Chen Yu
> > > Subject: [PATCH] ACPI / button: Avoid using broken _LID on Surface
> > > tablet
> > >
> > [Lv Zheng]
> > The word 'broken' is not proper.
> > This just seems like an indication of a Linux gap.
> > Possibly just:
> > 1. losses of GPEs;
> > 2. lacking in the correct init value of lid driver state; and 3. lacking in the
> sense
> > of lid capability
> [Yu] I agree, but the major problem here is that, there is no SCI triggered when
> opening the LID,
> and since this problem exist on Windows too(can not wake system up by open
> lid),
> it looks like either a BIOS problem(does not get the right lid status
> by EC eccess) or a hardware issue, I'm not sure if Linux is aware of the open
> event from lid.
> And even if Linux does not send the lid status after resume(althought wrong
> status: close) to input layer/netlink,
> the daemon systemd would also suspend   the system after 20 seconds(and
> there is no SCI during 20s), so
> it seems systemd(or other) daemon is using a timeout framework, so the
> quickest way to fix this problem
> is to send the 'correct' lid status to daemon after resume, I know this is ugly..
> >
> > > Some platforms such as Surface 3, Surface Pro 1 have broken _LID
> > [Lv Zheng]
> > Ditto, 'broken' is not correct.
> >
> > > that, either _LID returns 'closed' during bootup, or _LID fails to
> > > return the up-to-date lid state to OSPM. This is because that, on
> > > these platforms _LID is implemented by returning a local variable,
> > > which can only be updated by lid events:
> > >
> > > Device (LID)
> > > {
> > > 	Name (LIDB, Zero)
> > > 	Method (_LID, 0, NotSerialized)
> > > 	{
> > > 		Return (LIDB)
> > > 	}
> > > }
> > >
> > > Method (_E4C, 0, Serialized)
> > > {
> > > 	If ((HELD == One))
> > > 	{
> > > 		^^LID.LIDB = One
> > > 	}
> > > 	Else
> > > 	{
> > > 		^^LID.LIDB = Zero
> > > 		Notify (LID, 0x80)
> > > 	}
> > > }
> > >
> > > After the lid is closed, _E4C updates the LIDB to zero, then system
> > > falls asleep, however when lid is opened, there would be no interrupt
> > > triggered due to hardware design, we have to wake the system up by
> > > pressing power button, as a result, LIDB remains zero after resumed,
> > > thus _LID returns 'close' to systemd daemon(or does not return any
> > > value to systemd), as a result the system suspends again even though
> > > we do nothing.
> > >
> > > This patch is to provide a possible workaround for these broken
> > > platforms, by introducing a 'cached' lid state, which is not based on
> > > evaluating _LID, but based on maintaining the lid state in a
> > > event-driven manner:
> > >
> > > 1. lid cache state is assigned to 'open' explicitly during boot up.
> > > 2. lid cache state can only be changed during suspend/resume, or someone
> > >    notifies the lid device.
> > > 3. always return lid cache state instead of _LID to sysfs.
> > [Lv Zheng]
> > According to my understanding, returning _LID isn't wrong.
> >
> > In fact, some platforms rely on this behavior.
> > I know for sure there is a platform:
> > 1. Only generates LID open notification, and doesn't generate LID close
> > notification; and 2. _LID return correct state as it includes several EC accesses
> > to obtain correct hardware status.
> > Changing this behavior could break such platform while it is apparently
> > working by returning _LID from sysfs.
> >
> > OTOH, if BIOS lid state has been correctly updated according to the
> > notification, _LID is ensured to be correct by BIOS.
> >
> > So could you stop changing this behavior?
> > I can sense regressions around this change.
> >
> > >
> > > Linked: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> > > Reported-and-tested-by: GiH <gih@maier.one>
> > > Reported-by: David J. Goehrig <dave@dloh.org>
> > > Reported-by: Stephen Just <stephenjust@gmail.com>
> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > > ---
> > >  drivers/acpi/button.c | 61
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 60 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index
> > > 5c3b091..ec2a027 100644
> > > --- a/drivers/acpi/button.c
> > > +++ b/drivers/acpi/button.c
> > > @@ -28,6 +28,7 @@
> > >  #include <linux/input.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/dmi.h>
> > >  #include <acpi/button.h>
> > >
> > >  #define PREFIX "ACPI: "
> > > @@ -53,6 +54,9 @@
> > >  #define ACPI_BUTTON_DEVICE_NAME_LID	"Lid Switch"
> > >  #define ACPI_BUTTON_TYPE_LID		0x05
> > >
> > > +#define ACPI_LID_CACHE_OPEN			1
> > > +#define ACPI_LID_CACHE_CLOSE			0
> > > +
> > >  #define _COMPONENT		ACPI_BUTTON_COMPONENT
> > >  ACPI_MODULE_NAME("button");
> > >
> > > @@ -101,8 +105,10 @@ struct acpi_button {
> > >  	char phys[32];			/* for input device */
> > >  	unsigned long pushed;
> > >  	bool suspended;
> > > +	unsigned long long cache_state;
> > >  };
> > >
> > > +static int use_lid_cache_state;
> > >  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> > >  static struct acpi_device *lid_device;
> > >
> > > @@ -118,8 +124,13 @@ static int acpi_button_state_seq_show(struct
> > > seq_file *seq, void *offset)
> > >  	struct acpi_device *device = seq->private;
> > >  	acpi_status status;
> > >  	unsigned long long state;
> > > +	struct acpi_button *button = acpi_driver_data(device);
> > >
> > >  	status = acpi_evaluate_integer(device->handle, "_LID", NULL,
> > > &state);
> > > +
> > > +	if (use_lid_cache_state)
> > > +		state = button->cache_state;
> > > +
> > >  	seq_printf(seq, "state:      %s\n",
> > >  		   ACPI_FAILURE(status) ? "unsupported" :
> > >  			(state ? "open" : "closed"));
> > > @@ -233,15 +244,23 @@ int acpi_lid_open(void)  {
> > >  	acpi_status status;
> > >  	unsigned long long state;
> > > +	struct acpi_button *button;
> > >
> > >  	if (!lid_device)
> > >  		return -ENODEV;
> > >
> > > +	button = acpi_driver_data(lid_device);
> > > +	if (!button)
> > > +		return -ENODEV;
> > > +
> > >  	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
> > >  				       &state);
> > >  	if (ACPI_FAILURE(status))
> > >  		return -ENODEV;
> > >
> > > +	if (use_lid_cache_state)
> > > +		state = button->cache_state;
> > > +
> > >  	return !!state;
> > >  }
> > >  EXPORT_SYMBOL(acpi_lid_open);
> > > @@ -257,6 +276,9 @@ static int acpi_lid_send_state(struct acpi_device
> > > *device)
> > >  	if (ACPI_FAILURE(status))
> > >  		return -ENODEV;
> > >
> > > +	if (use_lid_cache_state)
> > > +		state = button->cache_state;
> > > +
> > >  	/* input layer checks if event is redundant */
> > >  	input_report_switch(button->input, SW_LID, !state);
> > >  	input_sync(button->input);
> > > @@ -290,6 +312,8 @@ 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) {
> > > +			if (use_lid_cache_state)
> > > +				button->cache_state = !button->cache_state;
> > >  			acpi_lid_send_state(device);
> > >  		} else {
> > >  			int keycode;
> > > @@ -325,6 +349,9 @@ static int acpi_button_suspend(struct device *dev)
> > >  	struct acpi_button *button = acpi_driver_data(device);
> > >
> > >  	button->suspended = true;
> > > +	if (use_lid_cache_state)
> > > +		button->cache_state = ACPI_LID_CACHE_CLOSE;
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -334,12 +361,41 @@ static int acpi_button_resume(struct device *dev)
> > >  	struct acpi_button *button = acpi_driver_data(device);
> > >
> > >  	button->suspended = false;
> > > -	if (button->type == ACPI_BUTTON_TYPE_LID)
> > > +	if (button->type == ACPI_BUTTON_TYPE_LID) {
> > > +		if (use_lid_cache_state)
> > > +			button->cache_state = ACPI_LID_CACHE_OPEN;
> > >  		return acpi_lid_send_state(device);
> > > +	}
> > >  	return 0;
> > >  }
> > >  #endif
> > >
> > > +static int switch_lid_mode(const struct dmi_system_id *d) {
> > > +	use_lid_cache_state = 1;
> > > +	return 0;
> > > +}
> > > +
> > > +static struct dmi_system_id broken_lid_dmi_table[] = {
> > [Lv Zheng]
> > 'lid_dmi_table' could be better.
> > It is not a good idea to maintain quirks for 'broken' BIOS in the kernel source
> > tree.
> > Quirks should be prepared by the kernel to work around issues that the
> > kernel hasn't enough knowledge/isn't willing to handle.
> > For BIOS quirks, it is better to just provide boot parameters for the
> > distribution vendors.
> [Yu] Yes, I used to provide a boot parameter as you suggested before, but users
> gave feedback they'd like to fix this problem transparently..
[Lv Zheng] 
According to the knowledge of various BIOS _LID implementation, Linux button driver doesn't seem to be working wrong.
It in fact works correctly and is able to handle all BIOS _LID implementations.

So this looks to me like a user space bug.
That the user space doesn't contain a proper mode to handle ACPI BIOS _LID implementations.

Why should kernel work this gap around?
The gap is:
The user space requires lid device to act in the way it wants.
While the ACPI BIOS only provides lid behavior that is working for Windows power-saving settings.
IMO,
1. Windows Only requires LID close notifications to trigger power-save action and only evaluates _LID after receiving a LID notification,
    BIOSen only ensure _LID returning value is correct after a notification.
2. But Linux user space requires all LID notifications to be instantly/correctly reported and wants to know the exact LID states whenever it reads the states from the sysfs.
It doesn't seem to be possible for the kernel to work between BIOSen and the user space to fill such a gap.

Thanks and best regards
-Lv

> >
> > > +	{
> > > +	 .callback = switch_lid_mode,
> > > +	 .ident = "Surface Pro 1",
> > > +	 .matches = {
> > > +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface with
> > Windows 8
> > > Pro"),
> > > +		},
> > > +	},
> > > +	{
> > > +	 .callback = switch_lid_mode,
> > > +	 .ident = "Surface 3",
> > > +	 .matches = {
> > > +		DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> > > +		DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
> > > +		},
> > > +	},
> > > +	{}
> > > +};
> > > +
> > [Lv Zheng]
> > They are all "connected standby" platforms.
> > And we can see there will be more and more such platforms manufactured
> > from then on.
> > In order not to increase this quirk table unlimitedly, could you try to fix the
> > 'GPE loss on s2i' issues for such kind of platforms?
> [Yu] This might be a hardware issue that cause the loss of GPE, there is no SCi
> triggered
> when open the lid.
> 
> Thanks,
> yu
> >
> > Thanks and best regards
> > -Lv
> >
> > >  static int acpi_button_add(struct acpi_device *device)  {
> > >  	struct acpi_button *button;
> > > @@ -380,6 +436,7 @@ 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);
> > > +		dmi_check_system(broken_lid_dmi_table);
> > >  	} else {
> > >  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
> > >  		error = -ENODEV;
> > > @@ -416,6 +473,8 @@ static int acpi_button_add(struct acpi_device
> > *device)
> > >  	if (error)
> > >  		goto err_remove_fs;
> > >  	if (button->type == ACPI_BUTTON_TYPE_LID) {
> > > +		if (use_lid_cache_state)
> > > +			button->cache_state = ACPI_LID_CACHE_OPEN;
> > >  		acpi_lid_send_state(device);
> > >  		/*
> > >  		 * This assumes there's only one lid device, or if there are
> > > --
> > > 1.8.4.2
> > >
> > > --
> > > 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-02-22  7:24     ` Zheng, Lv
@ 2016-02-22 14:14         ` Bastien Nocera
  0 siblings, 0 replies; 11+ messages in thread
From: Bastien Nocera @ 2016-02-22 14:14 UTC (permalink / raw)
  To: Zheng, Lv, Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, cwhuang

On Mon, 2016-02-22 at 07:24 +0000, Zheng, Lv wrote:
> [Lv Zheng] 
> According to the knowledge of various BIOS _LID implementation, Linux
> button driver doesn't seem to be working wrong.
> It in fact works correctly and is able to handle all BIOS _LID
> implementations.
> 
> So this looks to me like a user space bug.

I don't think it is one, given the kernel API for that functionality.
If, instead of exposing the lid as an input switch, the kernel only
ever sent an event saying "the lid is now closed" then we wouldn't have
that problem, as we'd likely expect the lid to be opened when starting
the machine (if present).

> That the user space doesn't contain a proper mode to handle ACPI BIOS
> _LID implementations.
> 
> Why should kernel work this gap around?
> The gap is:
> The user space requires lid device to act in the way it wants.

In the way that it's always worked up until now, rather.

> While the ACPI BIOS only provides lid behavior that is working for
> Windows power-saving settings.
> IMO,
> 1. Windows Only requires LID close notifications to trigger power-
> save action and only evaluates _LID after receiving a LID
> notification,
>     BIOSen only ensure _LID returning value is correct after a
> notification.

In which case, expecting the lid to always be opened on startup would
probably be a fair assumption, no?

> 2. But Linux user space requires all LID notifications to be
> instantly/correctly reported and wants to know the exact LID states
> whenever it reads the states from the sysfs.
> It doesn't seem to be possible for the kernel to work between BIOSen
> and the user space to fill such a gap.

If you quirk the kernel lid handling to always be opened on startup,
and we waited until the first event to correct it if necessary, seems
like the easiest way to go.

But that brings me the question of how Windows (and then Linux) behave
when you've booted your laptop and closed the lid straight away, before
any driver in the OS had the chance to be around to see the
notification?

It also brings the question of how Windows will know that the lid is
still closed when coming out of suspend by pressing the power button,
which can happen depending on the hardware design (it's certainly
possible to press the power button with the lid closed on the Surface
3, and likely other Surfaces).

I'm not happy about the "fix" for this problem either, but blaming
user-space for this is harsh, given the API exported by the kernel and
how pretty much every laptop worked.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
@ 2016-02-22 14:14         ` Bastien Nocera
  0 siblings, 0 replies; 11+ messages in thread
From: Bastien Nocera @ 2016-02-22 14:14 UTC (permalink / raw)
  To: Zheng, Lv, Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, cwhuang

On Mon, 2016-02-22 at 07:24 +0000, Zheng, Lv wrote:
> [Lv Zheng] 
> According to the knowledge of various BIOS _LID implementation, Linux
> button driver doesn't seem to be working wrong.
> It in fact works correctly and is able to handle all BIOS _LID
> implementations.
> 
> So this looks to me like a user space bug.

I don't think it is one, given the kernel API for that functionality.
If, instead of exposing the lid as an input switch, the kernel only
ever sent an event saying "the lid is now closed" then we wouldn't have
that problem, as we'd likely expect the lid to be opened when starting
the machine (if present).

> That the user space doesn't contain a proper mode to handle ACPI BIOS
> _LID implementations.
> 
> Why should kernel work this gap around?
> The gap is:
> The user space requires lid device to act in the way it wants.

In the way that it's always worked up until now, rather.

> While the ACPI BIOS only provides lid behavior that is working for
> Windows power-saving settings.
> IMO,
> 1. Windows Only requires LID close notifications to trigger power-
> save action and only evaluates _LID after receiving a LID
> notification,
>     BIOSen only ensure _LID returning value is correct after a
> notification.

In which case, expecting the lid to always be opened on startup would
probably be a fair assumption, no?

> 2. But Linux user space requires all LID notifications to be
> instantly/correctly reported and wants to know the exact LID states
> whenever it reads the states from the sysfs.
> It doesn't seem to be possible for the kernel to work between BIOSen
> and the user space to fill such a gap.

If you quirk the kernel lid handling to always be opened on startup,
and we waited until the first event to correct it if necessary, seems
like the easiest way to go.

But that brings me the question of how Windows (and then Linux) behave
when you've booted your laptop and closed the lid straight away, before
any driver in the OS had the chance to be around to see the
notification?

It also brings the question of how Windows will know that the lid is
still closed when coming out of suspend by pressing the power button,
which can happen depending on the hardware design (it's certainly
possible to press the power button with the lid closed on the Surface
3, and likely other Surfaces).

I'm not happy about the "fix" for this problem either, but blaming
user-space for this is harsh, given the API exported by the kernel and
how pretty much every laptop worked.

Cheers

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-02-22 14:14         ` Bastien Nocera
  (?)
@ 2016-03-04  3:37         ` Zheng, Lv
  2016-03-07 16:19             ` Bastien Nocera
  -1 siblings, 1 reply; 11+ messages in thread
From: Zheng, Lv @ 2016-03-04  3:37 UTC (permalink / raw)
  To: Bastien Nocera, Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, cwhuang

Hi,

> From: Bastien Nocera [mailto:hadess@hadess.net]
> Subject: Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
> 
> On Mon, 2016-02-22 at 07:24 +0000, Zheng, Lv wrote:
> > [Lv Zheng]
> > According to the knowledge of various BIOS _LID implementation, Linux
> > button driver doesn't seem to be working wrong.
> > It in fact works correctly and is able to handle all BIOS _LID
> > implementations.
> >
> > So this looks to me like a user space bug.
> 
> I don't think it is one, given the kernel API for that functionality.
> If, instead of exposing the lid as an input switch, the kernel only
> ever sent an event saying "the lid is now closed" then we wouldn't have
> that problem, as we'd likely expect the lid to be opened when starting
> the machine (if present).
[Lv Zheng] 
Let me show you several proofs of Windows behaviors.

1. Have you ever seen a screen on Windows that can tell you the status of LID.
If you have seen such screen, what the status is when the system is booted.
I have never seen such a UI, so Windows in fact cares no current LID status.

2. Have you ever seen a screen on Windows that allow users to specify open LID actions?
The answer is no.
On traditional x86 platforms, system is woken up via wake GPEs.
So the open event in fact is handled by BIOS and Windows cares nothing about it.

3. Have you ever seen a screen on Windows that allow users to specify close LID actions?
Yes, I have, it is in power options.
So Windows in fact only cares about the LID close notification and will only check LID status when it receives this notification.

The above facts can make clear about what BIOS will implement for the ACPI LID.

And from this point of view, in kernel ACPI LID driver, only this commit is wrong:
commit 23de5d9ef2a4bbc4f733f58311bcb7cf6239c813
Author: Alexey Starikovskiy <astarikovskiy@suse.de>
Subject: ACPI: button: send initial lid state after add and resume
This seems to be a false root caused bug fix:
https://bugzilla.novell.com/show_bug.cgi?id=326814
I guess the original bug was in the EC driver or somewhere else.

But to our surprises, after reverting this commit.
We can see:
1. Kernel doesn't trigger any event to the userspace.
2. But we still can see that Surface is automatically suspended every several seconds.

According to the Bugzilla investigation, the suspend was triggered by systemd.
I thus just concluded that this could be a userspace bug.
But maybe this is a systemd behavior triggered by some kernel events that are not captured by our debugging patches.
Let the reporter and the developer do more investigation there.

I just raised my concern about this fix.
The fix brought by this patch is apparently wrong according to the above conclusions.

> 
> > That the user space doesn't contain a proper mode to handle ACPI BIOS
> > _LID implementations.
> >
> > Why should kernel work this gap around?
> > The gap is:
> > The user space requires lid device to act in the way it wants.
> 
> In the way that it's always worked up until now, rather.
[Lv Zheng] 
This is not persuasive.
Things can appear to be working with workarounds applied.
During my career, I have seen plenty of such cases.
And I‘ve proven that there are many such kind of wrongly root caused fixes in ACPI subsystem in this community during the past years.

> 
> > While the ACPI BIOS only provides lid behavior that is working for
> > Windows power-saving settings.
> > IMO,
> > 1. Windows Only requires LID close notifications to trigger power-
> > save action and only evaluates _LID after receiving a LID
> > notification,
> >     BIOSen only ensure _LID returning value is correct after a
> > notification.
> 
> In which case, expecting the lid to always be opened on startup would
> probably be a fair assumption, no?
> 
[Lv Zheng] 
The answer should be no here.

If we make system default LID status as open on boot/resume, then what about this case?

According to our tests, we can reboot/resume surface with LID closed into a running Windows.
So if we force the LID status to be opened after the reboot/resume, then this also seems to be a conflict against the real status.
What the benefit to force such a wrong status, given that we are all not obsessives...

> > 2. But Linux user space requires all LID notifications to be
> > instantly/correctly reported and wants to know the exact LID states
> > whenever it reads the states from the sysfs.
> > It doesn't seem to be possible for the kernel to work between BIOSen
> > and the user space to fill such a gap.
> 
> If you quirk the kernel lid handling to always be opened on startup,
> and we waited until the first event to correct it if necessary, seems
> like the easiest way to go.
> 
> But that brings me the question of how Windows (and then Linux) behave
> when you've booted your laptop and closed the lid straight away, before
> any driver in the OS had the chance to be around to see the
> notification?
> 
[Lv Zheng] 
If we booted the laptop with LID opened, and close it right before the Windows is fully booted.
We can see the laptop booted into a running Windows.

> It also brings the question of how Windows will know that the lid is
> still closed when coming out of suspend by pressing the power button,
> which can happen depending on the hardware design (it's certainly
> possible to press the power button with the lid closed on the Surface
> 3, and likely other Surfaces).
> 
[Lv Zheng] 
If we booted/resumed the laptop with LID closed by pressing the power button.
We can see the laptop booted/resumed into a running Windows.

> I'm not happy about the "fix" for this problem either, but blaming
> user-space for this is harsh, given the API exported by the kernel and
> how pretty much every laptop worked.
> 
[Lv Zheng] 
It worked for so many years on top of traditional x86 laptops where the LID open event is handled by the BIOS to trigger a wake GPE.
Now we are talking about runtime idle systems where resuming from the s2idle invokes no BIOS code.
The new case is still working on Windows.
Which forces us to re-consider the kernel API of the ACPI LID.

Cheers
-Lv

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-03-04  3:37         ` Zheng, Lv
@ 2016-03-07 16:19             ` Bastien Nocera
  0 siblings, 0 replies; 11+ messages in thread
From: Bastien Nocera @ 2016-03-07 16:19 UTC (permalink / raw)
  To: Zheng, Lv, Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, cwhuang

On Fri, 2016-03-04 at 03:37 +0000, Zheng, Lv wrote:
<snip>
> 
> > I'm not happy about the "fix" for this problem either, but blaming
> > user-space for this is harsh, given the API exported by the kernel
> > and
> > how pretty much every laptop worked.
> > 
> [Lv Zheng] 
> It worked for so many years on top of traditional x86 laptops where
> the LID open event is handled by the BIOS to trigger a wake GPE.
> Now we are talking about runtime idle systems where resuming from the
> s2idle invokes no BIOS code.
> The new case is still working on Windows.
> Which forces us to re-consider the kernel API of the ACPI LID.

The current kernel API works on 99% of machines up until now, user-
space could not have been expected to know that the Lid status might be
incorrect on some machines, it had no way to know that, there was no
documentation saying "don't use the Lid status outside of Lid status
change events".

We either need to extend the current API to export the fact that we
should ignore the LID status outside of events, or we need a new API.

(We could fix this without any kernel changes, but we really need the
kernel to document that fact, and export it to user-space)

This is something you should have said at the start of the mail,
instead of repeatedly saying that user-space was broken. User-space
behaviour hasn't changed there for more than 10 years, and I'm fairly
certain it's worked the same way as APM (on x86) and PMU (on Mac PPC
machines) did.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
@ 2016-03-07 16:19             ` Bastien Nocera
  0 siblings, 0 replies; 11+ messages in thread
From: Bastien Nocera @ 2016-03-07 16:19 UTC (permalink / raw)
  To: Zheng, Lv, Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, cwhuang

On Fri, 2016-03-04 at 03:37 +0000, Zheng, Lv wrote:
<snip>
> 
> > I'm not happy about the "fix" for this problem either, but blaming
> > user-space for this is harsh, given the API exported by the kernel
> > and
> > how pretty much every laptop worked.
> > 
> [Lv Zheng] 
> It worked for so many years on top of traditional x86 laptops where
> the LID open event is handled by the BIOS to trigger a wake GPE.
> Now we are talking about runtime idle systems where resuming from the
> s2idle invokes no BIOS code.
> The new case is still working on Windows.
> Which forces us to re-consider the kernel API of the ACPI LID.

The current kernel API works on 99% of machines up until now, user-
space could not have been expected to know that the Lid status might be
incorrect on some machines, it had no way to know that, there was no
documentation saying "don't use the Lid status outside of Lid status
change events".

We either need to extend the current API to export the fact that we
should ignore the LID status outside of events, or we need a new API.

(We could fix this without any kernel changes, but we really need the
kernel to document that fact, and export it to user-space)

This is something you should have said at the start of the mail,
instead of repeatedly saying that user-space was broken. User-space
behaviour hasn't changed there for more than 10 years, and I'm fairly
certain it's worked the same way as APM (on x86) and PMU (on Mac PPC
machines) did.

Cheers

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
  2016-03-07 16:19             ` Bastien Nocera
  (?)
@ 2016-03-22  7:04             ` Zheng, Lv
  -1 siblings, 0 replies; 11+ messages in thread
From: Zheng, Lv @ 2016-03-22  7:04 UTC (permalink / raw)
  To: Bastien Nocera, Chen, Yu C, linux-acpi
  Cc: linux-kernel, rjw, lenb, Zhang, Rui, cwhuang

Hi,

> From: Bastien Nocera [mailto:hadess@hadess.net]
> Subject: Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
> 
> On Fri, 2016-03-04 at 03:37 +0000, Zheng, Lv wrote:
> <snip>
> >
> > > I'm not happy about the "fix" for this problem either, but blaming
> > > user-space for this is harsh, given the API exported by the kernel
> > > and
> > > how pretty much every laptop worked.
> > >
> > [Lv Zheng]
> > It worked for so many years on top of traditional x86 laptops where
> > the LID open event is handled by the BIOS to trigger a wake GPE.
> > Now we are talking about runtime idle systems where resuming from the
> > s2idle invokes no BIOS code.
> > The new case is still working on Windows.
> > Which forces us to re-consider the kernel API of the ACPI LID.
> 
> The current kernel API works on 99% of machines up until now, user-
> space could not have been expected to know that the Lid status might be
> incorrect on some machines, it had no way to know that, there was no
> documentation saying "don't use the Lid status outside of Lid status
> change events".
> 
> We either need to extend the current API to export the fact that we
> should ignore the LID status outside of events, or we need a new API.
> 
> (We could fix this without any kernel changes, but we really need the
> kernel to document that fact, and export it to user-space)
> 
> This is something you should have said at the start of the mail,
> instead of repeatedly saying that user-space was broken. User-space
> behaviour hasn't changed there for more than 10 years, and I'm fairly
> certain it's worked the same way as APM (on x86) and PMU (on Mac PPC
> machines) did.
[Lv Zheng] 
If you want a documentation fix.
It seems we need to fix ACPI spec first by clarifying this behavior.
By submitting ACPI spec change, we can also gain MS confirmation.
So please wait a bit longer to have this clarified.

Thanks
-Lv


> 
> Cheers

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-03-22  7:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-03 11:04 [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet Chen Yu
2016-02-15  3:34 ` Zheng, Lv
2016-02-15  3:34   ` Zheng, Lv
2016-02-15  5:39   ` Chen, Yu C
2016-02-22  7:24     ` Zheng, Lv
2016-02-22 14:14       ` Bastien Nocera
2016-02-22 14:14         ` Bastien Nocera
2016-03-04  3:37         ` Zheng, Lv
2016-03-07 16:19           ` Bastien Nocera
2016-03-07 16:19             ` Bastien Nocera
2016-03-22  7:04             ` Zheng, Lv

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.