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

Groups > linux.kernel > #1411060 > unrolled thread

[PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification

Back to article view | Back to linux.kernel

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

Contents

  [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification Lv Zheng <lv.zheng@intel.com> - 2016-06-01 12:20 +0200
    Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state  notification Bastien Nocera <hadess@hadess.net> - 2016-06-01 13:10 +0200
      RE: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state  notification "Zheng, Lv" <lv.zheng@intel.com> - 2016-06-02 03:10 +0200
        Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state  notification Bastien Nocera <hadess@hadess.net> - 2016-06-02 16:10 +0200
          Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2016-06-02 17:30 +0200
            RE: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state  notification "Zheng, Lv" <lv.zheng@intel.com> - 2016-06-03 02:50 +0200

#1411060 — [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification

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, users can simply configure ACPI button driver to send initial
"open" lid state using button.lid_init_state=open to avoid such kind of
issues. And our ultimate target should be making
button.lid_init_state=ignore the default behavior. This patch implements
the 2 options and keep the old behavior (button.lid_init_state=method).

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>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/acpi/button.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 6e291c1..148f4e5 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_METHOD;
 
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
@@ -285,6 +290,21 @@ static int acpi_lid_update_state(struct acpi_device *device)
 	return acpi_lid_notify_state(device, state);
 }
 
+static void acpi_lid_initialize_state(struct acpi_device *device)
+{
+	switch (lid_init_state) {
+	case ACPI_BUTTON_LID_INIT_OPEN:
+		(void)acpi_lid_notify_state(device, 1);
+		break;
+	case ACPI_BUTTON_LID_INIT_METHOD:
+		(void)acpi_lid_update_state(device);
+		break;
+	case ACPI_BUTTON_LID_INIT_IGNORE:
+	default:
+		break;
+	}
+}
+
 static void acpi_button_notify(struct acpi_device *device, u32 event)
 {
 	struct acpi_button *button = acpi_driver_data(device);
@@ -341,6 +361,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)
+		acpi_lid_initialize_state(device);
 	return 0;
 }
 #endif
@@ -421,6 +443,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_initialize_state(device);
 		/*
 		 * This assumes there's only one lid device, or if there are
 		 * more we only care about the last one...
@@ -450,4 +473,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]

#1411095 — Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification

On Wed, 2016-06-01 at 18:10 +0800, Lv Zheng 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.

"implement"

> Before the situation is improved from the userspace and from the graphics
> driver, users can simply configure ACPI button driver to send initial
> "open" lid state using button.lid_init_state=open to avoid such kind of
> issues. And our ultimate target should be making
> button.lid_init_state=ignore the default behavior. This patch implements
> the 2 options and keep the old behavior (button.lid_init_state=method).

I still don't think it's reasonable to expect any changes in user-space 
unless you start documenting what the API to user-space actually is.

(I work on UPower, which also exports that information, and which gets
used in gnome-settings-daemon in a number of ways)

> 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>
> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/acpi/button.c |   61
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 6e291c1..148f4e5 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_METHOD;
>  
>  /* ---------------------------------------------------------------
> -----------
>                                FS Interface (/proc)
> @@ -285,6 +290,21 @@ static int acpi_lid_update_state(struct
> acpi_device *device)
>  	return acpi_lid_notify_state(device, state);
>  }
>  
> +static void acpi_lid_initialize_state(struct acpi_device *device)
> +{
> +	switch (lid_init_state) {
> +	case ACPI_BUTTON_LID_INIT_OPEN:
> +		(void)acpi_lid_notify_state(device, 1);
> +		break;
> +	case ACPI_BUTTON_LID_INIT_METHOD:
> +		(void)acpi_lid_update_state(device);
> +		break;
> +	case ACPI_BUTTON_LID_INIT_IGNORE:
> +	default:
> +		break;
> +	}
> +}
> +
>  static void acpi_button_notify(struct acpi_device *device, u32
> event)
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
> @@ -341,6 +361,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)
> +		acpi_lid_initialize_state(device);
>  	return 0;
>  }
>  #endif
> @@ -421,6 +443,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_initialize_state(device);
>  		/*
>  		 * This assumes there's only one lid device, or if
> there are
>  		 * more we only care about the last one...
> @@ -450,4 +473,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);

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

#1411731 — RE: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification

Hi,

> From: Bastien Nocera [mailto:hadess@hadess.net]
> Subject: Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state
> notification
> 
> On Wed, 2016-06-01 at 18:10 +0800, Lv Zheng 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.
> 
> "implement"
[Lv Zheng] 
Thanks for pointing out, I'll send an UPDATE to this.

> 
> > Before the situation is improved from the userspace and from the
> graphics
> > driver, users can simply configure ACPI button driver to send initial
> > "open" lid state using button.lid_init_state=open to avoid such kind of
> > issues. And our ultimate target should be making
> > button.lid_init_state=ignore the default behavior. This patch implements
> > the 2 options and keep the old behavior (button.lid_init_state=method).
> 
> I still don't think it's reasonable to expect any changes in user-space
> unless you start documenting what the API to user-space actually is.
[Lv Zheng] 
IMO, the ACPI lid driver should be responsible for sending lid key event (especially "close") to the user space.
So if someone need to implement an ACPI lid key event quirk, we could help to implement it from the kernel space.
And since the initial lid state is not stable, we have to stop doing quirks around it inside of the Linux kernel, or inside of the customized AML tables.
User space can still access /proc/acpi/button/lid/LID0/state, but should stop thinking that it is reliable.

These are what I can conclude from the bugs.

Thanks and best regards
-Lv

> 
> (I work on UPower, which also exports that information, and which gets
> used in gnome-settings-daemon in a number of ways)
> 
> > 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>
> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/acpi/button.c |   61
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 6e291c1..148f4e5 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_METHOD;
> >
> >  /* ---------------------------------------------------------------
> > -----------
> >                                FS Interface (/proc)
> > @@ -285,6 +290,21 @@ static int acpi_lid_update_state(struct
> > acpi_device *device)
> >  	return acpi_lid_notify_state(device, state);
> >  }
> >
> > +static void acpi_lid_initialize_state(struct acpi_device *device)
> > +{
> > +	switch (lid_init_state) {
> > +	case ACPI_BUTTON_LID_INIT_OPEN:
> > +		(void)acpi_lid_notify_state(device, 1);
> > +		break;
> > +	case ACPI_BUTTON_LID_INIT_METHOD:
> > +		(void)acpi_lid_update_state(device);
> > +		break;
> > +	case ACPI_BUTTON_LID_INIT_IGNORE:
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >  static void acpi_button_notify(struct acpi_device *device, u32
> > event)
> >  {
> >  	struct acpi_button *button = acpi_driver_data(device);
> > @@ -341,6 +361,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)
> > +		acpi_lid_initialize_state(device);
> >  	return 0;
> >  }
> >  #endif
> > @@ -421,6 +443,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_initialize_state(device);
> >  		/*
> >  		 * This assumes there's only one lid device, or if
> > there are
> >  		 * more we only care about the last one...
> > @@ -450,4 +473,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);

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

#1412249 — Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification

On Thu, 2016-06-02 at 01:08 +0000, Zheng, Lv wrote:
> Hi,
> 
> > From: Bastien Nocera [mailto:hadess@hadess.net]
> > Subject: Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial
> > lid state
> > notification
> > 
> > On Wed, 2016-06-01 at 18:10 +0800, Lv Zheng 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.
> > 
> > "implement"
> [Lv Zheng] 
> Thanks for pointing out, I'll send an UPDATE to this.
> 
> > 
> > > Before the situation is improved from the userspace and from the
> > graphics
> > > driver, users can simply configure ACPI button driver to send
> > > initial
> > > "open" lid state using button.lid_init_state=open to avoid such
> > > kind of
> > > issues. And our ultimate target should be making
> > > button.lid_init_state=ignore the default behavior. This patch
> > > implements
> > > the 2 options and keep the old behavior
> > > (button.lid_init_state=method).
> > 
> > I still don't think it's reasonable to expect any changes in user-
> > space
> > unless you start documenting what the API to user-space actually
> > is.
> [Lv Zheng] 
> IMO, the ACPI lid driver should be responsible for sending lid key
> event (especially "close") to the user space.
> So if someone need to implement an ACPI lid key event quirk, we could
> help to implement it from the kernel space.
> And since the initial lid state is not stable, we have to stop doing
> quirks around it inside of the Linux kernel, or inside of the
> customized AML tables.
> User space can still access /proc/acpi/button/lid/LID0/state, but
> should stop thinking that it is reliable.
> 
> These are what I can conclude from the bugs.

There's still no documentation for user-space in the patch, and no way
to disable the "legacy" support (disabling access to the cached LID
state, especially through the input layer which is what logind and
upower use).

You can't expect user-space to change in major ways for those few
devices if the API doesn't force them to, through deprecation notices
and documentation.

Cheers

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

#1412361

On Thu, Jun 2, 2016 at 4:01 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2016-06-02 at 01:08 +0000, Zheng, Lv wrote:
>> Hi,
>>
>> > From: Bastien Nocera [mailto:hadess@hadess.net]
>> > Subject: Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial
>> > lid state
>> > notification
>> >
>> > On Wed, 2016-06-01 at 18:10 +0800, Lv Zheng 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.
>> >
>> > "implement"
>> [Lv Zheng]
>> Thanks for pointing out, I'll send an UPDATE to this.
>>
>> >
>> > > Before the situation is improved from the userspace and from the
>> > graphics
>> > > driver, users can simply configure ACPI button driver to send
>> > > initial
>> > > "open" lid state using button.lid_init_state=open to avoid such
>> > > kind of
>> > > issues. And our ultimate target should be making
>> > > button.lid_init_state=ignore the default behavior. This patch
>> > > implements
>> > > the 2 options and keep the old behavior
>> > > (button.lid_init_state=method).
>> >
>> > I still don't think it's reasonable to expect any changes in user-
>> > space
>> > unless you start documenting what the API to user-space actually
>> > is.
>> [Lv Zheng]
>> IMO, the ACPI lid driver should be responsible for sending lid key
>> event (especially "close") to the user space.
>> So if someone need to implement an ACPI lid key event quirk, we could
>> help to implement it from the kernel space.
>> And since the initial lid state is not stable, we have to stop doing
>> quirks around it inside of the Linux kernel, or inside of the
>> customized AML tables.
>> User space can still access /proc/acpi/button/lid/LID0/state, but
>> should stop thinking that it is reliable.
>>
>> These are what I can conclude from the bugs.

After further thoughts, I also think it is a bad idea to request user
space to change behavior with respect to the LID switch event we
forward from the keyboard:
- it looks like Windows doesn't care about LID open on some
(entry-level) platforms: the Samsung N210 is one of the first netbooks
from 2010. The Surface (pro or not) are tablets. On these low cost
systems, we can easily assume that the user needs to have the LID open
to have the system working. You can't connect a docking station, and
they are probably not used in a professional environment.
- for the high end machines (think professional), we actually need to
have a valid LID state given that the machines can be used on a
docking station, so LID closed. If we do not send a reliable LID state
for those laptops we will break user space and more likely annoy
users: we might light up the closed internal monitor and migrate all
the currently open applications to this screen.

So I think Windows might be able to detect those 2 categories of
environments and behave accordingly.

Then, if we want to express to user space that the LID switch state is
not reliable, we should stop setting it up as an input device with a
EV_SWITCH in it. The kernel has to be reliable, and we can't start
saying that this particular switch in a system might not be reliable.
In this, I join Bastien's point of view where we need to start
deprecating and document what needs to be done, and introduce a new
way of reporting the LID events to user space (by using a
KEY_LID_CLOSE for instance).

I still think this patch is necessary. Until we manage to understand
what is going on on Windows for the non reliable LID state, we can
always ask users to use the button.lid_init_state=open to prevent the
freeze loops they are seeing.
The deprecation process could be to send the open state at resume on
the SW_LID event, and send both the close and a KEY_LID_CLOSE event on
close (no KEY_* on open).

For professional laptops (with docking capability), I can't see how we
could avoid forwarding a reliable state, and we need them to stick to
button.lid_init_state=method (keep SW_LID and not send the
KEY_LID_CLOSE event so userspace knows it's reliable).

Hope this helps,
Benjamin


>
> There's still no documentation for user-space in the patch, and no way
> to disable the "legacy" support (disabling access to the cached LID
> state, especially through the input layer which is what logind and
> upower use).
>
> You can't expect user-space to change in major ways for those few
> devices if the API doesn't force them to, through deprecation notices
> and documentation.
>
> Cheers

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

#1412657 — RE: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state
> notification
> 
> On Thu, Jun 2, 2016 at 4:01 PM, Bastien Nocera <hadess@hadess.net>
> wrote:
> > On Thu, 2016-06-02 at 01:08 +0000, Zheng, Lv wrote:
> >> Hi,
> >>
> >> > From: Bastien Nocera [mailto:hadess@hadess.net]
> >> > Subject: Re: [PATCH v3 3/3] ACPI / button: Add quirks for initial
> >> > lid state
> >> > notification
> >> >
> >> > On Wed, 2016-06-01 at 18:10 +0800, Lv Zheng 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.
> >> >
> >> > "implement"
> >> [Lv Zheng]
> >> Thanks for pointing out, I'll send an UPDATE to this.
> >>
> >> >
> >> > > Before the situation is improved from the userspace and from the
> >> > graphics
> >> > > driver, users can simply configure ACPI button driver to send
> >> > > initial
> >> > > "open" lid state using button.lid_init_state=open to avoid such
> >> > > kind of
> >> > > issues. And our ultimate target should be making
> >> > > button.lid_init_state=ignore the default behavior. This patch
> >> > > implements
> >> > > the 2 options and keep the old behavior
> >> > > (button.lid_init_state=method).
> >> >
> >> > I still don't think it's reasonable to expect any changes in user-
> >> > space
> >> > unless you start documenting what the API to user-space actually
> >> > is.
> >> [Lv Zheng]
> >> IMO, the ACPI lid driver should be responsible for sending lid key
> >> event (especially "close") to the user space.
> >> So if someone need to implement an ACPI lid key event quirk, we could
> >> help to implement it from the kernel space.
> >> And since the initial lid state is not stable, we have to stop doing
> >> quirks around it inside of the Linux kernel, or inside of the
> >> customized AML tables.
> >> User space can still access /proc/acpi/button/lid/LID0/state, but
> >> should stop thinking that it is reliable.
> >>
> >> These are what I can conclude from the bugs.
> 
> After further thoughts, I also think it is a bad idea to request user
> space to change behavior with respect to the LID switch event we
> forward from the keyboard:
> - it looks like Windows doesn't care about LID open on some
> (entry-level) platforms: the Samsung N210 is one of the first netbooks
> from 2010. The Surface (pro or not) are tablets. On these low cost
> systems, we can easily assume that the user needs to have the LID open
> to have the system working. You can't connect a docking station, and
> they are probably not used in a professional environment.
> - for the high end machines (think professional), we actually need to
> have a valid LID state given that the machines can be used on a
> docking station, so LID closed. If we do not send a reliable LID state
> for those laptops we will break user space and more likely annoy
> users: we might light up the closed internal monitor and migrate all
> the currently open applications to this screen.
> 
> So I think Windows might be able to detect those 2 categories of
> environments and behave accordingly.
> 
> Then, if we want to express to user space that the LID switch state is
> not reliable, we should stop setting it up as an input device with a
> EV_SWITCH in it. The kernel has to be reliable, and we can't start
> saying that this particular switch in a system might not be reliable.
> In this, I join Bastien's point of view where we need to start
> deprecating and document what needs to be done, and introduce a new
> way of reporting the LID events to user space (by using a
> KEY_LID_CLOSE for instance).
> 
> I still think this patch is necessary. Until we manage to understand
> what is going on on Windows for the non reliable LID state, we can
> always ask users to use the button.lid_init_state=open to prevent the
> freeze loops they are seeing.
> The deprecation process could be to send the open state at resume on
> the SW_LID event, and send both the close and a KEY_LID_CLOSE event on
> close (no KEY_* on open).
> 
> For professional laptops (with docking capability), I can't see how we
> could avoid forwarding a reliable state, and we need them to stick to
> button.lid_init_state=method (keep SW_LID and not send the
> KEY_LID_CLOSE event so userspace knows it's reliable).
[Lv Zheng] 
All sound reasonable to me.
We'll discuss this internally before making further changes.

For the documentation work.
I'm planning to send several documents around ACPICA release, ACPICA debugger, and probing de-facto standard ACPI behavior.
So I can help to add one for "ACPI control method lid device" to clarify this in the same series.

Thanks and best regards
-Lv

> 
> Hope this helps,
> Benjamin
> 
> 
> >
> > There's still no documentation for user-space in the patch, and no way
> > to disable the "legacy" support (disabling access to the cached LID
> > state, especially through the input layer which is what logind and
> > upower use).
> >
> > You can't expect user-space to change in major ways for those few
> > devices if the API doesn't force them to, through deprecation notices
> > and documentation.
> >
> > Cheers

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web