| Message ID | 1454497460-16803-1-git-send-email-yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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
SGksDQoNCj4gRnJvbTogQmFzdGllbiBOb2NlcmEgW21haWx0bzpoYWRlc3NAaGFkZXNzLm5ldF0N Cj4gU3ViamVjdDogUmU6IFtQQVRDSF0gQUNQSSAvIGJ1dHRvbjogQXZvaWQgdXNpbmcgYnJva2Vu IF9MSUQgb24gU3VyZmFjZSB0YWJsZXQNCj4gDQo+IE9uIE1vbiwgMjAxNi0wMi0yMiBhdCAwNzoy NCArMDAwMCwgWmhlbmcsIEx2IHdyb3RlOg0KPiA+IFtMdiBaaGVuZ10NCj4gPiBBY2NvcmRpbmcg dG8gdGhlIGtub3dsZWRnZSBvZiB2YXJpb3VzIEJJT1MgX0xJRCBpbXBsZW1lbnRhdGlvbiwgTGlu dXgNCj4gPiBidXR0b24gZHJpdmVyIGRvZXNuJ3Qgc2VlbSB0byBiZSB3b3JraW5nIHdyb25nLg0K PiA+IEl0IGluIGZhY3Qgd29ya3MgY29ycmVjdGx5IGFuZCBpcyBhYmxlIHRvIGhhbmRsZSBhbGwg QklPUyBfTElEDQo+ID4gaW1wbGVtZW50YXRpb25zLg0KPiA+DQo+ID4gU28gdGhpcyBsb29rcyB0 byBtZSBsaWtlIGEgdXNlciBzcGFjZSBidWcuDQo+IA0KPiBJIGRvbid0IHRoaW5rIGl0IGlzIG9u ZSwgZ2l2ZW4gdGhlIGtlcm5lbCBBUEkgZm9yIHRoYXQgZnVuY3Rpb25hbGl0eS4NCj4gSWYsIGlu c3RlYWQgb2YgZXhwb3NpbmcgdGhlIGxpZCBhcyBhbiBpbnB1dCBzd2l0Y2gsIHRoZSBrZXJuZWwg b25seQ0KPiBldmVyIHNlbnQgYW4gZXZlbnQgc2F5aW5nICJ0aGUgbGlkIGlzIG5vdyBjbG9zZWQi IHRoZW4gd2Ugd291bGRuJ3QgaGF2ZQ0KPiB0aGF0IHByb2JsZW0sIGFzIHdlJ2QgbGlrZWx5IGV4 cGVjdCB0aGUgbGlkIHRvIGJlIG9wZW5lZCB3aGVuIHN0YXJ0aW5nDQo+IHRoZSBtYWNoaW5lIChp ZiBwcmVzZW50KS4NCltMdiBaaGVuZ10gDQpMZXQgbWUgc2hvdyB5b3Ugc2V2ZXJhbCBwcm9vZnMg b2YgV2luZG93cyBiZWhhdmlvcnMuDQoNCjEuIEhhdmUgeW91IGV2ZXIgc2VlbiBhIHNjcmVlbiBv biBXaW5kb3dzIHRoYXQgY2FuIHRlbGwgeW91IHRoZSBzdGF0dXMgb2YgTElELg0KSWYgeW91IGhh dmUgc2VlbiBzdWNoIHNjcmVlbiwgd2hhdCB0aGUgc3RhdHVzIGlzIHdoZW4gdGhlIHN5c3RlbSBp cyBib290ZWQuDQpJIGhhdmUgbmV2ZXIgc2VlbiBzdWNoIGEgVUksIHNvIFdpbmRvd3MgaW4gZmFj dCBjYXJlcyBubyBjdXJyZW50IExJRCBzdGF0dXMuDQoNCjIuIEhhdmUgeW91IGV2ZXIgc2VlbiBh IHNjcmVlbiBvbiBXaW5kb3dzIHRoYXQgYWxsb3cgdXNlcnMgdG8gc3BlY2lmeSBvcGVuIExJRCBh Y3Rpb25zPw0KVGhlIGFuc3dlciBpcyBuby4NCk9uIHRyYWRpdGlvbmFsIHg4NiBwbGF0Zm9ybXMs IHN5c3RlbSBpcyB3b2tlbiB1cCB2aWEgd2FrZSBHUEVzLg0KU28gdGhlIG9wZW4gZXZlbnQgaW4g ZmFjdCBpcyBoYW5kbGVkIGJ5IEJJT1MgYW5kIFdpbmRvd3MgY2FyZXMgbm90aGluZyBhYm91dCBp dC4NCg0KMy4gSGF2ZSB5b3UgZXZlciBzZWVuIGEgc2NyZWVuIG9uIFdpbmRvd3MgdGhhdCBhbGxv dyB1c2VycyB0byBzcGVjaWZ5IGNsb3NlIExJRCBhY3Rpb25zPw0KWWVzLCBJIGhhdmUsIGl0IGlz IGluIHBvd2VyIG9wdGlvbnMuDQpTbyBXaW5kb3dzIGluIGZhY3Qgb25seSBjYXJlcyBhYm91dCB0 aGUgTElEIGNsb3NlIG5vdGlmaWNhdGlvbiBhbmQgd2lsbCBvbmx5IGNoZWNrIExJRCBzdGF0dXMg d2hlbiBpdCByZWNlaXZlcyB0aGlzIG5vdGlmaWNhdGlvbi4NCg0KVGhlIGFib3ZlIGZhY3RzIGNh biBtYWtlIGNsZWFyIGFib3V0IHdoYXQgQklPUyB3aWxsIGltcGxlbWVudCBmb3IgdGhlIEFDUEkg TElELg0KDQpBbmQgZnJvbSB0aGlzIHBvaW50IG9mIHZpZXcsIGluIGtlcm5lbCBBQ1BJIExJRCBk cml2ZXIsIG9ubHkgdGhpcyBjb21taXQgaXMgd3Jvbmc6DQpjb21taXQgMjNkZTVkOWVmMmE0YmJj NGY3MzNmNTgzMTFiY2I3Y2Y2MjM5YzgxMw0KQXV0aG9yOiBBbGV4ZXkgU3Rhcmlrb3Zza2l5IDxh c3Rhcmlrb3Zza2l5QHN1c2UuZGU+DQpTdWJqZWN0OiBBQ1BJOiBidXR0b246IHNlbmQgaW5pdGlh bCBsaWQgc3RhdGUgYWZ0ZXIgYWRkIGFuZCByZXN1bWUNClRoaXMgc2VlbXMgdG8gYmUgYSBmYWxz ZSByb290IGNhdXNlZCBidWcgZml4Og0KaHR0cHM6Ly9idWd6aWxsYS5ub3ZlbGwuY29tL3Nob3df YnVnLmNnaT9pZD0zMjY4MTQNCkkgZ3Vlc3MgdGhlIG9yaWdpbmFsIGJ1ZyB3YXMgaW4gdGhlIEVD IGRyaXZlciBvciBzb21ld2hlcmUgZWxzZS4NCg0KQnV0IHRvIG91ciBzdXJwcmlzZXMsIGFmdGVy IHJldmVydGluZyB0aGlzIGNvbW1pdC4NCldlIGNhbiBzZWU6DQoxLiBLZXJuZWwgZG9lc24ndCB0 cmlnZ2VyIGFueSBldmVudCB0byB0aGUgdXNlcnNwYWNlLg0KMi4gQnV0IHdlIHN0aWxsIGNhbiBz ZWUgdGhhdCBTdXJmYWNlIGlzIGF1dG9tYXRpY2FsbHkgc3VzcGVuZGVkIGV2ZXJ5IHNldmVyYWwg c2Vjb25kcy4NCg0KQWNjb3JkaW5nIHRvIHRoZSBCdWd6aWxsYSBpbnZlc3RpZ2F0aW9uLCB0aGUg c3VzcGVuZCB3YXMgdHJpZ2dlcmVkIGJ5IHN5c3RlbWQuDQpJIHRodXMganVzdCBjb25jbHVkZWQg dGhhdCB0aGlzIGNvdWxkIGJlIGEgdXNlcnNwYWNlIGJ1Zy4NCkJ1dCBtYXliZSB0aGlzIGlzIGEg c3lzdGVtZCBiZWhhdmlvciB0cmlnZ2VyZWQgYnkgc29tZSBrZXJuZWwgZXZlbnRzIHRoYXQgYXJl IG5vdCBjYXB0dXJlZCBieSBvdXIgZGVidWdnaW5nIHBhdGNoZXMuDQpMZXQgdGhlIHJlcG9ydGVy IGFuZCB0aGUgZGV2ZWxvcGVyIGRvIG1vcmUgaW52ZXN0aWdhdGlvbiB0aGVyZS4NCg0KSSBqdXN0 IHJhaXNlZCBteSBjb25jZXJuIGFib3V0IHRoaXMgZml4Lg0KVGhlIGZpeCBicm91Z2h0IGJ5IHRo aXMgcGF0Y2ggaXMgYXBwYXJlbnRseSB3cm9uZyBhY2NvcmRpbmcgdG8gdGhlIGFib3ZlIGNvbmNs dXNpb25zLg0KDQo+IA0KPiA+IFRoYXQgdGhlIHVzZXIgc3BhY2UgZG9lc24ndCBjb250YWluIGEg cHJvcGVyIG1vZGUgdG8gaGFuZGxlIEFDUEkgQklPUw0KPiA+IF9MSUQgaW1wbGVtZW50YXRpb25z Lg0KPiA+DQo+ID4gV2h5IHNob3VsZCBrZXJuZWwgd29yayB0aGlzIGdhcCBhcm91bmQ/DQo+ID4g VGhlIGdhcCBpczoNCj4gPiBUaGUgdXNlciBzcGFjZSByZXF1aXJlcyBsaWQgZGV2aWNlIHRvIGFj dCBpbiB0aGUgd2F5IGl0IHdhbnRzLg0KPiANCj4gSW4gdGhlIHdheSB0aGF0IGl0J3MgYWx3YXlz IHdvcmtlZCB1cCB1bnRpbCBub3csIHJhdGhlci4NCltMdiBaaGVuZ10gDQpUaGlzIGlzIG5vdCBw ZXJzdWFzaXZlLg0KVGhpbmdzIGNhbiBhcHBlYXIgdG8gYmUgd29ya2luZyB3aXRoIHdvcmthcm91 bmRzIGFwcGxpZWQuDQpEdXJpbmcgbXkgY2FyZWVyLCBJIGhhdmUgc2VlbiBwbGVudHkgb2Ygc3Vj aCBjYXNlcy4NCkFuZCBJ4oCYdmUgcHJvdmVuIHRoYXQgdGhlcmUgYXJlIG1hbnkgc3VjaCBraW5k IG9mIHdyb25nbHkgcm9vdCBjYXVzZWQgZml4ZXMgaW4gQUNQSSBzdWJzeXN0ZW0gaW4gdGhpcyBj b21tdW5pdHkgZHVyaW5nIHRoZSBwYXN0IHllYXJzLg0KDQo+IA0KPiA+IFdoaWxlIHRoZSBBQ1BJ IEJJT1Mgb25seSBwcm92aWRlcyBsaWQgYmVoYXZpb3IgdGhhdCBpcyB3b3JraW5nIGZvcg0KPiA+ IFdpbmRvd3MgcG93ZXItc2F2aW5nIHNldHRpbmdzLg0KPiA+IElNTywNCj4gPiAxLiBXaW5kb3dz IE9ubHkgcmVxdWlyZXMgTElEIGNsb3NlIG5vdGlmaWNhdGlvbnMgdG8gdHJpZ2dlciBwb3dlci0N Cj4gPiBzYXZlIGFjdGlvbiBhbmQgb25seSBldmFsdWF0ZXMgX0xJRCBhZnRlciByZWNlaXZpbmcg YSBMSUQNCj4gPiBub3RpZmljYXRpb24sDQo+ID4gwqDCoMKgIEJJT1NlbiBvbmx5IGVuc3VyZSBf TElEIHJldHVybmluZyB2YWx1ZSBpcyBjb3JyZWN0IGFmdGVyIGENCj4gPiBub3RpZmljYXRpb24u DQo+IA0KPiBJbiB3aGljaCBjYXNlLCBleHBlY3RpbmcgdGhlIGxpZCB0byBhbHdheXMgYmUgb3Bl bmVkIG9uIHN0YXJ0dXAgd291bGQNCj4gcHJvYmFibHkgYmUgYSBmYWlyIGFzc3VtcHRpb24sIG5v Pw0KPiANCltMdiBaaGVuZ10gDQpUaGUgYW5zd2VyIHNob3VsZCBiZSBubyBoZXJlLg0KDQpJZiB3 ZSBtYWtlIHN5c3RlbSBkZWZhdWx0IExJRCBzdGF0dXMgYXMgb3BlbiBvbiBib290L3Jlc3VtZSwg dGhlbiB3aGF0IGFib3V0IHRoaXMgY2FzZT8NCg0KQWNjb3JkaW5nIHRvIG91ciB0ZXN0cywgd2Ug Y2FuIHJlYm9vdC9yZXN1bWUgc3VyZmFjZSB3aXRoIExJRCBjbG9zZWQgaW50byBhIHJ1bm5pbmcg V2luZG93cy4NClNvIGlmIHdlIGZvcmNlIHRoZSBMSUQgc3RhdHVzIHRvIGJlIG9wZW5lZCBhZnRl ciB0aGUgcmVib290L3Jlc3VtZSwgdGhlbiB0aGlzIGFsc28gc2VlbXMgdG8gYmUgYSBjb25mbGlj dCBhZ2FpbnN0IHRoZSByZWFsIHN0YXR1cy4NCldoYXQgdGhlIGJlbmVmaXQgdG8gZm9yY2Ugc3Vj aCBhIHdyb25nIHN0YXR1cywgZ2l2ZW4gdGhhdCB3ZSBhcmUgYWxsIG5vdCBvYnNlc3NpdmVzLi4u DQoNCj4gPiAyLiBCdXQgTGludXggdXNlciBzcGFjZSByZXF1aXJlcyBhbGwgTElEIG5vdGlmaWNh dGlvbnMgdG8gYmUNCj4gPiBpbnN0YW50bHkvY29ycmVjdGx5IHJlcG9ydGVkIGFuZCB3YW50cyB0 byBrbm93IHRoZSBleGFjdCBMSUQgc3RhdGVzDQo+ID4gd2hlbmV2ZXIgaXQgcmVhZHMgdGhlIHN0 YXRlcyBmcm9tIHRoZSBzeXNmcy4NCj4gPiBJdCBkb2Vzbid0IHNlZW0gdG8gYmUgcG9zc2libGUg Zm9yIHRoZSBrZXJuZWwgdG8gd29yayBiZXR3ZWVuIEJJT1Nlbg0KPiA+IGFuZCB0aGUgdXNlciBz cGFjZSB0byBmaWxsIHN1Y2ggYSBnYXAuDQo+IA0KPiBJZiB5b3UgcXVpcmsgdGhlIGtlcm5lbCBs aWQgaGFuZGxpbmcgdG8gYWx3YXlzIGJlIG9wZW5lZCBvbiBzdGFydHVwLA0KPiBhbmQgd2Ugd2Fp dGVkIHVudGlsIHRoZSBmaXJzdCBldmVudCB0byBjb3JyZWN0IGl0IGlmIG5lY2Vzc2FyeSwgc2Vl bXMNCj4gbGlrZSB0aGUgZWFzaWVzdCB3YXkgdG8gZ28uDQo+IA0KPiBCdXQgdGhhdCBicmluZ3Mg bWUgdGhlIHF1ZXN0aW9uIG9mIGhvdyBXaW5kb3dzIChhbmQgdGhlbiBMaW51eCkgYmVoYXZlDQo+ IHdoZW4geW91J3ZlIGJvb3RlZCB5b3VyIGxhcHRvcCBhbmQgY2xvc2VkIHRoZSBsaWQgc3RyYWln aHQgYXdheSwgYmVmb3JlDQo+IGFueSBkcml2ZXIgaW4gdGhlIE9TIGhhZCB0aGUgY2hhbmNlIHRv IGJlIGFyb3VuZCB0byBzZWUgdGhlDQo+IG5vdGlmaWNhdGlvbj8NCj4gDQpbTHYgWmhlbmddIA0K SWYgd2UgYm9vdGVkIHRoZSBsYXB0b3Agd2l0aCBMSUQgb3BlbmVkLCBhbmQgY2xvc2UgaXQgcmln aHQgYmVmb3JlIHRoZSBXaW5kb3dzIGlzIGZ1bGx5IGJvb3RlZC4NCldlIGNhbiBzZWUgdGhlIGxh cHRvcCBib290ZWQgaW50byBhIHJ1bm5pbmcgV2luZG93cy4NCg0KPiBJdCBhbHNvIGJyaW5ncyB0 aGUgcXVlc3Rpb24gb2YgaG93IFdpbmRvd3Mgd2lsbCBrbm93IHRoYXQgdGhlIGxpZCBpcw0KPiBz dGlsbCBjbG9zZWQgd2hlbiBjb21pbmcgb3V0IG9mIHN1c3BlbmQgYnkgcHJlc3NpbmcgdGhlIHBv d2VyIGJ1dHRvbiwNCj4gd2hpY2ggY2FuIGhhcHBlbiBkZXBlbmRpbmcgb24gdGhlIGhhcmR3YXJl IGRlc2lnbiAoaXQncyBjZXJ0YWlubHkNCj4gcG9zc2libGUgdG8gcHJlc3MgdGhlIHBvd2VyIGJ1 dHRvbiB3aXRoIHRoZSBsaWQgY2xvc2VkIG9uIHRoZSBTdXJmYWNlDQo+IDMsIGFuZCBsaWtlbHkg b3RoZXIgU3VyZmFjZXMpLg0KPiANCltMdiBaaGVuZ10gDQpJZiB3ZSBib290ZWQvcmVzdW1lZCB0 aGUgbGFwdG9wIHdpdGggTElEIGNsb3NlZCBieSBwcmVzc2luZyB0aGUgcG93ZXIgYnV0dG9uLg0K V2UgY2FuIHNlZSB0aGUgbGFwdG9wIGJvb3RlZC9yZXN1bWVkIGludG8gYSBydW5uaW5nIFdpbmRv d3MuDQoNCj4gSSdtIG5vdCBoYXBweSBhYm91dCB0aGUgImZpeCIgZm9yIHRoaXMgcHJvYmxlbSBl aXRoZXIsIGJ1dCBibGFtaW5nDQo+IHVzZXItc3BhY2UgZm9yIHRoaXMgaXMgaGFyc2gsIGdpdmVu IHRoZSBBUEkgZXhwb3J0ZWQgYnkgdGhlIGtlcm5lbCBhbmQNCj4gaG93IHByZXR0eSBtdWNoIGV2 ZXJ5IGxhcHRvcCB3b3JrZWQuDQo+IA0KW0x2IFpoZW5nXSANCkl0IHdvcmtlZCBmb3Igc28gbWFu eSB5ZWFycyBvbiB0b3Agb2YgdHJhZGl0aW9uYWwgeDg2IGxhcHRvcHMgd2hlcmUgdGhlIExJRCBv cGVuIGV2ZW50IGlzIGhhbmRsZWQgYnkgdGhlIEJJT1MgdG8gdHJpZ2dlciBhIHdha2UgR1BFLg0K Tm93IHdlIGFyZSB0YWxraW5nIGFib3V0IHJ1bnRpbWUgaWRsZSBzeXN0ZW1zIHdoZXJlIHJlc3Vt aW5nIGZyb20gdGhlIHMyaWRsZSBpbnZva2VzIG5vIEJJT1MgY29kZS4NClRoZSBuZXcgY2FzZSBp cyBzdGlsbCB3b3JraW5nIG9uIFdpbmRvd3MuDQpXaGljaCBmb3JjZXMgdXMgdG8gcmUtY29uc2lk ZXIgdGhlIGtlcm5lbCBBUEkgb2YgdGhlIEFDUEkgTElELg0KDQpDaGVlcnMNCi1Mdg0K -- 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
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
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
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
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(-)