Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1402194 > unrolled thread
| Started by | Lv Zheng <lv.zheng@intel.com> |
|---|---|
| First post | 2016-05-17 10:30 +0200 |
| Last post | 2016-05-28 00:20 +0200 |
| Articles | 18 — 6 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume Lv Zheng <lv.zheng@intel.com> - 2016-05-17 10:30 +0200
Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-18 01:40 +0200
RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-18 03:30 +0200
Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-19 01:00 +0200
RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-19 04:00 +0200
Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-19 15:30 +0200
Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2016-05-26 15:40 +0200
RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-30 03:50 +0200
Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume Bastien Nocera <hadess@hadess.net> - 2016-05-18 15:00 +0200
Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-18 23:50 +0200
RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-19 04:00 +0200
[PATCH v2 2/3] ACPI / button: Refactor functions to eliminate redundant code Lv Zheng <lv.zheng@intel.com> - 2016-05-27 09:20 +0200
[PATCH v2 1/3] ACPI / button: Remove initial lid state notification Lv Zheng <lv.zheng@intel.com> - 2016-05-27 09:20 +0200
[PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume Lv Zheng <lv.zheng@intel.com> - 2016-05-27 09:20 +0200
Re: [PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2016-05-30 10:20 +0200
RE: [PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume "Zheng, Lv" <lv.zheng@intel.com> - 2016-05-31 05:00 +0200
[PATCH v2 0/3] ACPI / button: Clarify initial lid state Lv Zheng <lv.zheng@intel.com> - 2016-05-27 09:20 +0200
Re: [PATCH v2 0/3] ACPI / button: Clarify initial lid state Valdis.Kletnieks@vt.edu - 2016-05-28 00:20 +0200
| From | Lv Zheng <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-17 10:30 +0200 |
| Subject | [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rzIy6-28V-11@gated-at.bofh.it> |
(This patch hasn't been tested, it's sent for comments)
Linux userspace (systemd-logind) keeps on rechecking lid state when the
lid state is closed. If it failed to update the lid state to open after
boot/resume, it could suspend the system. But as:
1. Traditional ACPI platform may not generate the lid open event after
resuming as the open event is actually handled by the BIOS and the system
is then resumed from a FACS vector.
2. The _LID control method's initial returning value is not reliable. The
_LID control method is described to return the "current" lid state,
however the word of "current" has ambiguity, many BIOSen return lid
state upon last lid notification while the developers may think the
BIOSen should always return the lid state upon last _LID evaluation.
There won't be difference when we evaluate _LID during the runtime, the
problem is the initial returning value of this function. When the BIOSen
implement this control method with cached value, the initial returning
value is likely not reliable.
Thus there is no mean for the ACPI lid driver to provide such an event
conveying correct current lid state. When there is no such an event or the
event conveys wrong result, false suspending can be examined.
The root cause of the issue is systemd itself, it could handle the ACPI
control method lid device by implementing a special option like
LidSwitchLevelTriggered=False when it detected the ACPI lid device. However
there is no explicit documentation clarified the ambiguity, we need to
work it around in the kernel before systemd changing its mind.
Link 1: https://lkml.org/2016/3/7/460
Link 2: https://github.com/systemd/systemd/issues/2087
Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211
https://bugzilla.kernel.org/show_bug.cgi?id=106151
https://bugzilla.kernel.org/show_bug.cgi?id=106941
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Bastien Nocera: <hadess@hadess.net>
---
drivers/acpi/button.c | 63 +++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 59 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 5c3b091..bb14ca5 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -53,6 +53,10 @@
#define ACPI_BUTTON_DEVICE_NAME_LID "Lid Switch"
#define ACPI_BUTTON_TYPE_LID 0x05
+#define ACPI_BUTTON_LID_INIT_IGNORE 0x00
+#define ACPI_BUTTON_LID_INIT_OPEN 0x01
+#define ACPI_BUTTON_LID_INIT_METHOD 0x02
+
#define _COMPONENT ACPI_BUTTON_COMPONENT
ACPI_MODULE_NAME("button");
@@ -105,6 +109,7 @@ struct acpi_button {
static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
static struct acpi_device *lid_device;
+static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
/* --------------------------------------------------------------------------
FS Interface (/proc)
@@ -246,7 +251,8 @@ int acpi_lid_open(void)
}
EXPORT_SYMBOL(acpi_lid_open);
-static int acpi_lid_send_state(struct acpi_device *device)
+static int acpi_lid_send_state(struct acpi_device *device,
+ bool notify_init_state)
{
struct acpi_button *button = acpi_driver_data(device);
unsigned long long state;
@@ -257,6 +263,10 @@ static int acpi_lid_send_state(struct acpi_device *device)
if (ACPI_FAILURE(status))
return -ENODEV;
+ if (notify_init_state &&
+ lid_init_state == ACPI_BUTTON_LID_INIT_OPEN)
+ state = 1;
+
/* input layer checks if event is redundant */
input_report_switch(button->input, SW_LID, !state);
input_sync(button->input);
@@ -278,6 +288,13 @@ static int acpi_lid_send_state(struct acpi_device *device)
return ret;
}
+static int acpi_lid_send_init_state(struct acpi_device *device)
+{
+ if (lid_init_state != ACPI_BUTTON_LID_INIT_IGNORE)
+ return acpi_lid_send_state(device, true);
+ return 0;
+}
+
static void acpi_button_notify(struct acpi_device *device, u32 event)
{
struct acpi_button *button = acpi_driver_data(device);
@@ -290,7 +307,7 @@ 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) {
- acpi_lid_send_state(device);
+ acpi_lid_send_state(device, false);
} else {
int keycode;
@@ -335,7 +352,7 @@ static int acpi_button_resume(struct device *dev)
button->suspended = false;
if (button->type == ACPI_BUTTON_TYPE_LID)
- return acpi_lid_send_state(device);
+ return acpi_lid_send_init_state(device);
return 0;
}
#endif
@@ -416,7 +433,7 @@ static int acpi_button_add(struct acpi_device *device)
if (error)
goto err_remove_fs;
if (button->type == ACPI_BUTTON_TYPE_LID) {
- acpi_lid_send_state(device);
+ acpi_lid_send_init_state(device);
/*
* This assumes there's only one lid device, or if there are
* more we only care about the last one...
@@ -446,4 +463,42 @@ static int acpi_button_remove(struct acpi_device *device)
return 0;
}
+static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
+{
+ int result = 0;
+
+ if (!strncmp(val, "open", sizeof("open") - 1)) {
+ lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
+ pr_info("Notify initial lid state as open\n");
+ } else if (!strncmp(val, "method", sizeof("method") - 1)) {
+ lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
+ pr_info("Notify initial lid state with _LID return value\n");
+ } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
+ lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
+ pr_info("Do not notify initial lid state\n");
+ } else
+ result = -EINVAL;
+ return result;
+}
+
+static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
+{
+ switch (lid_init_state) {
+ case ACPI_BUTTON_LID_INIT_OPEN:
+ return sprintf(buffer, "open");
+ case ACPI_BUTTON_LID_INIT_METHOD:
+ return sprintf(buffer, "method");
+ case ACPI_BUTTON_LID_INIT_IGNORE:
+ return sprintf(buffer, "ignore");
+ default:
+ return sprintf(buffer, "invalid");
+ }
+ return 0;
+}
+
+module_param_call(lid_init_state,
+ param_set_lid_init_state, param_get_lid_init_state,
+ NULL, 0644);
+MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
+
module_acpi_driver(acpi_button_driver);
--
1.7.10
[toc] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-05-18 01:40 +0200 |
| Message-ID | <rzWKK-2DG-11@gated-at.bofh.it> |
| In reply to | #1402194 |
On Tue, May 17, 2016 at 10:27 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> (This patch hasn't been tested, it's sent for comments)
I have a couple of concerns (see below).
> Linux userspace (systemd-logind) keeps on rechecking lid state when the
> lid state is closed. If it failed to update the lid state to open after
> boot/resume, it could suspend the system. But as:
> 1. Traditional ACPI platform may not generate the lid open event after
> resuming as the open event is actually handled by the BIOS and the system
> is then resumed from a FACS vector.
> 2. The _LID control method's initial returning value is not reliable. The
> _LID control method is described to return the "current" lid state,
> however the word of "current" has ambiguity, many BIOSen return lid
> state upon last lid notification while the developers may think the
> BIOSen should always return the lid state upon last _LID evaluation.
> There won't be difference when we evaluate _LID during the runtime, the
> problem is the initial returning value of this function. When the BIOSen
> implement this control method with cached value, the initial returning
> value is likely not reliable.
> Thus there is no mean for the ACPI lid driver to provide such an event
> conveying correct current lid state. When there is no such an event or the
> event conveys wrong result, false suspending can be examined.
>
> The root cause of the issue is systemd itself, it could handle the ACPI
> control method lid device by implementing a special option like
> LidSwitchLevelTriggered=False when it detected the ACPI lid device. However
> there is no explicit documentation clarified the ambiguity, we need to
> work it around in the kernel before systemd changing its mind.
The above doesn't explain how the issue is addressed here.
> Link 1: https://lkml.org/2016/3/7/460
> Link 2: https://github.com/systemd/systemd/issues/2087
> Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> https://bugzilla.kernel.org/show_bug.cgi?id=106151
> https://bugzilla.kernel.org/show_bug.cgi?id=106941
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Cc: Bastien Nocera: <hadess@hadess.net>
> ---
> drivers/acpi/button.c | 63 +++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 5c3b091..bb14ca5 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -53,6 +53,10 @@
> #define ACPI_BUTTON_DEVICE_NAME_LID "Lid Switch"
> #define ACPI_BUTTON_TYPE_LID 0x05
>
> +#define ACPI_BUTTON_LID_INIT_IGNORE 0x00
> +#define ACPI_BUTTON_LID_INIT_OPEN 0x01
> +#define ACPI_BUTTON_LID_INIT_METHOD 0x02
> +
> #define _COMPONENT ACPI_BUTTON_COMPONENT
> ACPI_MODULE_NAME("button");
>
> @@ -105,6 +109,7 @@ struct acpi_button {
>
> static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> static struct acpi_device *lid_device;
> +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
>
> /* --------------------------------------------------------------------------
> FS Interface (/proc)
> @@ -246,7 +251,8 @@ int acpi_lid_open(void)
> }
> EXPORT_SYMBOL(acpi_lid_open);
>
> -static int acpi_lid_send_state(struct acpi_device *device)
> +static int acpi_lid_send_state(struct acpi_device *device,
> + bool notify_init_state)
> {
> struct acpi_button *button = acpi_driver_data(device);
> unsigned long long state;
> @@ -257,6 +263,10 @@ static int acpi_lid_send_state(struct acpi_device *device)
> if (ACPI_FAILURE(status))
> return -ENODEV;
>
> + if (notify_init_state &&
> + lid_init_state == ACPI_BUTTON_LID_INIT_OPEN)
> + state = 1;
> +
Why do we need to complicate this function?
Can't we have a separate function for sending the fake "lid open" event?
> /* input layer checks if event is redundant */
> input_report_switch(button->input, SW_LID, !state);
> input_sync(button->input);
> @@ -278,6 +288,13 @@ static int acpi_lid_send_state(struct acpi_device *device)
> return ret;
> }
>
> +static int acpi_lid_send_init_state(struct acpi_device *device)
> +{
> + if (lid_init_state != ACPI_BUTTON_LID_INIT_IGNORE)
> + return acpi_lid_send_state(device, true);
> + return 0;
> +}
> +
> static void acpi_button_notify(struct acpi_device *device, u32 event)
> {
> struct acpi_button *button = acpi_driver_data(device);
> @@ -290,7 +307,7 @@ 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) {
> - acpi_lid_send_state(device);
> + acpi_lid_send_state(device, false);
I wouldn't change this code at all.
> } else {
> int keycode;
>
> @@ -335,7 +352,7 @@ static int acpi_button_resume(struct device *dev)
>
> button->suspended = false;
> if (button->type == ACPI_BUTTON_TYPE_LID)
> - return acpi_lid_send_state(device);
> + return acpi_lid_send_init_state(device);
> return 0;
> }
> #endif
> @@ -416,7 +433,7 @@ static int acpi_button_add(struct acpi_device *device)
> if (error)
> goto err_remove_fs;
> if (button->type == ACPI_BUTTON_TYPE_LID) {
> - acpi_lid_send_state(device);
> + acpi_lid_send_init_state(device);
> /*
> * This assumes there's only one lid device, or if there are
> * more we only care about the last one...
> @@ -446,4 +463,42 @@ static int acpi_button_remove(struct acpi_device *device)
> return 0;
> }
>
> +static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
> +{
> + int result = 0;
> +
> + if (!strncmp(val, "open", sizeof("open") - 1)) {
> + lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> + pr_info("Notify initial lid state as open\n");
> + } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> + lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> + pr_info("Notify initial lid state with _LID return value\n");
> + } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> + lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> + pr_info("Do not notify initial lid state\n");
> + } else
> + result = -EINVAL;
> + return result;
> +}
> +
> +static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
> +{
> + switch (lid_init_state) {
> + case ACPI_BUTTON_LID_INIT_OPEN:
> + return sprintf(buffer, "open");
> + case ACPI_BUTTON_LID_INIT_METHOD:
> + return sprintf(buffer, "method");
> + case ACPI_BUTTON_LID_INIT_IGNORE:
> + return sprintf(buffer, "ignore");
> + default:
> + return sprintf(buffer, "invalid");
> + }
> + return 0;
> +}
> +
> +module_param_call(lid_init_state,
> + param_set_lid_init_state, param_get_lid_init_state,
> + NULL, 0644);
> +MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial state");
> +
I'm not seeing a particular value in having this command line switch
to be honest. Apparently, the issue can be worked around from user
space in any case, so why do we need one more way to work around it?
> module_acpi_driver(acpi_button_driver);
> --
The main concern is general, though. Evidently, we send fake lid
input events to user space on init and resume. I don't think this is
a good idea, because it may confuse systems like Chrome that want to
implement "dark resume" scenarios and may rely on input events to
decide whether to start a UI or suspend again.
Thus it might be better to simply drop the sending of those fake input
events from the code.
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-18 03:30 +0200 |
| Subject | RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rzYtc-3J2-13@gated-at.bofh.it> |
| In reply to | #1402655 |
Hi, Rafael
Thanks for the review.
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Wednesday, May 18, 2016 7:37 AM
> To: Zheng, Lv <lv.zheng@intel.com>
> Cc: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Rafael J. Wysocki
> <rjw@rjwysocki.net>; Brown, Len <len.brown@intel.com>; Lv Zheng
> <zetalog@gmail.com>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>;
> Bastien Nocera: <hadess@hadess.net>
> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
> boot/resume
>
> On Tue, May 17, 2016 at 10:27 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > (This patch hasn't been tested, it's sent for comments)
>
> I have a couple of concerns (see below).
>
> > Linux userspace (systemd-logind) keeps on rechecking lid state when the
> > lid state is closed. If it failed to update the lid state to open after
> > boot/resume, it could suspend the system. But as:
> > 1. Traditional ACPI platform may not generate the lid open event after
> > resuming as the open event is actually handled by the BIOS and the system
> > is then resumed from a FACS vector.
> > 2. The _LID control method's initial returning value is not reliable. The
> > _LID control method is described to return the "current" lid state,
> > however the word of "current" has ambiguity, many BIOSen return lid
> > state upon last lid notification while the developers may think the
> > BIOSen should always return the lid state upon last _LID evaluation.
> > There won't be difference when we evaluate _LID during the runtime, the
> > problem is the initial returning value of this function. When the BIOSen
> > implement this control method with cached value, the initial returning
> > value is likely not reliable.
> > Thus there is no mean for the ACPI lid driver to provide such an event
> > conveying correct current lid state. When there is no such an event or the
> > event conveys wrong result, false suspending can be examined.
> >
> > The root cause of the issue is systemd itself, it could handle the ACPI
> > control method lid device by implementing a special option like
> > LidSwitchLevelTriggered=False when it detected the ACPI lid device. However
> > there is no explicit documentation clarified the ambiguity, we need to
> > work it around in the kernel before systemd changing its mind.
>
> The above doesn't explain how the issue is addressed here.
[Lv Zheng]
The story is a bit long.
We can see several issues that some platform suspends right after boot/resume.
We noticed that on that platforms, _LID is always implemented with cached lid state returned.
And it's initial returning value may be "closed" after boot/resume.
It appears the acpi_lid_send_state() sent after boot/resume is the culprit to report the wrong lid state to the userspace.
But to our surprise, after delete the 2 lines, reporters still can see suspends after boot/resume.
That's because of systemd implementation.
It contains code logic that:
When the lid state is closed, a re-checking mechanism is installed.
So if we do not send any notification after boot/resume and the old lid state is "closed".
systemd determines to suspend in the re-checking mechanism.
>
> > Link 1: https://lkml.org/2016/3/7/460
> > Link 2: https://github.com/systemd/systemd/issues/2087
> > Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> > https://bugzilla.kernel.org/show_bug.cgi?id=106151
> > https://bugzilla.kernel.org/show_bug.cgi?id=106941
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Cc: Bastien Nocera: <hadess@hadess.net>
> > ---
> > drivers/acpi/button.c | 63
> +++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 59 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 5c3b091..bb14ca5 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -53,6 +53,10 @@
> > #define ACPI_BUTTON_DEVICE_NAME_LID "Lid Switch"
> > #define ACPI_BUTTON_TYPE_LID 0x05
> >
> > +#define ACPI_BUTTON_LID_INIT_IGNORE 0x00
> > +#define ACPI_BUTTON_LID_INIT_OPEN 0x01
> > +#define ACPI_BUTTON_LID_INIT_METHOD 0x02
> > +
> > #define _COMPONENT ACPI_BUTTON_COMPONENT
> > ACPI_MODULE_NAME("button");
> >
> > @@ -105,6 +109,7 @@ struct acpi_button {
> >
> > static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> > static struct acpi_device *lid_device;
> > +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> >
> > /* --------------------------------------------------------------------------
> > FS Interface (/proc)
> > @@ -246,7 +251,8 @@ int acpi_lid_open(void)
> > }
> > EXPORT_SYMBOL(acpi_lid_open);
> >
> > -static int acpi_lid_send_state(struct acpi_device *device)
> > +static int acpi_lid_send_state(struct acpi_device *device,
> > + bool notify_init_state)
> > {
> > struct acpi_button *button = acpi_driver_data(device);
> > unsigned long long state;
> > @@ -257,6 +263,10 @@ static int acpi_lid_send_state(struct acpi_device
> *device)
> > if (ACPI_FAILURE(status))
> > return -ENODEV;
> >
> > + if (notify_init_state &&
> > + lid_init_state == ACPI_BUTTON_LID_INIT_OPEN)
> > + state = 1;
> > +
>
> Why do we need to complicate this function?
>
> Can't we have a separate function for sending the fake "lid open" event?
[Lv Zheng]
Yes, we can.
But I put the code here for reasons.
I intentionally kept the _LID evaluation right after boot/resume.
Because I validated Windows behavior.
It seems Windows evaluates _LID right after boot.
So I kept _LID evaluated right after boot to prevent compliance issues.
>
> > /* input layer checks if event is redundant */
> > input_report_switch(button->input, SW_LID, !state);
> > input_sync(button->input);
> > @@ -278,6 +288,13 @@ static int acpi_lid_send_state(struct acpi_device
> *device)
> > return ret;
> > }
> >
> > +static int acpi_lid_send_init_state(struct acpi_device *device)
> > +{
> > + if (lid_init_state != ACPI_BUTTON_LID_INIT_IGNORE)
> > + return acpi_lid_send_state(device, true);
> > + return 0;
> > +}
> > +
> > static void acpi_button_notify(struct acpi_device *device, u32 event)
> > {
> > struct acpi_button *button = acpi_driver_data(device);
> > @@ -290,7 +307,7 @@ 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) {
> > - acpi_lid_send_state(device);
> > + acpi_lid_send_state(device, false);
>
> I wouldn't change this code at all.
>
> > } else {
> > int keycode;
> >
> > @@ -335,7 +352,7 @@ static int acpi_button_resume(struct device *dev)
> >
> > button->suspended = false;
> > if (button->type == ACPI_BUTTON_TYPE_LID)
> > - return acpi_lid_send_state(device);
> > + return acpi_lid_send_init_state(device);
> > return 0;
> > }
> > #endif
> > @@ -416,7 +433,7 @@ static int acpi_button_add(struct acpi_device *device)
> > if (error)
> > goto err_remove_fs;
> > if (button->type == ACPI_BUTTON_TYPE_LID) {
> > - acpi_lid_send_state(device);
> > + acpi_lid_send_init_state(device);
> > /*
> > * This assumes there's only one lid device, or if there are
> > * more we only care about the last one...
> > @@ -446,4 +463,42 @@ static int acpi_button_remove(struct acpi_device
> *device)
> > return 0;
> > }
> >
> > +static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
> > +{
> > + int result = 0;
> > +
> > + if (!strncmp(val, "open", sizeof("open") - 1)) {
> > + lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > + pr_info("Notify initial lid state as open\n");
> > + } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> > + lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > + pr_info("Notify initial lid state with _LID return value\n");
> > + } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> > + lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> > + pr_info("Do not notify initial lid state\n");
> > + } else
> > + result = -EINVAL;
> > + return result;
> > +}
> > +
> > +static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
> > +{
> > + switch (lid_init_state) {
> > + case ACPI_BUTTON_LID_INIT_OPEN:
> > + return sprintf(buffer, "open");
> > + case ACPI_BUTTON_LID_INIT_METHOD:
> > + return sprintf(buffer, "method");
> > + case ACPI_BUTTON_LID_INIT_IGNORE:
> > + return sprintf(buffer, "ignore");
> > + default:
> > + return sprintf(buffer, "invalid");
> > + }
> > + return 0;
> > +}
> > +
> > +module_param_call(lid_init_state,
> > + param_set_lid_init_state, param_get_lid_init_state,
> > + NULL, 0644);
> > +MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial
> state");
> > +
>
> I'm not seeing a particular value in having this command line switch
> to be honest. Apparently, the issue can be worked around from user
> space in any case, so why do we need one more way to work around it?
>
> > module_acpi_driver(acpi_button_driver);
> > --
>
> The main concern is general, though. Evidently, we send fake lid
> input events to user space on init and resume. I don't think this is
> a good idea, because it may confuse systems like Chrome that want to
> implement "dark resume" scenarios and may rely on input events to
> decide whether to start a UI or suspend again.
>
> Thus it might be better to simply drop the sending of those fake input
> events from the code.
[Lv Zheng]
If we did this right now, many other userspace could be broken.
So we prepared the options to allow users to choose.
Thanks and best regards
-Lv
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-05-19 01:00 +0200 |
| Message-ID | <rAiBA-8gC-31@gated-at.bofh.it> |
| In reply to | #1402688 |
On Wed, May 18, 2016 at 3:25 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Rafael
>
> Thanks for the review.
>
>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
>> Rafael J. Wysocki
>> Sent: Wednesday, May 18, 2016 7:37 AM
>> To: Zheng, Lv <lv.zheng@intel.com>
>> Cc: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Rafael J. Wysocki
>> <rjw@rjwysocki.net>; Brown, Len <len.brown@intel.com>; Lv Zheng
>> <zetalog@gmail.com>; Linux Kernel Mailing List <linux-
>> kernel@vger.kernel.org>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>;
>> Bastien Nocera: <hadess@hadess.net>
>> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
>> boot/resume
>>
>> On Tue, May 17, 2016 at 10:27 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > (This patch hasn't been tested, it's sent for comments)
>>
>> I have a couple of concerns (see below).
>>
>> > Linux userspace (systemd-logind) keeps on rechecking lid state when the
>> > lid state is closed. If it failed to update the lid state to open after
>> > boot/resume, it could suspend the system. But as:
>> > 1. Traditional ACPI platform may not generate the lid open event after
>> > resuming as the open event is actually handled by the BIOS and the system
>> > is then resumed from a FACS vector.
>> > 2. The _LID control method's initial returning value is not reliable. The
>> > _LID control method is described to return the "current" lid state,
>> > however the word of "current" has ambiguity, many BIOSen return lid
>> > state upon last lid notification while the developers may think the
>> > BIOSen should always return the lid state upon last _LID evaluation.
>> > There won't be difference when we evaluate _LID during the runtime, the
>> > problem is the initial returning value of this function. When the BIOSen
>> > implement this control method with cached value, the initial returning
>> > value is likely not reliable.
>> > Thus there is no mean for the ACPI lid driver to provide such an event
>> > conveying correct current lid state. When there is no such an event or the
>> > event conveys wrong result, false suspending can be examined.
>> >
>> > The root cause of the issue is systemd itself, it could handle the ACPI
>> > control method lid device by implementing a special option like
>> > LidSwitchLevelTriggered=False when it detected the ACPI lid device. However
>> > there is no explicit documentation clarified the ambiguity, we need to
>> > work it around in the kernel before systemd changing its mind.
>>
>> The above doesn't explain how the issue is addressed here.
> [Lv Zheng]
> The story is a bit long.
> We can see several issues that some platform suspends right after boot/resume.
> We noticed that on that platforms, _LID is always implemented with cached lid state returned.
> And it's initial returning value may be "closed" after boot/resume.
>
> It appears the acpi_lid_send_state() sent after boot/resume is the culprit to report the wrong lid state to the userspace.
> But to our surprise, after delete the 2 lines, reporters still can see suspends after boot/resume.
> That's because of systemd implementation.
> It contains code logic that:
> When the lid state is closed, a re-checking mechanism is installed.
> So if we do not send any notification after boot/resume and the old lid state is "closed".
> systemd determines to suspend in the re-checking mechanism.
If that really is the case, it is plain silly and I don't think we can
do anything in the kernel to help here.
>>
>> > Link 1: https://lkml.org/2016/3/7/460
>> > Link 2: https://github.com/systemd/systemd/issues/2087
>> > Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211
>> > https://bugzilla.kernel.org/show_bug.cgi?id=106151
>> > https://bugzilla.kernel.org/show_bug.cgi?id=106941
>> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
>> > Cc: Bastien Nocera: <hadess@hadess.net>
>> > ---
>> > drivers/acpi/button.c | 63
>> +++++++++++++++++++++++++++++++++++++++++++++----
>> > 1 file changed, 59 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
>> > index 5c3b091..bb14ca5 100644
>> > --- a/drivers/acpi/button.c
>> > +++ b/drivers/acpi/button.c
>> > @@ -53,6 +53,10 @@
>> > #define ACPI_BUTTON_DEVICE_NAME_LID "Lid Switch"
>> > #define ACPI_BUTTON_TYPE_LID 0x05
>> >
>> > +#define ACPI_BUTTON_LID_INIT_IGNORE 0x00
>> > +#define ACPI_BUTTON_LID_INIT_OPEN 0x01
>> > +#define ACPI_BUTTON_LID_INIT_METHOD 0x02
>> > +
>> > #define _COMPONENT ACPI_BUTTON_COMPONENT
>> > ACPI_MODULE_NAME("button");
>> >
>> > @@ -105,6 +109,7 @@ struct acpi_button {
>> >
>> > static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>> > static struct acpi_device *lid_device;
>> > +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
>> >
>> > /* --------------------------------------------------------------------------
>> > FS Interface (/proc)
>> > @@ -246,7 +251,8 @@ int acpi_lid_open(void)
>> > }
>> > EXPORT_SYMBOL(acpi_lid_open);
>> >
>> > -static int acpi_lid_send_state(struct acpi_device *device)
>> > +static int acpi_lid_send_state(struct acpi_device *device,
>> > + bool notify_init_state)
>> > {
>> > struct acpi_button *button = acpi_driver_data(device);
>> > unsigned long long state;
>> > @@ -257,6 +263,10 @@ static int acpi_lid_send_state(struct acpi_device
>> *device)
>> > if (ACPI_FAILURE(status))
>> > return -ENODEV;
>> >
>> > + if (notify_init_state &&
>> > + lid_init_state == ACPI_BUTTON_LID_INIT_OPEN)
>> > + state = 1;
>> > +
>>
>> Why do we need to complicate this function?
>>
>> Can't we have a separate function for sending the fake "lid open" event?
> [Lv Zheng]
>
> Yes, we can.
> But I put the code here for reasons.
>
> I intentionally kept the _LID evaluation right after boot/resume.
> Because I validated Windows behavior.
> It seems Windows evaluates _LID right after boot.
> So I kept _LID evaluated right after boot to prevent compliance issues.
I don't quite see what compliance issues could result from skipping
the _LID evaluation after boot.
>>
>> > /* input layer checks if event is redundant */
>> > input_report_switch(button->input, SW_LID, !state);
>> > input_sync(button->input);
>> > @@ -278,6 +288,13 @@ static int acpi_lid_send_state(struct acpi_device
>> *device)
>> > return ret;
>> > }
>> >
>> > +static int acpi_lid_send_init_state(struct acpi_device *device)
>> > +{
>> > + if (lid_init_state != ACPI_BUTTON_LID_INIT_IGNORE)
>> > + return acpi_lid_send_state(device, true);
>> > + return 0;
>> > +}
>> > +
>> > static void acpi_button_notify(struct acpi_device *device, u32 event)
>> > {
>> > struct acpi_button *button = acpi_driver_data(device);
>> > @@ -290,7 +307,7 @@ 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) {
>> > - acpi_lid_send_state(device);
>> > + acpi_lid_send_state(device, false);
>>
>> I wouldn't change this code at all.
>>
>> > } else {
>> > int keycode;
>> >
>> > @@ -335,7 +352,7 @@ static int acpi_button_resume(struct device *dev)
>> >
>> > button->suspended = false;
>> > if (button->type == ACPI_BUTTON_TYPE_LID)
>> > - return acpi_lid_send_state(device);
>> > + return acpi_lid_send_init_state(device);
>> > return 0;
>> > }
>> > #endif
>> > @@ -416,7 +433,7 @@ static int acpi_button_add(struct acpi_device *device)
>> > if (error)
>> > goto err_remove_fs;
>> > if (button->type == ACPI_BUTTON_TYPE_LID) {
>> > - acpi_lid_send_state(device);
>> > + acpi_lid_send_init_state(device);
>> > /*
>> > * This assumes there's only one lid device, or if there are
>> > * more we only care about the last one...
>> > @@ -446,4 +463,42 @@ static int acpi_button_remove(struct acpi_device
>> *device)
>> > return 0;
>> > }
>> >
>> > +static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
>> > +{
>> > + int result = 0;
>> > +
>> > + if (!strncmp(val, "open", sizeof("open") - 1)) {
>> > + lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
>> > + pr_info("Notify initial lid state as open\n");
>> > + } else if (!strncmp(val, "method", sizeof("method") - 1)) {
>> > + lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
>> > + pr_info("Notify initial lid state with _LID return value\n");
>> > + } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
>> > + lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
>> > + pr_info("Do not notify initial lid state\n");
>> > + } else
>> > + result = -EINVAL;
>> > + return result;
>> > +}
>> > +
>> > +static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
>> > +{
>> > + switch (lid_init_state) {
>> > + case ACPI_BUTTON_LID_INIT_OPEN:
>> > + return sprintf(buffer, "open");
>> > + case ACPI_BUTTON_LID_INIT_METHOD:
>> > + return sprintf(buffer, "method");
>> > + case ACPI_BUTTON_LID_INIT_IGNORE:
>> > + return sprintf(buffer, "ignore");
>> > + default:
>> > + return sprintf(buffer, "invalid");
>> > + }
>> > + return 0;
>> > +}
>> > +
>> > +module_param_call(lid_init_state,
>> > + param_set_lid_init_state, param_get_lid_init_state,
>> > + NULL, 0644);
>> > +MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial
>> state");
>> > +
>>
>> I'm not seeing a particular value in having this command line switch
>> to be honest. Apparently, the issue can be worked around from user
>> space in any case, so why do we need one more way to work around it?
>>
>> > module_acpi_driver(acpi_button_driver);
>> > --
>>
>> The main concern is general, though. Evidently, we send fake lid
>> input events to user space on init and resume. I don't think this is
>> a good idea, because it may confuse systems like Chrome that want to
>> implement "dark resume" scenarios and may rely on input events to
>> decide whether to start a UI or suspend again.
>>
>> Thus it might be better to simply drop the sending of those fake input
>> events from the code.
> [Lv Zheng]
> If we did this right now, many other userspace could be broken.
> So we prepared the options to allow users to choose.
Do we have any evidence that any other user space stacks are affected?
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-19 04:00 +0200 |
| Subject | RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rAlpM-1Ad-3@gated-at.bofh.it> |
| In reply to | #1403336 |
Hi,
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
> boot/resume
>
> On Wed, May 18, 2016 at 3:25 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi, Rafael
> >
> > Thanks for the review.
> >
> >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> >> Rafael J. Wysocki
> >> Sent: Wednesday, May 18, 2016 7:37 AM
> >> To: Zheng, Lv <lv.zheng@intel.com>
> >> Cc: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Rafael J. Wysocki
> >> <rjw@rjwysocki.net>; Brown, Len <len.brown@intel.com>; Lv Zheng
> >> <zetalog@gmail.com>; Linux Kernel Mailing List <linux-
> >> kernel@vger.kernel.org>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>;
> >> Bastien Nocera: <hadess@hadess.net>
> >> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
> >> boot/resume
> >>
> >> On Tue, May 17, 2016 at 10:27 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> >> > (This patch hasn't been tested, it's sent for comments)
> >>
> >> I have a couple of concerns (see below).
> >>
> >> > Linux userspace (systemd-logind) keeps on rechecking lid state when the
> >> > lid state is closed. If it failed to update the lid state to open after
> >> > boot/resume, it could suspend the system. But as:
> >> > 1. Traditional ACPI platform may not generate the lid open event after
> >> > resuming as the open event is actually handled by the BIOS and the
> system
> >> > is then resumed from a FACS vector.
> >> > 2. The _LID control method's initial returning value is not reliable. The
> >> > _LID control method is described to return the "current" lid state,
> >> > however the word of "current" has ambiguity, many BIOSen return lid
> >> > state upon last lid notification while the developers may think the
> >> > BIOSen should always return the lid state upon last _LID evaluation.
> >> > There won't be difference when we evaluate _LID during the runtime,
> the
> >> > problem is the initial returning value of this function. When the BIOSen
> >> > implement this control method with cached value, the initial returning
> >> > value is likely not reliable.
> >> > Thus there is no mean for the ACPI lid driver to provide such an event
> >> > conveying correct current lid state. When there is no such an event or the
> >> > event conveys wrong result, false suspending can be examined.
> >> >
> >> > The root cause of the issue is systemd itself, it could handle the ACPI
> >> > control method lid device by implementing a special option like
> >> > LidSwitchLevelTriggered=False when it detected the ACPI lid device.
> However
> >> > there is no explicit documentation clarified the ambiguity, we need to
> >> > work it around in the kernel before systemd changing its mind.
> >>
> >> The above doesn't explain how the issue is addressed here.
> > [Lv Zheng]
> > The story is a bit long.
> > We can see several issues that some platform suspends right after
> boot/resume.
> > We noticed that on that platforms, _LID is always implemented with cached
> lid state returned.
> > And it's initial returning value may be "closed" after boot/resume.
> >
> > It appears the acpi_lid_send_state() sent after boot/resume is the culprit to
> report the wrong lid state to the userspace.
> > But to our surprise, after delete the 2 lines, reporters still can see suspends
> after boot/resume.
> > That's because of systemd implementation.
> > It contains code logic that:
> > When the lid state is closed, a re-checking mechanism is installed.
> > So if we do not send any notification after boot/resume and the old lid state
> is "closed".
> > systemd determines to suspend in the re-checking mechanism.
>
> If that really is the case, it is plain silly and I don't think we can
> do anything in the kernel to help here.
[Lv Zheng]
The problem is:
If we just removed the 2 lines sending wrong lid state after boot/resume.
Problem couldn't be solved.
It could only be solved by changing both the systemd and the kernel (deleting the 2 lines).
>
> >>
> >> > Link 1: https://lkml.org/2016/3/7/460
> >> > Link 2: https://github.com/systemd/systemd/issues/2087
> >> > Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=106151
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=106941
> >> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> >> > Cc: Bastien Nocera: <hadess@hadess.net>
> >> > ---
> >> > drivers/acpi/button.c | 63
> >> +++++++++++++++++++++++++++++++++++++++++++++----
> >> > 1 file changed, 59 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> >> > index 5c3b091..bb14ca5 100644
> >> > --- a/drivers/acpi/button.c
> >> > +++ b/drivers/acpi/button.c
> >> > @@ -53,6 +53,10 @@
> >> > #define ACPI_BUTTON_DEVICE_NAME_LID "Lid Switch"
> >> > #define ACPI_BUTTON_TYPE_LID 0x05
> >> >
> >> > +#define ACPI_BUTTON_LID_INIT_IGNORE 0x00
> >> > +#define ACPI_BUTTON_LID_INIT_OPEN 0x01
> >> > +#define ACPI_BUTTON_LID_INIT_METHOD 0x02
> >> > +
> >> > #define _COMPONENT ACPI_BUTTON_COMPONENT
> >> > ACPI_MODULE_NAME("button");
> >> >
> >> > @@ -105,6 +109,7 @@ struct acpi_button {
> >> >
> >> > static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> >> > static struct acpi_device *lid_device;
> >> > +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> >> >
> >> > /* --------------------------------------------------------------------------
> >> > FS Interface (/proc)
> >> > @@ -246,7 +251,8 @@ int acpi_lid_open(void)
> >> > }
> >> > EXPORT_SYMBOL(acpi_lid_open);
> >> >
> >> > -static int acpi_lid_send_state(struct acpi_device *device)
> >> > +static int acpi_lid_send_state(struct acpi_device *device,
> >> > + bool notify_init_state)
> >> > {
> >> > struct acpi_button *button = acpi_driver_data(device);
> >> > unsigned long long state;
> >> > @@ -257,6 +263,10 @@ static int acpi_lid_send_state(struct acpi_device
> >> *device)
> >> > if (ACPI_FAILURE(status))
> >> > return -ENODEV;
> >> >
> >> > + if (notify_init_state &&
> >> > + lid_init_state == ACPI_BUTTON_LID_INIT_OPEN)
> >> > + state = 1;
> >> > +
> >>
> >> Why do we need to complicate this function?
> >>
> >> Can't we have a separate function for sending the fake "lid open" event?
> > [Lv Zheng]
> >
> > Yes, we can.
> > But I put the code here for reasons.
> >
> > I intentionally kept the _LID evaluation right after boot/resume.
> > Because I validated Windows behavior.
> > It seems Windows evaluates _LID right after boot.
> > So I kept _LID evaluated right after boot to prevent compliance issues.
>
> I don't quite see what compliance issues could result from skipping
> the _LID evaluation after boot.
[Lv Zheng]
I'm not sure if there is a platform putting named object initialization code in _LID.
If you don't like it, we can stop evaluating _LID in the next version.
>
> >>
> >> > /* input layer checks if event is redundant */
> >> > input_report_switch(button->input, SW_LID, !state);
> >> > input_sync(button->input);
> >> > @@ -278,6 +288,13 @@ static int acpi_lid_send_state(struct acpi_device
> >> *device)
> >> > return ret;
> >> > }
> >> >
> >> > +static int acpi_lid_send_init_state(struct acpi_device *device)
> >> > +{
> >> > + if (lid_init_state != ACPI_BUTTON_LID_INIT_IGNORE)
> >> > + return acpi_lid_send_state(device, true);
> >> > + return 0;
> >> > +}
> >> > +
> >> > static void acpi_button_notify(struct acpi_device *device, u32 event)
> >> > {
> >> > struct acpi_button *button = acpi_driver_data(device);
> >> > @@ -290,7 +307,7 @@ 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) {
> >> > - acpi_lid_send_state(device);
> >> > + acpi_lid_send_state(device, false);
> >>
> >> I wouldn't change this code at all.
> >>
> >> > } else {
> >> > int keycode;
> >> >
> >> > @@ -335,7 +352,7 @@ static int acpi_button_resume(struct device *dev)
> >> >
> >> > button->suspended = false;
> >> > if (button->type == ACPI_BUTTON_TYPE_LID)
> >> > - return acpi_lid_send_state(device);
> >> > + return acpi_lid_send_init_state(device);
> >> > return 0;
> >> > }
> >> > #endif
> >> > @@ -416,7 +433,7 @@ static int acpi_button_add(struct acpi_device
> *device)
> >> > if (error)
> >> > goto err_remove_fs;
> >> > if (button->type == ACPI_BUTTON_TYPE_LID) {
> >> > - acpi_lid_send_state(device);
> >> > + acpi_lid_send_init_state(device);
> >> > /*
> >> > * This assumes there's only one lid device, or if there are
> >> > * more we only care about the last one...
> >> > @@ -446,4 +463,42 @@ static int acpi_button_remove(struct acpi_device
> >> *device)
> >> > return 0;
> >> > }
> >> >
> >> > +static int param_set_lid_init_state(const char *val, struct kernel_param
> *kp)
> >> > +{
> >> > + int result = 0;
> >> > +
> >> > + if (!strncmp(val, "open", sizeof("open") - 1)) {
> >> > + lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> >> > + pr_info("Notify initial lid state as open\n");
> >> > + } else if (!strncmp(val, "method", sizeof("method") - 1)) {
> >> > + lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> >> > + pr_info("Notify initial lid state with _LID return value\n");
> >> > + } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> >> > + lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> >> > + pr_info("Do not notify initial lid state\n");
> >> > + } else
> >> > + result = -EINVAL;
> >> > + return result;
> >> > +}
> >> > +
> >> > +static int param_get_lid_init_state(char *buffer, struct kernel_param *kp)
> >> > +{
> >> > + switch (lid_init_state) {
> >> > + case ACPI_BUTTON_LID_INIT_OPEN:
> >> > + return sprintf(buffer, "open");
> >> > + case ACPI_BUTTON_LID_INIT_METHOD:
> >> > + return sprintf(buffer, "method");
> >> > + case ACPI_BUTTON_LID_INIT_IGNORE:
> >> > + return sprintf(buffer, "ignore");
> >> > + default:
> >> > + return sprintf(buffer, "invalid");
> >> > + }
> >> > + return 0;
> >> > +}
> >> > +
> >> > +module_param_call(lid_init_state,
> >> > + param_set_lid_init_state, param_get_lid_init_state,
> >> > + NULL, 0644);
> >> > +MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial
> >> state");
> >> > +
> >>
> >> I'm not seeing a particular value in having this command line switch
> >> to be honest. Apparently, the issue can be worked around from user
> >> space in any case, so why do we need one more way to work around it?
> >>
> >> > module_acpi_driver(acpi_button_driver);
> >> > --
> >>
> >> The main concern is general, though. Evidently, we send fake lid
> >> input events to user space on init and resume. I don't think this is
> >> a good idea, because it may confuse systems like Chrome that want to
> >> implement "dark resume" scenarios and may rely on input events to
> >> decide whether to start a UI or suspend again.
> >>
> >> Thus it might be better to simply drop the sending of those fake input
> >> events from the code.
> > [Lv Zheng]
> > If we did this right now, many other userspace could be broken.
> > So we prepared the options to allow users to choose.
>
> Do we have any evidence that any other user space stacks are affected?
[Lv Zheng]
I didn't know any of such affections except the systemd.
Thanks and best regards
-Lv
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-05-19 15:30 +0200 |
| Message-ID | <rAwbw-kQ-19@gated-at.bofh.it> |
| In reply to | #1403388 |
On Thu, May 19, 2016 at 3:50 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > Hi, > >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of >> Rafael J. Wysocki >> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after >> boot/resume [cut] >> > That's because of systemd implementation. >> > It contains code logic that: >> > When the lid state is closed, a re-checking mechanism is installed. >> > So if we do not send any notification after boot/resume and the old lid state >> is "closed". >> > systemd determines to suspend in the re-checking mechanism. >> >> If that really is the case, it is plain silly and I don't think we can >> do anything in the kernel to help here. > > [Lv Zheng] > The problem is: > If we just removed the 2 lines sending wrong lid state after boot/resume. > Problem couldn't be solved. > It could only be solved by changing both the systemd and the kernel (deleting the 2 lines). There are two things here, there's a kernel issue (sending the fake input events) and there's a user-visible problem. Yes, it may not be possible to fix the user-visible problem by fixing the kernel issue alone, but pretty much by definition we can only fix the kernel issue in the kernel. However, it looks like it may not be possible to fix the user-visible problem without fixing the kernel issue in the first place, so maybe we should do that and attach the additional user space patch to the bug entries in question? [cut] >> > I intentionally kept the _LID evaluation right after boot/resume. >> > Because I validated Windows behavior. >> > It seems Windows evaluates _LID right after boot. >> > So I kept _LID evaluated right after boot to prevent compliance issues. >> >> I don't quite see what compliance issues could result from skipping >> the _LID evaluation after boot. > > [Lv Zheng] > I'm not sure if there is a platform putting named object initialization code in _LID. > If you don't like it, we can stop evaluating _LID in the next version. Well, unless there is a well-documented reason for doing this, I'd at least try to see what happens if we don't. Doing things for unspecified reasons is not a very good idea overall IMO. [cut] >> > [Lv Zheng] >> > If we did this right now, many other userspace could be broken. >> > So we prepared the options to allow users to choose. >> >> Do we have any evidence that any other user space stacks are affected? > > [Lv Zheng] > I didn't know any of such affections except the systemd. So let's focus on this one until we actually find another example.
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@gmail.com> |
|---|---|
| Date | 2016-05-26 15:40 +0200 |
| Subject | Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rD3G2-6ZR-5@gated-at.bofh.it> |
| In reply to | #1403738 |
[Jumping in the discussion at Bastien's request]
On Thu, May 19, 2016 at 3:21 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, May 19, 2016 at 3:50 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> Hi,
>>
>>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
>>> Rafael J. Wysocki
>>> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
>>> boot/resume
>
> [cut]
>
>>> > That's because of systemd implementation.
>>> > It contains code logic that:
>>> > When the lid state is closed, a re-checking mechanism is installed.
>>> > So if we do not send any notification after boot/resume and the old lid state
>>> is "closed".
>>> > systemd determines to suspend in the re-checking mechanism.
>>>
>>> If that really is the case, it is plain silly and I don't think we can
>>> do anything in the kernel to help here.
>>
>> [Lv Zheng]
>> The problem is:
>> If we just removed the 2 lines sending wrong lid state after boot/resume.
>> Problem couldn't be solved.
>> It could only be solved by changing both the systemd and the kernel (deleting the 2 lines).
>
> There are two things here, there's a kernel issue (sending the fake
> input events) and there's a user-visible problem. Yes, it may not be
> possible to fix the user-visible problem by fixing the kernel issue
> alone, but pretty much by definition we can only fix the kernel issue
> in the kernel.
>
> However, it looks like it may not be possible to fix the user-visible
> problem without fixing the kernel issue in the first place, so maybe
> we should do that and attach the additional user space patch to the
> bug entries in question?
>
> [cut]
>
>>> > I intentionally kept the _LID evaluation right after boot/resume.
>>> > Because I validated Windows behavior.
>>> > It seems Windows evaluates _LID right after boot.
>>> > So I kept _LID evaluated right after boot to prevent compliance issues.
>>>
>>> I don't quite see what compliance issues could result from skipping
>>> the _LID evaluation after boot.
>>
>> [Lv Zheng]
>> I'm not sure if there is a platform putting named object initialization code in _LID.
>> If you don't like it, we can stop evaluating _LID in the next version.
>
> Well, unless there is a well-documented reason for doing this, I'd at
> least try to see what happens if we don't.
>
> Doing things for unspecified reasons is not a very good idea overall IMO.
I found an issue on the surface 3 which explains why the initial state
of the _LID switch is wrong.
In gpiolib-acpi, we initialize an operation region for the LID switch
to be controlled by a GPIO. This GPIO triggers an _E4C method when
changed (see https://bugzilla.kernel.org/attachment.cgi?id=187171 in
GPO0). This GPIO event actually sets the correct initial state of LIDB,
which is forwarded by _LID.
Now, on the surface 3, there is an other gpio event (_E10) which, when
triggered at boot seems to put the sensor hub (over i2c-hid) in a
better shape:
I used to receive a5 a5 a5 a5 a5.. (garbage) after enabling S0 on the
sensor and when requesting data from it. Now I get a nice
[ +0.000137] i2c_hid i2c-MSHW0102:00: report (len=17): 11 00 01 02 05 00 00 00 00 00 00 00 00 00 18 fc 00
which seems more sensible from a HID point of view.
The patch is the following:
---
From 2c76d14a5ad089d0321a029edde3f91f3bc93ae3 Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date: Thu, 26 May 2016 15:29:10 +0200
Subject: [PATCH] gpiolib-acpi: make sure we trigger the events at least once
on boot
The Surface 3 has its _LID state controlled by an ACPI operation region
triggered by a GPIO event:
OperationRegion (GPOR, GeneralPurposeIo, Zero, One)
Field (GPOR, ByteAcc, NoLock, Preserve)
{
Connection (
GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone,
"\\_SB.GPO0", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x004C
}
),
HELD, 1
}
Method (_E4C, 0, Serialized) // _Exx: Edge-Triggered GPE
{
If ((HELD == One))
{
^^LID.LIDB = One
}
Else
{
^^LID.LIDB = Zero
Notify (LID, 0x80) // Status Change
}
Notify (^^PCI0.SPI1.NTRG, One) // Device Check
}
Currently, the state of LIDB is wrong until the user actually closes or
open the cover. We need to trigger the GPIO event once to update the
internal ACPI state.
Coincidentally, this also enables the integrated HID sensor hub which also
requires an ACPI gpio operation region to start initialization.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/gpio/gpiolib-acpi.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 2dc5258..71775a0 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -175,7 +175,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
irq_handler_t handler = NULL;
struct gpio_desc *desc;
unsigned long irqflags;
- int ret, pin, irq;
+ int ret, pin, irq, value;
if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
return AE_OK;
@@ -214,6 +214,8 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
gpiod_direction_input(desc);
+ value = gpiod_get_value(desc);
+
ret = gpiochip_lock_as_irq(chip, pin);
if (ret) {
dev_err(chip->parent, "Failed to lock GPIO as interrupt\n");
@@ -266,6 +268,15 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
}
list_add_tail(&event->node, &acpi_gpio->events);
+
+ /*
+ * Make sure we trigger the initial state of the IRQ when
+ * using RISING or FALLING.
+ */
+ if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
+ ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
+ handler(-1, event);
+
return AE_OK;
fail_free_event:
--
2.5.0
---
Now, if I am not mistaken, we could simply check the value of _LID on
resume and if it differs from the previous state, force an input_event
from the _LID. That should at least re-trigger the LID close event
(sent by the ACPI) for the next attempt.
Cheers,
Benjamin
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-30 03:50 +0200 |
| Subject | RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rEkv7-5U5-9@gated-at.bofh.it> |
| In reply to | #1407562 |
Hi,
> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
> boot/resume
>
> [Jumping in the discussion at Bastien's request]
>
> On Thu, May 19, 2016 at 3:21 PM, Rafael J. Wysocki <rafael@kernel.org>
> wrote:
> > On Thu, May 19, 2016 at 3:50 AM, Zheng, Lv <lv.zheng@intel.com>
> wrote:
> >> Hi,
> >>
> >>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf
> Of
> >>> Rafael J. Wysocki
> >>> Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after
> >>> boot/resume
> >
> > [cut]
> >
> >>> > That's because of systemd implementation.
> >>> > It contains code logic that:
> >>> > When the lid state is closed, a re-checking mechanism is installed.
> >>> > So if we do not send any notification after boot/resume and the old
> lid state
> >>> is "closed".
> >>> > systemd determines to suspend in the re-checking mechanism.
> >>>
> >>> If that really is the case, it is plain silly and I don't think we can
> >>> do anything in the kernel to help here.
> >>
> >> [Lv Zheng]
> >> The problem is:
> >> If we just removed the 2 lines sending wrong lid state after
> boot/resume.
> >> Problem couldn't be solved.
> >> It could only be solved by changing both the systemd and the kernel
> (deleting the 2 lines).
> >
> > There are two things here, there's a kernel issue (sending the fake
> > input events) and there's a user-visible problem. Yes, it may not be
> > possible to fix the user-visible problem by fixing the kernel issue
> > alone, but pretty much by definition we can only fix the kernel issue
> > in the kernel.
> >
> > However, it looks like it may not be possible to fix the user-visible
> > problem without fixing the kernel issue in the first place, so maybe
> > we should do that and attach the additional user space patch to the
> > bug entries in question?
> >
> > [cut]
> >
> >>> > I intentionally kept the _LID evaluation right after boot/resume.
> >>> > Because I validated Windows behavior.
> >>> > It seems Windows evaluates _LID right after boot.
> >>> > So I kept _LID evaluated right after boot to prevent compliance
> issues.
> >>>
> >>> I don't quite see what compliance issues could result from skipping
> >>> the _LID evaluation after boot.
> >>
> >> [Lv Zheng]
> >> I'm not sure if there is a platform putting named object initialization
> code in _LID.
> >> If you don't like it, we can stop evaluating _LID in the next version.
> >
> > Well, unless there is a well-documented reason for doing this, I'd at
> > least try to see what happens if we don't.
> >
> > Doing things for unspecified reasons is not a very good idea overall IMO.
>
> I found an issue on the surface 3 which explains why the initial state
> of the _LID switch is wrong.
> In gpiolib-acpi, we initialize an operation region for the LID switch
> to be controlled by a GPIO. This GPIO triggers an _E4C method when
> changed (see https://bugzilla.kernel.org/attachment.cgi?id=187171 in
> GPO0). This GPIO event actually sets the correct initial state of LIDB,
> which is forwarded by _LID.
>
> Now, on the surface 3, there is an other gpio event (_E10) which, when
> triggered at boot seems to put the sensor hub (over i2c-hid) in a
> better shape:
> I used to receive a5 a5 a5 a5 a5.. (garbage) after enabling S0 on the
> sensor and when requesting data from it. Now I get a nice
> [ +0.000137] i2c_hid i2c-MSHW0102:00: report (len=17): 11 00 01 02 05
> 00 00 00 00 00 00 00 00 00 18 fc 00
> which seems more sensible from a HID point of view.
[Lv Zheng]
So the problem of Surface 3 is different.
Which is related to the gpio irq.
We still have several issues related to this:
1. https://bugzilla.kernel.org/show_bug.cgi?id=89211
2. https://bugzilla.kernel.org/show_bug.cgi?id=106151
The _LID methods in the 2 cases all use cached value, and there is no lid notification can be seen after resume, causing "close" sent to the userspace.
Maybe we should go back to check if there is a different root cause.
Thanks and best regards
-Lv
>
> The patch is the following:
>
> ---
> From 2c76d14a5ad089d0321a029edde3f91f3bc93ae3 Mon Sep 17
> 00:00:00 2001
> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Date: Thu, 26 May 2016 15:29:10 +0200
> Subject: [PATCH] gpiolib-acpi: make sure we trigger the events at least
> once
> on boot
>
> The Surface 3 has its _LID state controlled by an ACPI operation region
> triggered by a GPIO event:
>
> OperationRegion (GPOR, GeneralPurposeIo, Zero, One)
> Field (GPOR, ByteAcc, NoLock, Preserve)
> {
> Connection (
> GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone,
> "\\_SB.GPO0", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x004C
> }
> ),
> HELD, 1
> }
>
> Method (_E4C, 0, Serialized) // _Exx: Edge-Triggered GPE
> {
> If ((HELD == One))
> {
> ^^LID.LIDB = One
> }
> Else
> {
> ^^LID.LIDB = Zero
> Notify (LID, 0x80) // Status Change
> }
>
> Notify (^^PCI0.SPI1.NTRG, One) // Device Check
> }
>
> Currently, the state of LIDB is wrong until the user actually closes or
> open the cover. We need to trigger the GPIO event once to update the
> internal ACPI state.
>
> Coincidentally, this also enables the integrated HID sensor hub which also
> requires an ACPI gpio operation region to start initialization.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/gpio/gpiolib-acpi.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 2dc5258..71775a0 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -175,7 +175,7 @@ static acpi_status
> acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> irq_handler_t handler = NULL;
> struct gpio_desc *desc;
> unsigned long irqflags;
> - int ret, pin, irq;
> + int ret, pin, irq, value;
>
> if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
> return AE_OK;
> @@ -214,6 +214,8 @@ static acpi_status
> acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
>
> gpiod_direction_input(desc);
>
> + value = gpiod_get_value(desc);
> +
> ret = gpiochip_lock_as_irq(chip, pin);
> if (ret) {
> dev_err(chip->parent, "Failed to lock GPIO as interrupt\n");
> @@ -266,6 +268,15 @@ static acpi_status
> acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> }
>
> list_add_tail(&event->node, &acpi_gpio->events);
> +
> + /*
> + * Make sure we trigger the initial state of the IRQ when
> + * using RISING or FALLING.
> + */
> + if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> + ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> + handler(-1, event);
> +
> return AE_OK;
>
> fail_free_event:
> --
> 2.5.0
>
> ---
>
> Now, if I am not mistaken, we could simply check the value of _LID on
> resume and if it differs from the previous state, force an input_event
> from the _LID. That should at least re-trigger the LID close event
> (sent by the ACPI) for the next attempt.
>
> Cheers,
> Benjamin
[toc] | [prev] | [next] | [standalone]
| From | Bastien Nocera <hadess@hadess.net> |
|---|---|
| Date | 2016-05-18 15:00 +0200 |
| Subject | Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rA9eW-2gz-15@gated-at.bofh.it> |
| In reply to | #1402194 |
On Tue, 2016-05-17 at 16:27 +0800, Lv Zheng wrote: > (This patch hasn't been tested, it's sent for comments) > > Linux userspace (systemd-logind) keeps on rechecking lid state when the > lid state is closed. If it failed to update the lid state to open after > boot/resume, it could suspend the system. But as: > 1. Traditional ACPI platform may not generate the lid open event after > resuming as the open event is actually handled by the BIOS and the system > is then resumed from a FACS vector. > 2. The _LID control method's initial returning value is not reliable. The > _LID control method is described to return the "current" lid state, > however the word of "current" has ambiguity, many BIOSen return lid > state upon last lid notification while the developers may think the > BIOSen should always return the lid state upon last _LID evaluation. > There won't be difference when we evaluate _LID during the runtime, the > problem is the initial returning value of this function. When the BIOSen > implement this control method with cached value, the initial returning > value is likely not reliable. > Thus there is no mean for the ACPI lid driver to provide such an event > conveying correct current lid state. When there is no such an event or the > event conveys wrong result, false suspending can be examined. > > The root cause of the issue is systemd itself, it could handle the ACPI > control method lid device by implementing a special option like > LidSwitchLevelTriggered=False when it detected the ACPI lid device. However > there is no explicit documentation clarified the ambiguity, we need to > work it around in the kernel before systemd changing its mind. > > Link 1: https://lkml.org/2016/3/7/460 > Link 2: https://github.com/systemd/systemd/issues/2087 > Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211 > https://bugzilla.kernel.org/show_bug.cgi?id=106151 > https://bugzilla.kernel.org/show_bug.cgi?id=106941 > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > Cc: Bastien Nocera: <hadess@hadess.net> As mentioned previously, I'd be much happier if either: - a new system was put in place that matched the ACPI specs and the way they are used by Windows - an additional tag/property was added to show that the API offered by the LID switch is different from the one that existing since ACPI support was added to Linux. And I'd really appreciate it if you stopped saying that it's systemd's fault. The kernel has offered an API to user-space that included the state of the LID switch. And the state of the LID switch has been correct for the majority of systems and devices for the past decade or so. The fact that this device means that the LID state isn't reliable anymore means that the kernel needs to communicate that to user-space. I'm volunteering to modify systemd and UPower to respect the fact that the LID switch state isn't available on those devices. If you want to find something to blame, blame the kernel for implementing an API that doesn't reflect what the hardware makes available. Though you can only say that with the benefit of hindsight. So, NAK from me.
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-05-18 23:50 +0200 |
| Message-ID | <rAhvP-7Ao-9@gated-at.bofh.it> |
| In reply to | #1402960 |
On Wed, May 18, 2016 at 2:57 PM, Bastien Nocera <hadess@hadess.net> wrote: > On Tue, 2016-05-17 at 16:27 +0800, Lv Zheng wrote: >> (This patch hasn't been tested, it's sent for comments) >> [cut] > > As mentioned previously, I'd be much happier if either: > - a new system was put in place that matched the ACPI specs and the way > they are used by Windows > - an additional tag/property was added to show that the API offered by > the LID switch is different from the one that existing since ACPI > support was added to Linux. I'm sorry, but I'm not following either of the above. > And I'd really appreciate it if you stopped saying that it's systemd's > fault. > > The kernel has offered an API to user-space that included the state of > the LID switch. To be precise, this is not what the kernel has been providing. It really is the state of the lid switch as reported by the platform firmware. The "as reported by the platform firmware" part appears to be missing from your argumentation and it is quite relevant, because the firmware, being what it is, messes up things at least occasionally, so by using that interface you may end up having to deal with a value that has been messed up by the firmware. Now, the kernel is not entirely innocent here, because it sometimes reports input events that it has not received to user space. That happens during system initialization and during resume from system suspend and is generally problematic, so I'd prefer to get rid of that behavior. That, however, has nothing to do with the spec compliance and/or the Windows' behavior. > And the state of the LID switch has been correct for > the majority of systems and devices for the past decade or so. The fact > that this device means that the LID state isn't reliable anymore means > that the kernel needs to communicate that to user-space. The problem, though, is that the kernel has no idea about the value being incorrect. It just passes the value over to user space and has no way to validate it. > I'm volunteering to modify systemd and UPower to respect the fact that > the LID switch state isn't available on those devices. If you want to > find something to blame, blame the kernel for implementing an API that > doesn't reflect what the hardware makes available. The API works as I described: it passes the value reported by the firmware to user space. I has always worked this way. Whether or not that is a useful API is a good question. I guess, however, that the fake input events reported by the kernel are really problematic here, because had they not been reported, you wouldn't have seen an incorrect lid switch state, most likely.
[toc] | [prev] | [next] | [standalone]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-19 04:00 +0200 |
| Subject | RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rAlpM-1Ad-9@gated-at.bofh.it> |
| In reply to | #1402960 |
Hi, > From: Bastien Nocera [mailto:hadess@hadess.net] > Subject: Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after > boot/resume > > On Tue, 2016-05-17 at 16:27 +0800, Lv Zheng wrote: > > (This patch hasn't been tested, it's sent for comments) > > > > Linux userspace (systemd-logind) keeps on rechecking lid state when the > > lid state is closed. If it failed to update the lid state to open after > > boot/resume, it could suspend the system. But as: > > 1. Traditional ACPI platform may not generate the lid open event after > > resuming as the open event is actually handled by the BIOS and the system > > is then resumed from a FACS vector. > > 2. The _LID control method's initial returning value is not reliable. The > > _LID control method is described to return the "current" lid state, > > however the word of "current" has ambiguity, many BIOSen return lid > > state upon last lid notification while the developers may think the > > BIOSen should always return the lid state upon last _LID evaluation. > > There won't be difference when we evaluate _LID during the runtime, the > > problem is the initial returning value of this function. When the BIOSen > > implement this control method with cached value, the initial returning > > value is likely not reliable. > > Thus there is no mean for the ACPI lid driver to provide such an event > > conveying correct current lid state. When there is no such an event or the > > event conveys wrong result, false suspending can be examined. > > > > The root cause of the issue is systemd itself, it could handle the ACPI > > control method lid device by implementing a special option like > > LidSwitchLevelTriggered=False when it detected the ACPI lid device. However > > there is no explicit documentation clarified the ambiguity, we need to > > work it around in the kernel before systemd changing its mind. > > > > Link 1: https://lkml.org/2016/3/7/460 > > Link 2: https://github.com/systemd/systemd/issues/2087 > > Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=89211 > > https://bugzilla.kernel.org/show_bug.cgi?id=106151 > > https://bugzilla.kernel.org/show_bug.cgi?id=106941 > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > Cc: Bastien Nocera: <hadess@hadess.net> > > As mentioned previously, I'd be much happier if either: > - a new system was put in place that matched the ACPI specs and the way > they are used by Windows [Lv Zheng] Then what about the old systems? Surface Pro 1 and Surface 3 are MS platforms that should have already adapted to the Windows. They are currently suffering from the same issue by running Linux. > - an additional tag/property was added to show that the API offered by > the LID switch is different from the one that existing since ACPI > support was added to Linux. > > And I'd really appreciate it if you stopped saying that it's systemd's > fault. [Lv Zheng] OK. Instead of saying this is a fault, I'll say systemd may need an additional option to handle ACPI lid device. > > The kernel has offered an API to user-space that included the state of > the LID switch. And the state of the LID switch has been correct for > the majority of systems and devices for the past decade or so. The fact > that this device means that the LID state isn't reliable anymore means > that the kernel needs to communicate that to user-space. > > I'm volunteering to modify systemd and UPower to respect the fact that > the LID switch state isn't available on those devices. If you want to > find something to blame, blame the kernel for implementing an API that > doesn't reflect what the hardware makes available. Though you can only > say that with the benefit of hindsight. > > So, NAK from me. [Lv Zheng] I'm out of the context here. This patch is generated to respect the current systemd behavior. If not, we'd rather just delete the 2 wrong lines. I know that you need a documentation clarification, we are putting effort on this. Thanks and best regards -Lv
[toc] | [prev] | [next] | [standalone]
| From | Lv Zheng <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 09:20 +0200 |
| Subject | [PATCH v2 2/3] ACPI / button: Refactor functions to eliminate redundant code |
| Message-ID | <rDkdP-ox-23@gated-at.bofh.it> |
| In reply to | #1402194 |
This patch simplies the code by merging the redundant code. No functional
changes.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/button.c | 92 +++++++++++++++++++++++++++----------------------
1 file changed, 50 insertions(+), 42 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9863278..e706e4b 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -113,16 +113,53 @@ static struct acpi_device *lid_device;
static struct proc_dir_entry *acpi_button_dir;
static struct proc_dir_entry *acpi_lid_dir;
+static int acpi_lid_evaluate_state(struct acpi_device *device)
+{
+ unsigned long long lid_state;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(device->handle, "_LID", NULL, &lid_state);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ return lid_state ? 1 : 0;
+}
+
+static int acpi_lid_notify_state(struct acpi_device *device, int state)
+{
+ struct acpi_button *button = acpi_driver_data(device);
+ int ret;
+
+ /* input layer checks if event is redundant */
+ input_report_switch(button->input, SW_LID, !state);
+ input_sync(button->input);
+
+ if (state)
+ pm_wakeup_event(&device->dev, 0);
+
+ ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
+ if (ret == NOTIFY_DONE)
+ ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
+ device);
+ if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
+ /*
+ * It is also regarded as success if the notifier_chain
+ * returns NOTIFY_OK or NOTIFY_DONE.
+ */
+ ret = 0;
+ }
+ return ret;
+}
+
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;
+ int state;
- status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
+ state = acpi_lid_evaluate_state(device);
seq_printf(seq, "state: %s\n",
- ACPI_FAILURE(status) ? "unsupported" :
- (state ? "open" : "closed"));
+ IS_ERR_VALUE(state) ? "unsupported" :
+ (state ? "open" : "closed"));
return 0;
}
@@ -231,51 +268,22 @@ EXPORT_SYMBOL(acpi_lid_notifier_unregister);
int acpi_lid_open(void)
{
- acpi_status status;
- unsigned long long state;
-
if (!lid_device)
return -ENODEV;
- status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
- &state);
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
- return !!state;
+ return acpi_lid_evaluate_state(lid_device);
}
EXPORT_SYMBOL(acpi_lid_open);
-static int acpi_lid_send_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device)
{
- struct acpi_button *button = acpi_driver_data(device);
- unsigned long long state;
- acpi_status status;
- int ret;
-
- status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
- if (ACPI_FAILURE(status))
- return -ENODEV;
+ int state;
- /* input layer checks if event is redundant */
- input_report_switch(button->input, SW_LID, !state);
- input_sync(button->input);
+ state = acpi_lid_evaluate_state(device);
+ if (IS_ERR_VALUE(state))
+ return state;
- if (state)
- pm_wakeup_event(&device->dev, 0);
-
- ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
- if (ret == NOTIFY_DONE)
- ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
- device);
- if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
- /*
- * It is also regarded as success if the notifier_chain
- * returns NOTIFY_OK or NOTIFY_DONE.
- */
- ret = 0;
- }
- return ret;
+ return acpi_lid_notify_state(device, state);
}
static void acpi_button_notify(struct acpi_device *device, u32 event)
@@ -290,7 +298,7 @@ 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) {
- acpi_lid_send_state(device);
+ acpi_lid_update_state(device);
} else {
int keycode;
--
1.7.10
[toc] | [prev] | [next] | [standalone]
| From | Lv Zheng <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 09:20 +0200 |
| Subject | [PATCH v2 1/3] ACPI / button: Remove initial lid state notification |
| Message-ID | <rDkdP-ox-17@gated-at.bofh.it> |
| In reply to | #1402194 |
The _LID control method's initial returning value is not reliable.
The _LID control method is described to return the "current" lid state.
However the word of "current" has ambiguity, many BIOSen return the lid
state upon the last lid notification instead of returning the lid state
upon the last _LID evaluation. There won't be difference when the _LID
control method is evaluated during the runtime, the problem is its initial
returning value. When the BIOSen implement this control method with cached
value, the initial returning value is likely not reliable. There are simply
so many examples retuning "close" as initial lid state (Link 1), sending
this state to the userspace causes suspending right after booting/resuming.
Since the lid state is implemented by the BIOSen, the kernel lid driver has
no idea how it can be correct, this patch stops sending the initial lid
state to the userspace to try to avoid sending the wrong lid state to the
userspace to trigger such kind of wrong suspending. This actually reverts
the following commit introduced for fixing a novell bug (Link 2):
Commit: 23de5d9ef2a4bbc4f733f58311bcb7cf6239c813
Subject: ACPI: button: send initial lid state after add and resume
Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=89211
https://bugzilla.kernel.org/show_bug.cgi?id=106151
https://bugzilla.kernel.org/show_bug.cgi?id=106941
Link 2: https://bugzilla.novell.com/show_bug.cgi?id=326814
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/button.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 5c3b091..9863278 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -334,8 +334,6 @@ 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)
- return acpi_lid_send_state(device);
return 0;
}
#endif
@@ -416,7 +414,6 @@ static int acpi_button_add(struct acpi_device *device)
if (error)
goto err_remove_fs;
if (button->type == ACPI_BUTTON_TYPE_LID) {
- acpi_lid_send_state(device);
/*
* This assumes there's only one lid device, or if there are
* more we only care about the last one...
--
1.7.10
[toc] | [prev] | [next] | [standalone]
| From | Lv Zheng <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 09:20 +0200 |
| Subject | [PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rDkdP-ox-27@gated-at.bofh.it> |
| In reply to | #1402194 |
Linux userspace (systemd-logind) keeps on rechecking lid state when the
lid state is closed. If it failed to update the lid state to open after
boot/resume, the system suspending right after the boot/resume could be
resulted.
Graphics drivers also uses the lid notifications to implment
MODESET_ON_LID_OPEN option.
Before the situation is improved from the userspace and from the graphics
driver, simply send initial "open" lid state to avoid issues. After this is
improved from the userspace and from the graphics driver, Linux kernel
could simply revert this minimal commit.
Link 1: https://lkml.org/2016/3/7/460
Link 2: https://github.com/systemd/systemd/issues/2087
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Cc: Bastien Nocera: <hadess@hadess.net>
---
drivers/acpi/button.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index e706e4b..6e77312 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -342,6 +342,8 @@ 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)
+ return acpi_lid_notify_state(device, 1);
return 0;
}
#endif
@@ -422,6 +424,7 @@ static int acpi_button_add(struct acpi_device *device)
if (error)
goto err_remove_fs;
if (button->type == ACPI_BUTTON_TYPE_LID) {
+ (void)acpi_lid_notify_state(device, 1);
/*
* This assumes there's only one lid device, or if there are
* more we only care about the last one...
--
1.7.10
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Tissoires <benjamin.tissoires@gmail.com> |
|---|---|
| Date | 2016-05-30 10:20 +0200 |
| Subject | Re: [PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rEqAx-1BB-7@gated-at.bofh.it> |
| In reply to | #1407865 |
On Fri, May 27, 2016 at 9:16 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> Linux userspace (systemd-logind) keeps on rechecking lid state when the
> lid state is closed. If it failed to update the lid state to open after
> boot/resume, the system suspending right after the boot/resume could be
> resulted.
> Graphics drivers also uses the lid notifications to implment
> MODESET_ON_LID_OPEN option.
>
> Before the situation is improved from the userspace and from the graphics
> driver, simply send initial "open" lid state to avoid issues. After this is
> improved from the userspace and from the graphics driver, Linux kernel
> could simply revert this minimal commit.
>
> Link 1: https://lkml.org/2016/3/7/460
> Link 2: https://github.com/systemd/systemd/issues/2087
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> Cc: Bastien Nocera: <hadess@hadess.net>
> ---
> drivers/acpi/button.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index e706e4b..6e77312 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -342,6 +342,8 @@ 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)
> + return acpi_lid_notify_state(device, 1);
As Valdis replied on 0/3, I don't think this is a good solution (even
temporary). Linux should not assume the current state of a input
device, and sending unconditionally 1 here is wrong. If the device is
on a docking station, you will wake up the wrong monitor and screw the
user session (and this will be a regression).
How about we simply send the current LID state stored in the ACPI?
something like calling acpi_lid_send_state() directly?
[15 min later, after writing a long email]
Well, it looks like we already have that in the kernel and for a long
time apparently.
So, I think the issue is that Microsoft does not wake up the system by
lid opening (seen in one of the comments in the mentioned bugs and by
looking at the behavior on the surface devices). It must be just
querying it's state on resume or might even not care at all until it
receives a close event.
If I read correctly, we managed to get the 3 bogus devices to a
correct state at the ACPI level (/proc/acpi/button/lid/LID0/state),
but we did not get the notification. Given that the LID state is
triggered by an ACPI operation region, there is no guarantee that the
resume of the acpi/button.c driver will be called after the region has
been updated/called.
I propose as a workaround to enable a kthread that will monitor the
lid state and update the correct value to userspace (5 sec of polling
time should be enough given that systemd checks every 20 sec).
We should probably have this workaround only for a set of known
devices, as it might just be temporary for those until the actual
underlying problem is fixed (wrong DSDT in the Surface 3 case that
doesn't notify at all, issue in the EC for the Surface Pro 1 and the
Samsung N210).
Cheers,
Benjamin
> return 0;
> }
> #endif
> @@ -422,6 +424,7 @@ static int acpi_button_add(struct acpi_device *device)
> if (error)
> goto err_remove_fs;
> if (button->type == ACPI_BUTTON_TYPE_LID) {
> + (void)acpi_lid_notify_state(device, 1);
> /*
> * This assumes there's only one lid device, or if there are
> * more we only care about the last one...
> --
> 1.7.10
>
> --
> 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]
| From | "Zheng, Lv" <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-31 05:00 +0200 |
| Subject | RE: [PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume |
| Message-ID | <rEI4p-4Qc-1@gated-at.bofh.it> |
| In reply to | #1408845 |
Hi,
> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v2 3/3] ACPI / button: Send "open" state after
> boot/resume
>
> On Fri, May 27, 2016 at 9:16 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > Linux userspace (systemd-logind) keeps on rechecking lid state when the
> > lid state is closed. If it failed to update the lid state to open after
> > boot/resume, the system suspending right after the boot/resume could
> be
> > resulted.
> > Graphics drivers also uses the lid notifications to implment
> > MODESET_ON_LID_OPEN option.
> >
> > Before the situation is improved from the userspace and from the
> graphics
> > driver, simply send initial "open" lid state to avoid issues. After this is
> > improved from the userspace and from the graphics driver, Linux kernel
> > could simply revert this minimal commit.
> >
> > Link 1: https://lkml.org/2016/3/7/460
> > Link 2: https://github.com/systemd/systemd/issues/2087
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Cc: Bastien Nocera: <hadess@hadess.net>
> > ---
> > drivers/acpi/button.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index e706e4b..6e77312 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -342,6 +342,8 @@ 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)
> > + return acpi_lid_notify_state(device, 1);
>
> As Valdis replied on 0/3, I don't think this is a good solution (even
> temporary). Linux should not assume the current state of a input
> device, and sending unconditionally 1 here is wrong. If the device is
> on a docking station, you will wake up the wrong monitor and screw the
> user session (and this will be a regression).
[Lv Zheng]
We are doing the test to see how this behaves on several different platforms.
>
> How about we simply send the current LID state stored in the ACPI?
> something like calling acpi_lid_send_state() directly?
[Lv Zheng]
This is what we are going to eliminate in [PATCH 01].
We have several real bugs related to sending a wrong state to the userspace.
Userspace will suspend right after resume because of the 'close' state.
>
> [15 min later, after writing a long email]
>
> Well, it looks like we already have that in the kernel and for a long
> time apparently.
>
> So, I think the issue is that Microsoft does not wake up the system by
> lid opening (seen in one of the comments in the mentioned bugs and by
> looking at the behavior on the surface devices). It must be just
> querying it's state on resume or might even not care at all until it
> receives a close event.
>
> If I read correctly, we managed to get the 3 bogus devices to a
> correct state at the ACPI level (/proc/acpi/button/lid/LID0/state),
> but we did not get the notification. Given that the LID state is
> triggered by an ACPI operation region, there is no guarantee that the
> resume of the acpi/button.c driver will be called after the region has
> been updated/called.
[Lv Zheng]
The understanding here is incorrect.
We have 3 bogus devices.
1 of them is surface 3 which is a hardware reduced platform.
The others are all traditional platforms.
=====
The facts are:
Both the platforms return cached lid state from _LID.
The cached value will be updated by lid irq (via GPIO IRQ, GPE, or EC event).
AML tables will send lid notification in the irq handler.
Some AML tables will update the cached value in _WAK (I'll describe why it is necessary below).
But updating the cached value in _WAK is not guaranteed by all AML tables.
For the 'close' state irq, all tables will send lid close notification.
For the 'open' state irq, it seems there are tables never sending lid open notification (sounds like Windows do not care about lid open).
=====
Surface 3 is entirely a different case.
It is a runtime idle system and hardware reduced.
On that kind of system, lid open is handled by OS not by BIOS.
Surface 3 is exactly the platform that doesn't send lid open notification.
I guess the AML is intentionally written in this way to be compliant to the traditional platforms.
While on the traditional platforms:
When lid is opened, BIOS handles the lid irq and wakes the system from the FACS waking vector.
So it is likely that there is no lid open irq after the system is resumed.
BIOS may forget to update the cached lid value in the _WAK or some other control methods that could be executed after resuming.
Then if we send _LID result to the user space, the cached value could apparently be 'close'.
That explains why there is no "lid open" configuration in the "Windows Device Manager".
>
> I propose as a workaround to enable a kthread that will monitor the
> lid state and update the correct value to userspace (5 sec of polling
> time should be enough given that systemd checks every 20 sec).
> We should probably have this workaround only for a set of known
> devices, as it might just be temporary for those until the actual
> underlying problem is fixed (wrong DSDT in the Surface 3 case that
> doesn't notify at all, issue in the EC for the Surface Pro 1 and the
> Samsung N210).
[Lv Zheng]
That cannot help to solve the issue/gap.
The problem is Linux userspace has a facility re-checking lid state when lid state is "close".
If it failed to update the lid state to "open" for a timeout period, it would suspend the platform.
We actually are recommending Linus userspace to introduce a new lid handling mode to only handle "lid close" event and ignore "lid open" event.
During the period this is not changed from the userspace, we have to send "open" after resume/boot in order not to trigger the timeout mechanism.
Thanks and best regards
-Lv
>
> Cheers,
> Benjamin
>
>
> > return 0;
> > }
> > #endif
> > @@ -422,6 +424,7 @@ static int acpi_button_add(struct acpi_device
> *device)
> > if (error)
> > goto err_remove_fs;
> > if (button->type == ACPI_BUTTON_TYPE_LID) {
> > + (void)acpi_lid_notify_state(device, 1);
> > /*
> > * This assumes there's only one lid device, or if there are
> > * more we only care about the last one...
> > --
> > 1.7.10
> >
> > --
> > 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]
| From | Lv Zheng <lv.zheng@intel.com> |
|---|---|
| Date | 2016-05-27 09:20 +0200 |
| Subject | [PATCH v2 0/3] ACPI / button: Clarify initial lid state |
| Message-ID | <rDkdP-ox-19@gated-at.bofh.it> |
| In reply to | #1402194 |
The initial _LID returning value is not reliable after boot/resume because the BIOS vendors may implement it by returning a cached value that is only updated when a lid notification is received. This causes strange things happening after resuming. This patchset fixes the issue according to this fact. Lv Zheng (3): ACPI / button: Remove initial lid state notification ACPI / button: Refactor functions to eliminate redundant code ACPI / button: Send "open" state after boot/resume drivers/acpi/button.c | 96 ++++++++++++++++++++++++++----------------------- 1 file changed, 52 insertions(+), 44 deletions(-) -- 1.7.10
[toc] | [prev] | [next] | [standalone]
| From | Valdis.Kletnieks@vt.edu |
|---|---|
| Date | 2016-05-28 00:20 +0200 |
| Subject | Re: [PATCH v2 0/3] ACPI / button: Clarify initial lid state |
| Message-ID | <rDygN-J1-7@gated-at.bofh.it> |
| In reply to | #1407866 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, 27 May 2016 15:15:43 +0800, Lv Zheng said: > The initial _LID returning value is not reliable after boot/resume because > the BIOS vendors may implement it by returning a cached value that is only > updated when a lid notification is received. > This causes strange things happening after resuming. This patchset fixes > the issue according to this fact. > > Lv Zheng (3): > ACPI / button: Remove initial lid state notification > ACPI / button: Refactor functions to eliminate redundant code > ACPI / button: Send "open" state after boot/resume If it sends an "open" status, what happens if the machine was in fact suspended with lid open, but is then resumed in a dock with an external monitor? (I'd test it myself, but I don't have suspend/resume configured on my laptop)
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web