[RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Lv Zheng
(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 <[hidden email]>
Cc: Bastien Nocera: <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Rafael J. Wysocki-5
On Tue, May 17, 2016 at 10:27 AM, Lv Zheng <[hidden email]> 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 <[hidden email]>
> Cc: Bastien Nocera: <[hidden email]>
> ---
>  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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Lv Zheng
Hi, Rafael

Thanks for the review.

> From: [hidden email] [mailto:[hidden email]] On Behalf Of
> Rafael J. Wysocki
> Sent: Wednesday, May 18, 2016 7:37 AM
> To: Zheng, Lv <[hidden email]>
> Cc: Wysocki, Rafael J <[hidden email]>; Rafael J. Wysocki
> <[hidden email]>; Brown, Len <[hidden email]>; Lv Zheng
> <[hidden email]>; Linux Kernel Mailing List <linux-
> [hidden email]>; ACPI Devel Maling List <[hidden email]>;
> Bastien Nocera: <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]>
> > Cc: Bastien Nocera: <[hidden email]>
> > ---
> >  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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Bastien Nocera
In reply to this post by Lv Zheng
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 <[hidden email]>
> Cc: Bastien Nocera: <[hidden email]>

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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Rafael J. Wysocki-5
On Wed, May 18, 2016 at 2:57 PM, Bastien Nocera <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Rafael J. Wysocki-5
In reply to this post by Lv Zheng
On Wed, May 18, 2016 at 3:25 AM, Zheng, Lv <[hidden email]> wrote:

> Hi, Rafael
>
> Thanks for the review.
>
>> From: [hidden email] [mailto:[hidden email]] On Behalf Of
>> Rafael J. Wysocki
>> Sent: Wednesday, May 18, 2016 7:37 AM
>> To: Zheng, Lv <[hidden email]>
>> Cc: Wysocki, Rafael J <[hidden email]>; Rafael J. Wysocki
>> <[hidden email]>; Brown, Len <[hidden email]>; Lv Zheng
>> <[hidden email]>; Linux Kernel Mailing List <linux-
>> [hidden email]>; ACPI Devel Maling List <[hidden email]>;
>> Bastien Nocera: <[hidden email]>
>> 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 <[hidden email]> 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 <[hidden email]>
>> > Cc: Bastien Nocera: <[hidden email]>
>> > ---
>> >  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?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Lv Zheng
Hi,

> From: [hidden email] [mailto:[hidden email]] 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 <[hidden email]> wrote:
> > Hi, Rafael
> >
> > Thanks for the review.
> >
> >> From: [hidden email] [mailto:[hidden email]] On Behalf Of
> >> Rafael J. Wysocki
> >> Sent: Wednesday, May 18, 2016 7:37 AM
> >> To: Zheng, Lv <[hidden email]>
> >> Cc: Wysocki, Rafael J <[hidden email]>; Rafael J. Wysocki
> >> <[hidden email]>; Brown, Len <[hidden email]>; Lv Zheng
> >> <[hidden email]>; Linux Kernel Mailing List <linux-
> >> [hidden email]>; ACPI Devel Maling List <linux-
> [hidden email]>;
> >> Bastien Nocera: <[hidden email]>
> >> 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 <[hidden email]> 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 <[hidden email]>
> >> > Cc: Bastien Nocera: <[hidden email]>
> >> > ---
> >> >  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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Lv Zheng
In reply to this post by Bastien Nocera
Hi,

> From: Bastien Nocera [mailto:[hidden email]]
> 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 <[hidden email]>
> > Cc: Bastien Nocera: <[hidden email]>
>
> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Rafael J. Wysocki-5
In reply to this post by Lv Zheng
On Thu, May 19, 2016 at 3:50 AM, Zheng, Lv <[hidden email]> wrote:
> Hi,
>
>> From: [hidden email] [mailto:[hidden email]] 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume

Benjamin Tissoires
[Jumping in the discussion at Bastien's request]

On Thu, May 19, 2016 at 3:21 PM, Rafael J. Wysocki <[hidden email]> wrote:

> On Thu, May 19, 2016 at 3:50 AM, Zheng, Lv <[hidden email]> wrote:
>> Hi,
>>
>>> From: [hidden email] [mailto:[hidden email]] 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 <[hidden email]>
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 <[hidden email]>
---
 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
Loading...