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

Groups > linux.kernel > #1334125 > unrolled thread

RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet

Started by"Zheng, Lv" <lv.zheng@intel.com>
First post2016-02-15 04:40 +0100
Last post2016-02-22 15:20 +0100
Articles 4 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet "Zheng, Lv" <lv.zheng@intel.com> - 2016-02-15 04:40 +0100
    RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet "Chen, Yu C" <yu.c.chen@intel.com> - 2016-02-15 06:50 +0100
      RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet "Zheng, Lv" <lv.zheng@intel.com> - 2016-02-22 08:30 +0100
        Re: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet Bastien Nocera <hadess@hadess.net> - 2016-02-22 15:20 +0100

#1334125 — RE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet

From"Zheng, Lv" <lv.zheng@intel.com>
Date2016-02-15 04:40 +0100
SubjectRE: [PATCH] ACPI / button: Avoid using broken _LID on Surface tablet
Message-ID<r2ib0-3Gn-15@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1334169

From"Chen, Yu C" <yu.c.chen@intel.com>
Date2016-02-15 06:50 +0100
Message-ID<r2kcO-55M-11@gated-at.bofh.it>
In reply to#1334125
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

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

#1339159

From"Zheng, Lv" <lv.zheng@intel.com>
Date2016-02-22 08:30 +0100
Message-ID<r4T6q-2dV-7@gated-at.bofh.it>
In reply to#1334169
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

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

#1339503

FromBastien Nocera <hadess@hadess.net>
Date2016-02-22 15:20 +0100
Message-ID<r4Zvc-6YD-9@gated-at.bofh.it>
In reply to#1339159
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web