diff mbox

[v2,4/4] ACPI / button: Add document for ACPI control method lid device restrictions

Message ID 3f24a00df89f06661a64af6b4679a99bfff09aa7.1467875143.git.lv.zheng@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lv Zheng July 7, 2016, 7:11 a.m. UTC
There are many AML tables reporting wrong initial lid state, and some of
them never reports lid state. As a proxy layer acting between, ACPI button
driver is not able to handle all such cases, but need to re-define the
usage model of the ACPI lid. That is:
1. It's initial state is not reliable;
2. There may not be open event;
3. Userspace should only take action against the close event which is
   reliable, always sent after a real lid close.
This patch adds documentation of the usage model.

Link: https://lkml.org/2016/3/7/460
Link: 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>
Cc: linux-input@vger.kernel.org
---
 Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/acpi/acpi-lid.txt

Comments

Benjamin Tissoires July 8, 2016, 9:17 a.m. UTC | #1
Hi,

On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> There are many AML tables reporting wrong initial lid state, and some of
> them never reports lid state. As a proxy layer acting between, ACPI button
> driver is not able to handle all such cases, but need to re-define the
> usage model of the ACPI lid. That is:
> 1. It's initial state is not reliable;
> 2. There may not be open event;
> 3. Userspace should only take action against the close event which is
>    reliable, always sent after a real lid close.
> This patch adds documentation of the usage model.
>
> Link: https://lkml.org/2016/3/7/460
> Link: 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>
> Cc: linux-input@vger.kernel.org
> ---
>  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/acpi/acpi-lid.txt
>
> diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
> new file mode 100644
> index 0000000..7e4f7ed
> --- /dev/null
> +++ b/Documentation/acpi/acpi-lid.txt
> @@ -0,0 +1,62 @@
> +Usage Model of the ACPI Control Method Lid Device
> +
> +Copyright (C) 2016, Intel Corporation
> +Author: Lv Zheng <lv.zheng@intel.com>
> +
> +
> +Abstract:
> +
> +Platforms containing lids convey lid state (open/close) to OSPMs using a
> +control method lid device. To implement this, the AML tables issue
> +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> +changed. The _LID control method for the lid device must be implemented to
> +report the "current" state of the lid as either "opened" or "closed".
> +
> +This document describes the restrictions and the expections of the Linux
> +ACPI lid device driver.
> +
> +
> +1. Restrictions of the returning value of the _LID control method
> +
> +The _LID control method is described to return the "current" lid state.
> +However the word of "current" has ambiguity, many AML tables return the lid
> +state upon the last lid notification instead of returning the lid state
> +upon the last _LID evaluation. There won't be difference when the _LID
> +control method is evaluated during the runtime, the problem is its initial
> +returning value. When the AML tables implement this control method with
> +cached value, the initial returning value is likely not reliable. There are
> +simply so many examples always retuning "closed" as initial lid state.
> +
> +2. Restrictions of the lid state change notifications
> +
> +There are many AML tables never notifying when the lid device state is
> +changed to "opened". But it is ensured that the AML tables always notify
> +"closed" when the lid state is changed to "closed". This is normally used
> +to trigger some system power saving operations on Windows. Since it is
> +fully tested, this notification is reliable for all AML tables.
> +
> +3. Expections for the userspace users of the ACPI lid device driver
> +
> +The userspace programs should stop relying on
> +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
> +used for the validation purpose.

I'd say: this file actually calls the _LID method described above. And
given the previous explanation, it is not reliable enough on some
platforms. So it is strongly advised for user-space program to not
solely rely on this file to determine the actual lid state.

> +
> +New userspace programs should rely on the lid "closed" notification to
> +trigger some power saving operations and may stop taking actions according
> +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
> +prepared for the new userspace to implement this ACPI control method lid
> +device specific logics.

That's not entirely what we discussed before (to prevent regressions):
- if the device doesn't have reliable LID switch state, then there
would be the new input event, and so userspace should only rely on
opened notifications.
- if the device has reliable switch information, the new input event
should not be exported and userspace knows that the current input
switch event is reliable.

Also, using a new "switch" event is a terrible idea. Switches have a
state (open/close) and you are using this to forward a single open
event. So using a switch just allows you to say to userspace you are
using the "new" LID meaning, but you'll still have to manually reset
the switch and you will have to document how this event is not a
switch.

Please use a simple KEY_LID_OPEN event you will send through
[input_key_event(KEY_LID_OPEN, 1), input_sync(),
input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows
how to handle.

> +
> +During the period the userspace hasn't been switched to use the new
> +SW_ACPI_LID event, Linux users can use the following boot parameter to
> +handle possible issues:
> +  button.lid_init_state=method:
> +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> +   reports the initial lid state using the returning value of the _LID
> +   control method.
> +   This can be used to fix some platforms if the _LID control method's
> +   returning value is reliable.
> +  button.lid_init_state=open:
> +   Linux kernel always reports the initial lid state as "opened".
> +   This may fix some platforms if the returning value of the _LID control
> +   method is not reliable.

This worries me as there is no plan after "During the period the
userspace hasn't been switched to use the new event".

I really hope you'll keep sending SW_LID for reliable LID platforms,
and not remove it entirely as you will break platforms.

Cheers,
Benjamin

> --
> 1.7.10
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov July 8, 2016, 5:51 p.m. UTC | #2
On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> Hi,
> 
> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > There are many AML tables reporting wrong initial lid state, and some of
> > them never reports lid state. As a proxy layer acting between, ACPI button
> > driver is not able to handle all such cases, but need to re-define the
> > usage model of the ACPI lid. That is:
> > 1. It's initial state is not reliable;
> > 2. There may not be open event;
> > 3. Userspace should only take action against the close event which is
> >    reliable, always sent after a real lid close.
> > This patch adds documentation of the usage model.
> >
> > Link: https://lkml.org/2016/3/7/460
> > Link: 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>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >  create mode 100644 Documentation/acpi/acpi-lid.txt
> >
> > diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
> > new file mode 100644
> > index 0000000..7e4f7ed
> > --- /dev/null
> > +++ b/Documentation/acpi/acpi-lid.txt
> > @@ -0,0 +1,62 @@
> > +Usage Model of the ACPI Control Method Lid Device
> > +
> > +Copyright (C) 2016, Intel Corporation
> > +Author: Lv Zheng <lv.zheng@intel.com>
> > +
> > +
> > +Abstract:
> > +
> > +Platforms containing lids convey lid state (open/close) to OSPMs using a
> > +control method lid device. To implement this, the AML tables issue
> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> > +changed. The _LID control method for the lid device must be implemented to
> > +report the "current" state of the lid as either "opened" or "closed".
> > +
> > +This document describes the restrictions and the expections of the Linux
> > +ACPI lid device driver.
> > +
> > +
> > +1. Restrictions of the returning value of the _LID control method
> > +
> > +The _LID control method is described to return the "current" lid state.
> > +However the word of "current" has ambiguity, many AML tables return the lid
> > +state upon the last lid notification instead of returning the lid state
> > +upon the last _LID evaluation. There won't be difference when the _LID
> > +control method is evaluated during the runtime, the problem is its initial
> > +returning value. When the AML tables implement this control method with
> > +cached value, the initial returning value is likely not reliable. There are
> > +simply so many examples always retuning "closed" as initial lid state.
> > +
> > +2. Restrictions of the lid state change notifications
> > +
> > +There are many AML tables never notifying when the lid device state is
> > +changed to "opened". But it is ensured that the AML tables always notify
> > +"closed" when the lid state is changed to "closed". This is normally used
> > +to trigger some system power saving operations on Windows. Since it is
> > +fully tested, this notification is reliable for all AML tables.
> > +
> > +3. Expections for the userspace users of the ACPI lid device driver
> > +
> > +The userspace programs should stop relying on
> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
> > +used for the validation purpose.
> 
> I'd say: this file actually calls the _LID method described above. And
> given the previous explanation, it is not reliable enough on some
> platforms. So it is strongly advised for user-space program to not
> solely rely on this file to determine the actual lid state.
> 
> > +
> > +New userspace programs should rely on the lid "closed" notification to
> > +trigger some power saving operations and may stop taking actions according
> > +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
> > +prepared for the new userspace to implement this ACPI control method lid
> > +device specific logics.
> 
> That's not entirely what we discussed before (to prevent regressions):
> - if the device doesn't have reliable LID switch state, then there
> would be the new input event, and so userspace should only rely on
> opened notifications.
> - if the device has reliable switch information, the new input event
> should not be exported and userspace knows that the current input
> switch event is reliable.
> 
> Also, using a new "switch" event is a terrible idea. Switches have a
> state (open/close) and you are using this to forward a single open
> event. So using a switch just allows you to say to userspace you are
> using the "new" LID meaning, but you'll still have to manually reset
> the switch and you will have to document how this event is not a
> switch.
> 
> Please use a simple KEY_LID_OPEN event you will send through
> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows
> how to handle.
> 
> > +
> > +During the period the userspace hasn't been switched to use the new
> > +SW_ACPI_LID event, Linux users can use the following boot parameter to
> > +handle possible issues:
> > +  button.lid_init_state=method:
> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> > +   reports the initial lid state using the returning value of the _LID
> > +   control method.
> > +   This can be used to fix some platforms if the _LID control method's
> > +   returning value is reliable.
> > +  button.lid_init_state=open:
> > +   Linux kernel always reports the initial lid state as "opened".
> > +   This may fix some platforms if the returning value of the _LID control
> > +   method is not reliable.
> 
> This worries me as there is no plan after "During the period the
> userspace hasn't been switched to use the new event".
> 
> I really hope you'll keep sending SW_LID for reliable LID platforms,
> and not remove it entirely as you will break platforms.

How about we leave the kernel alone and userspace (which would have to
cope with the new KEY_LID_OPEN anyway) would just have to know that if
switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then it
can't trust the events and it needs additional heuristics.

Thanks.
Lv Zheng July 11, 2016, 3:20 a.m. UTC | #3
Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]

> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control

> method lid device restrictions

> 

> Hi,

> 

> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:

> > There are many AML tables reporting wrong initial lid state, and some of

> > them never reports lid state. As a proxy layer acting between, ACPI

> button

> > driver is not able to handle all such cases, but need to re-define the

> > usage model of the ACPI lid. That is:

> > 1. It's initial state is not reliable;

> > 2. There may not be open event;

> > 3. Userspace should only take action against the close event which is

> >    reliable, always sent after a real lid close.

> > This patch adds documentation of the usage model.

> >

> > Link: https://lkml.org/2016/3/7/460

> > Link: 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>

> > Cc: linux-input@vger.kernel.org

> > ---

> >  Documentation/acpi/acpi-lid.txt |   62

> +++++++++++++++++++++++++++++++++++++++

> >  1 file changed, 62 insertions(+)

> >  create mode 100644 Documentation/acpi/acpi-lid.txt

> >

> > diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-

> lid.txt

> > new file mode 100644

> > index 0000000..7e4f7ed

> > --- /dev/null

> > +++ b/Documentation/acpi/acpi-lid.txt

> > @@ -0,0 +1,62 @@

> > +Usage Model of the ACPI Control Method Lid Device

> > +

> > +Copyright (C) 2016, Intel Corporation

> > +Author: Lv Zheng <lv.zheng@intel.com>

> > +

> > +

> > +Abstract:

> > +

> > +Platforms containing lids convey lid state (open/close) to OSPMs using

> a

> > +control method lid device. To implement this, the AML tables issue

> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has

> > +changed. The _LID control method for the lid device must be

> implemented to

> > +report the "current" state of the lid as either "opened" or "closed".

> > +

> > +This document describes the restrictions and the expections of the

> Linux

> > +ACPI lid device driver.

> > +

> > +

> > +1. Restrictions of the returning value of the _LID control method

> > +

> > +The _LID control method is described to return the "current" lid state.

> > +However the word of "current" has ambiguity, many AML tables return

> the lid

> > +state upon the last lid notification instead of returning the lid state

> > +upon the last _LID evaluation. There won't be difference when the _LID

> > +control method is evaluated during the runtime, the problem is its

> initial

> > +returning value. When the AML tables implement this control method

> with

> > +cached value, the initial returning value is likely not reliable. There are

> > +simply so many examples always retuning "closed" as initial lid state.

> > +

> > +2. Restrictions of the lid state change notifications

> > +

> > +There are many AML tables never notifying when the lid device state is

> > +changed to "opened". But it is ensured that the AML tables always

> notify

> > +"closed" when the lid state is changed to "closed". This is normally used

> > +to trigger some system power saving operations on Windows. Since it is

> > +fully tested, this notification is reliable for all AML tables.

> > +

> > +3. Expections for the userspace users of the ACPI lid device driver

> > +

> > +The userspace programs should stop relying on

> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only

> > +used for the validation purpose.

> 

> I'd say: this file actually calls the _LID method described above. And

> given the previous explanation, it is not reliable enough on some

> platforms. So it is strongly advised for user-space program to not

> solely rely on this file to determine the actual lid state.

[Lv Zheng] 
OK.

> 

> > +

> > +New userspace programs should rely on the lid "closed" notification to

> > +trigger some power saving operations and may stop taking actions

> according

> > +to the lid "opened" notification. A new input switch event -

> SW_ACPI_LID is

> > +prepared for the new userspace to implement this ACPI control method

> lid

> > +device specific logics.

> 

> That's not entirely what we discussed before (to prevent regressions):

> - if the device doesn't have reliable LID switch state, then there

> would be the new input event, and so userspace should only rely on

> opened notifications.

> - if the device has reliable switch information, the new input event

> should not be exported and userspace knows that the current input

> switch event is reliable.

> 

> Also, using a new "switch" event is a terrible idea. Switches have a

> state (open/close) and you are using this to forward a single open

> event. So using a switch just allows you to say to userspace you are

> using the "new" LID meaning, but you'll still have to manually reset

> the switch and you will have to document how this event is not a

> switch.

> 

> Please use a simple KEY_LID_OPEN event you will send through

> [input_key_event(KEY_LID_OPEN, 1), input_sync(),

> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows

> how to handle.

[Lv Zheng] 
It should be KEY_LID_CLOSE.
However I checked the KEY code definitions.
It seems their values are highly dependent on the HID specification.
I'm not sure which key code value should I use for this.
Could you point me out?

> 

> > +

> > +During the period the userspace hasn't been switched to use the new

> > +SW_ACPI_LID event, Linux users can use the following boot parameter

> to

> > +handle possible issues:

> > +  button.lid_init_state=method:

> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel

> > +   reports the initial lid state using the returning value of the _LID

> > +   control method.

> > +   This can be used to fix some platforms if the _LID control method's

> > +   returning value is reliable.

> > +  button.lid_init_state=open:

> > +   Linux kernel always reports the initial lid state as "opened".

> > +   This may fix some platforms if the returning value of the _LID control

> > +   method is not reliable.

> 

> This worries me as there is no plan after "During the period the

> userspace hasn't been switched to use the new event".

> 

> I really hope you'll keep sending SW_LID for reliable LID platforms,

> and not remove it entirely as you will break platforms.


[Lv Zheng] 
We won't remove SW_LID from the kernel :).

And we haven't removed SW_LID from the acpi button driver.
We'll just stop sending "initial lid state" from acpi button driver, i.e., the behavior carried out by "button.lid_init_state=ignore".

Maybe it is not sufficient, after the userspace has been changed to support the new event, we should stop sending SW_LID from acpi button driver.

Cheers,
-Lv
Bastien Nocera July 11, 2016, 10:58 a.m. UTC | #4
On Mon, 2016-07-11 at 03:20 +0000, Zheng, Lv wrote:
> 
<snip>
> > This worries me as there is no plan after "During the period the
> > userspace hasn't been switched to use the new event".
> > 
> > I really hope you'll keep sending SW_LID for reliable LID
> > platforms,
> > and not remove it entirely as you will break platforms.
> 
> [Lv Zheng] 
> We won't remove SW_LID from the kernel :).
> 
> And we haven't removed SW_LID from the acpi button driver.
> We'll just stop sending "initial lid state" from acpi button driver,
> i.e., the behavior carried out by "button.lid_init_state=ignore".
> 
> Maybe it is not sufficient, after the userspace has been changed to
> support the new event, we should stop sending SW_LID from acpi button
> driver.

For the affected devices? Sure, but I don't think that's a reasonable
thing to do for "all" the devices. We have a majority of laptops where
this isn't a problem, and it's not even a problem any more on one of
the devices that triggered this discussion (there's a patch for make
the LID status match reality for the Surface 3).
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires July 11, 2016, 11:34 a.m. UTC | #5
On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
>> Hi,
>>
>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > There are many AML tables reporting wrong initial lid state, and some of
>> > them never reports lid state. As a proxy layer acting between, ACPI button
>> > driver is not able to handle all such cases, but need to re-define the
>> > usage model of the ACPI lid. That is:
>> > 1. It's initial state is not reliable;
>> > 2. There may not be open event;
>> > 3. Userspace should only take action against the close event which is
>> >    reliable, always sent after a real lid close.
>> > This patch adds documentation of the usage model.
>> >
>> > Link: https://lkml.org/2016/3/7/460
>> > Link: 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>
>> > Cc: linux-input@vger.kernel.org
>> > ---
>> >  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 62 insertions(+)
>> >  create mode 100644 Documentation/acpi/acpi-lid.txt
>> >
>> > diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
>> > new file mode 100644
>> > index 0000000..7e4f7ed
>> > --- /dev/null
>> > +++ b/Documentation/acpi/acpi-lid.txt
>> > @@ -0,0 +1,62 @@
>> > +Usage Model of the ACPI Control Method Lid Device
>> > +
>> > +Copyright (C) 2016, Intel Corporation
>> > +Author: Lv Zheng <lv.zheng@intel.com>
>> > +
>> > +
>> > +Abstract:
>> > +
>> > +Platforms containing lids convey lid state (open/close) to OSPMs using a
>> > +control method lid device. To implement this, the AML tables issue
>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
>> > +changed. The _LID control method for the lid device must be implemented to
>> > +report the "current" state of the lid as either "opened" or "closed".
>> > +
>> > +This document describes the restrictions and the expections of the Linux
>> > +ACPI lid device driver.
>> > +
>> > +
>> > +1. Restrictions of the returning value of the _LID control method
>> > +
>> > +The _LID control method is described to return the "current" lid state.
>> > +However the word of "current" has ambiguity, many AML tables return the lid
>> > +state upon the last lid notification instead of returning the lid state
>> > +upon the last _LID evaluation. There won't be difference when the _LID
>> > +control method is evaluated during the runtime, the problem is its initial
>> > +returning value. When the AML tables implement this control method with
>> > +cached value, the initial returning value is likely not reliable. There are
>> > +simply so many examples always retuning "closed" as initial lid state.
>> > +
>> > +2. Restrictions of the lid state change notifications
>> > +
>> > +There are many AML tables never notifying when the lid device state is
>> > +changed to "opened". But it is ensured that the AML tables always notify
>> > +"closed" when the lid state is changed to "closed". This is normally used
>> > +to trigger some system power saving operations on Windows. Since it is
>> > +fully tested, this notification is reliable for all AML tables.
>> > +
>> > +3. Expections for the userspace users of the ACPI lid device driver
>> > +
>> > +The userspace programs should stop relying on
>> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
>> > +used for the validation purpose.
>>
>> I'd say: this file actually calls the _LID method described above. And
>> given the previous explanation, it is not reliable enough on some
>> platforms. So it is strongly advised for user-space program to not
>> solely rely on this file to determine the actual lid state.
>>
>> > +
>> > +New userspace programs should rely on the lid "closed" notification to
>> > +trigger some power saving operations and may stop taking actions according
>> > +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
>> > +prepared for the new userspace to implement this ACPI control method lid
>> > +device specific logics.
>>
>> That's not entirely what we discussed before (to prevent regressions):
>> - if the device doesn't have reliable LID switch state, then there
>> would be the new input event, and so userspace should only rely on
>> opened notifications.
>> - if the device has reliable switch information, the new input event
>> should not be exported and userspace knows that the current input
>> switch event is reliable.
>>
>> Also, using a new "switch" event is a terrible idea. Switches have a
>> state (open/close) and you are using this to forward a single open
>> event. So using a switch just allows you to say to userspace you are
>> using the "new" LID meaning, but you'll still have to manually reset
>> the switch and you will have to document how this event is not a
>> switch.
>>
>> Please use a simple KEY_LID_OPEN event you will send through
>> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
>> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows
>> how to handle.
>>
>> > +
>> > +During the period the userspace hasn't been switched to use the new
>> > +SW_ACPI_LID event, Linux users can use the following boot parameter to
>> > +handle possible issues:
>> > +  button.lid_init_state=method:
>> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>> > +   reports the initial lid state using the returning value of the _LID
>> > +   control method.
>> > +   This can be used to fix some platforms if the _LID control method's
>> > +   returning value is reliable.
>> > +  button.lid_init_state=open:
>> > +   Linux kernel always reports the initial lid state as "opened".
>> > +   This may fix some platforms if the returning value of the _LID control
>> > +   method is not reliable.
>>
>> This worries me as there is no plan after "During the period the
>> userspace hasn't been switched to use the new event".
>>
>> I really hope you'll keep sending SW_LID for reliable LID platforms,
>> and not remove it entirely as you will break platforms.
>
> How about we leave the kernel alone and userspace (which would have to
> cope with the new KEY_LID_OPEN anyway) would just have to know that if
> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then it
> can't trust the events and it needs additional heuristics.
>

I really wished we could leave the kernel alone, but some platform
need fixes: we are using an EV_SW, and on those platform, we only get
the close event, which means it gets ignored by the input layer.
Those platform have a form factor which makes the situation acceptable
(tablet with cover, or very low cost laptops).
However, switching away from a different EV_SW and tell userspace to
ignore it would break systems when you are using a docking station.
The acpi would provide fake values for the LID switch and this will
screw the session if you are working on a docking station with an
external monitor and the LID closed.

My initial suggestion was:
- detect in the kernel whether the ACPI LID information was judged as reliable
- if reliable keep things as it
- if not reliable add an extra KEY_LID_CLOSE to notify userspace that
the SW_LID is not reliable (it will emulate the LID open events) and
that it will only get true close events.

After further thoughts, I think we can simply add the extra key, and
have an hwdb entry in logind to enumerate the devices only relying on
the close event. If the userspace is not updated, we can tell user to
use the button.lid_init_state=open quirk to simulate the behavior
logind should expect.

That means only adding one extra key (and its events) in the kernel
and let userspace decide what to do.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires July 11, 2016, 11:42 a.m. UTC | #6
On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>> method lid device restrictions
>>
>> Hi,
>>
>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > There are many AML tables reporting wrong initial lid state, and some of
>> > them never reports lid state. As a proxy layer acting between, ACPI
>> button
>> > driver is not able to handle all such cases, but need to re-define the
>> > usage model of the ACPI lid. That is:
>> > 1. It's initial state is not reliable;
>> > 2. There may not be open event;
>> > 3. Userspace should only take action against the close event which is
>> >    reliable, always sent after a real lid close.
>> > This patch adds documentation of the usage model.
>> >
>> > Link: https://lkml.org/2016/3/7/460
>> > Link: 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>
>> > Cc: linux-input@vger.kernel.org
>> > ---
>> >  Documentation/acpi/acpi-lid.txt |   62
>> +++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 62 insertions(+)
>> >  create mode 100644 Documentation/acpi/acpi-lid.txt
>> >
>> > diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-
>> lid.txt
>> > new file mode 100644
>> > index 0000000..7e4f7ed
>> > --- /dev/null
>> > +++ b/Documentation/acpi/acpi-lid.txt
>> > @@ -0,0 +1,62 @@
>> > +Usage Model of the ACPI Control Method Lid Device
>> > +
>> > +Copyright (C) 2016, Intel Corporation
>> > +Author: Lv Zheng <lv.zheng@intel.com>
>> > +
>> > +
>> > +Abstract:
>> > +
>> > +Platforms containing lids convey lid state (open/close) to OSPMs using
>> a
>> > +control method lid device. To implement this, the AML tables issue
>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
>> > +changed. The _LID control method for the lid device must be
>> implemented to
>> > +report the "current" state of the lid as either "opened" or "closed".
>> > +
>> > +This document describes the restrictions and the expections of the
>> Linux
>> > +ACPI lid device driver.
>> > +
>> > +
>> > +1. Restrictions of the returning value of the _LID control method
>> > +
>> > +The _LID control method is described to return the "current" lid state.
>> > +However the word of "current" has ambiguity, many AML tables return
>> the lid
>> > +state upon the last lid notification instead of returning the lid state
>> > +upon the last _LID evaluation. There won't be difference when the _LID
>> > +control method is evaluated during the runtime, the problem is its
>> initial
>> > +returning value. When the AML tables implement this control method
>> with
>> > +cached value, the initial returning value is likely not reliable. There are
>> > +simply so many examples always retuning "closed" as initial lid state.
>> > +
>> > +2. Restrictions of the lid state change notifications
>> > +
>> > +There are many AML tables never notifying when the lid device state is
>> > +changed to "opened". But it is ensured that the AML tables always
>> notify
>> > +"closed" when the lid state is changed to "closed". This is normally used
>> > +to trigger some system power saving operations on Windows. Since it is
>> > +fully tested, this notification is reliable for all AML tables.
>> > +
>> > +3. Expections for the userspace users of the ACPI lid device driver
>> > +
>> > +The userspace programs should stop relying on
>> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
>> > +used for the validation purpose.
>>
>> I'd say: this file actually calls the _LID method described above. And
>> given the previous explanation, it is not reliable enough on some
>> platforms. So it is strongly advised for user-space program to not
>> solely rely on this file to determine the actual lid state.
> [Lv Zheng]
> OK.
>
>>
>> > +
>> > +New userspace programs should rely on the lid "closed" notification to
>> > +trigger some power saving operations and may stop taking actions
>> according
>> > +to the lid "opened" notification. A new input switch event -
>> SW_ACPI_LID is
>> > +prepared for the new userspace to implement this ACPI control method
>> lid
>> > +device specific logics.
>>
>> That's not entirely what we discussed before (to prevent regressions):
>> - if the device doesn't have reliable LID switch state, then there
>> would be the new input event, and so userspace should only rely on
>> opened notifications.
>> - if the device has reliable switch information, the new input event
>> should not be exported and userspace knows that the current input
>> switch event is reliable.
>>
>> Also, using a new "switch" event is a terrible idea. Switches have a
>> state (open/close) and you are using this to forward a single open
>> event. So using a switch just allows you to say to userspace you are
>> using the "new" LID meaning, but you'll still have to manually reset
>> the switch and you will have to document how this event is not a
>> switch.
>>
>> Please use a simple KEY_LID_OPEN event you will send through
>> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
>> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows
>> how to handle.
> [Lv Zheng]
> It should be KEY_LID_CLOSE.

yep, sorry.

> However I checked the KEY code definitions.
> It seems their values are highly dependent on the HID specification.

That was convenient enough when the code was written. Now, we can
extend new keycodes as we want, as long as Dmitry agrees :)

> I'm not sure which key code value should I use for this.
> Could you point me out?
>


>>
>> > +
>> > +During the period the userspace hasn't been switched to use the new
>> > +SW_ACPI_LID event, Linux users can use the following boot parameter
>> to
>> > +handle possible issues:
>> > +  button.lid_init_state=method:
>> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>> > +   reports the initial lid state using the returning value of the _LID
>> > +   control method.
>> > +   This can be used to fix some platforms if the _LID control method's
>> > +   returning value is reliable.
>> > +  button.lid_init_state=open:
>> > +   Linux kernel always reports the initial lid state as "opened".
>> > +   This may fix some platforms if the returning value of the _LID control
>> > +   method is not reliable.
>>
>> This worries me as there is no plan after "During the period the
>> userspace hasn't been switched to use the new event".
>>
>> I really hope you'll keep sending SW_LID for reliable LID platforms,
>> and not remove it entirely as you will break platforms.
>
> [Lv Zheng]
> We won't remove SW_LID from the kernel :).
>
> And we haven't removed SW_LID from the acpi button driver.
> We'll just stop sending "initial lid state" from acpi button driver, i.e., the behavior carried out by "button.lid_init_state=ignore".
>
> Maybe it is not sufficient, after the userspace has been changed to support the new event, we should stop sending SW_LID from acpi button driver.
>
> Cheers,
> -Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires July 11, 2016, 11:47 a.m. UTC | #7
[I just realised Ctrl+enter means "send" for gmail, see the end of the answers]

On Mon, Jul 11, 2016 at 1:42 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> Hi,
>>
>>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
>>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>>> method lid device restrictions
>>>
>>> Hi,
>>>
>>> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>>> > There are many AML tables reporting wrong initial lid state, and some of
>>> > them never reports lid state. As a proxy layer acting between, ACPI
>>> button
>>> > driver is not able to handle all such cases, but need to re-define the
>>> > usage model of the ACPI lid. That is:
>>> > 1. It's initial state is not reliable;
>>> > 2. There may not be open event;
>>> > 3. Userspace should only take action against the close event which is
>>> >    reliable, always sent after a real lid close.
>>> > This patch adds documentation of the usage model.
>>> >
>>> > Link: https://lkml.org/2016/3/7/460
>>> > Link: 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>
>>> > Cc: linux-input@vger.kernel.org
>>> > ---
>>> >  Documentation/acpi/acpi-lid.txt |   62
>>> +++++++++++++++++++++++++++++++++++++++
>>> >  1 file changed, 62 insertions(+)
>>> >  create mode 100644 Documentation/acpi/acpi-lid.txt
>>> >
>>> > diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-
>>> lid.txt
>>> > new file mode 100644
>>> > index 0000000..7e4f7ed
>>> > --- /dev/null
>>> > +++ b/Documentation/acpi/acpi-lid.txt
>>> > @@ -0,0 +1,62 @@
>>> > +Usage Model of the ACPI Control Method Lid Device
>>> > +
>>> > +Copyright (C) 2016, Intel Corporation
>>> > +Author: Lv Zheng <lv.zheng@intel.com>
>>> > +
>>> > +
>>> > +Abstract:
>>> > +
>>> > +Platforms containing lids convey lid state (open/close) to OSPMs using
>>> a
>>> > +control method lid device. To implement this, the AML tables issue
>>> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
>>> > +changed. The _LID control method for the lid device must be
>>> implemented to
>>> > +report the "current" state of the lid as either "opened" or "closed".
>>> > +
>>> > +This document describes the restrictions and the expections of the
>>> Linux
>>> > +ACPI lid device driver.
>>> > +
>>> > +
>>> > +1. Restrictions of the returning value of the _LID control method
>>> > +
>>> > +The _LID control method is described to return the "current" lid state.
>>> > +However the word of "current" has ambiguity, many AML tables return
>>> the lid
>>> > +state upon the last lid notification instead of returning the lid state
>>> > +upon the last _LID evaluation. There won't be difference when the _LID
>>> > +control method is evaluated during the runtime, the problem is its
>>> initial
>>> > +returning value. When the AML tables implement this control method
>>> with
>>> > +cached value, the initial returning value is likely not reliable. There are
>>> > +simply so many examples always retuning "closed" as initial lid state.
>>> > +
>>> > +2. Restrictions of the lid state change notifications
>>> > +
>>> > +There are many AML tables never notifying when the lid device state is
>>> > +changed to "opened". But it is ensured that the AML tables always
>>> notify
>>> > +"closed" when the lid state is changed to "closed". This is normally used
>>> > +to trigger some system power saving operations on Windows. Since it is
>>> > +fully tested, this notification is reliable for all AML tables.
>>> > +
>>> > +3. Expections for the userspace users of the ACPI lid device driver
>>> > +
>>> > +The userspace programs should stop relying on
>>> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
>>> > +used for the validation purpose.
>>>
>>> I'd say: this file actually calls the _LID method described above. And
>>> given the previous explanation, it is not reliable enough on some
>>> platforms. So it is strongly advised for user-space program to not
>>> solely rely on this file to determine the actual lid state.
>> [Lv Zheng]
>> OK.
>>
>>>
>>> > +
>>> > +New userspace programs should rely on the lid "closed" notification to
>>> > +trigger some power saving operations and may stop taking actions
>>> according
>>> > +to the lid "opened" notification. A new input switch event -
>>> SW_ACPI_LID is
>>> > +prepared for the new userspace to implement this ACPI control method
>>> lid
>>> > +device specific logics.
>>>
>>> That's not entirely what we discussed before (to prevent regressions):
>>> - if the device doesn't have reliable LID switch state, then there
>>> would be the new input event, and so userspace should only rely on
>>> opened notifications.
>>> - if the device has reliable switch information, the new input event
>>> should not be exported and userspace knows that the current input
>>> switch event is reliable.
>>>
>>> Also, using a new "switch" event is a terrible idea. Switches have a
>>> state (open/close) and you are using this to forward a single open
>>> event. So using a switch just allows you to say to userspace you are
>>> using the "new" LID meaning, but you'll still have to manually reset
>>> the switch and you will have to document how this event is not a
>>> switch.
>>>
>>> Please use a simple KEY_LID_OPEN event you will send through
>>> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
>>> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows
>>> how to handle.
>> [Lv Zheng]
>> It should be KEY_LID_CLOSE.
>
> yep, sorry.
>
>> However I checked the KEY code definitions.
>> It seems their values are highly dependent on the HID specification.
>
> That was convenient enough when the code was written. Now, we can
> extend new keycodes as we want, as long as Dmitry agrees :)
>
>> I'm not sure which key code value should I use for this.
>> Could you point me out?
>>
>

I think using 0x277 (or 0x278 given that KEY_DATA is reusing
KEY_FASTREVERSE) would be fine.

>
>>>
>>> > +
>>> > +During the period the userspace hasn't been switched to use the new
>>> > +SW_ACPI_LID event, Linux users can use the following boot parameter
>>> to
>>> > +handle possible issues:
>>> > +  button.lid_init_state=method:
>>> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>>> > +   reports the initial lid state using the returning value of the _LID
>>> > +   control method.
>>> > +   This can be used to fix some platforms if the _LID control method's
>>> > +   returning value is reliable.
>>> > +  button.lid_init_state=open:
>>> > +   Linux kernel always reports the initial lid state as "opened".
>>> > +   This may fix some platforms if the returning value of the _LID control
>>> > +   method is not reliable.
>>>
>>> This worries me as there is no plan after "During the period the
>>> userspace hasn't been switched to use the new event".
>>>
>>> I really hope you'll keep sending SW_LID for reliable LID platforms,
>>> and not remove it entirely as you will break platforms.
>>
>> [Lv Zheng]
>> We won't remove SW_LID from the kernel :).
>>
>> And we haven't removed SW_LID from the acpi button driver.
>> We'll just stop sending "initial lid state" from acpi button driver, i.e., the behavior carried out by "button.lid_init_state=ignore".

That won't do for the very same use case than before. It makes sense
to boot a laptop while the LID is closed if you have an external
monitor plugged in (the docking station allows to have an extra power
button accessible when the lid is closed).

>>
>> Maybe it is not sufficient, after the userspace has been changed to support the new event, we should stop sending SW_LID from acpi button driver.

I'd say do not touch SW_LID at all (and allow users to tweak its
behavior for local fixes, which is what you currently have).
Just send the extra KEY_LID_CLOSE, no matter what.
And start listing which devices have troubles, and we can add those
into a hwdb file shipped with logind. I hope the systemd team will
agree with me.

Cheers,
Benjamin

>>
>> Cheers,
>> -Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov July 12, 2016, 12:41 a.m. UTC | #8
On Mon, Jul 11, 2016 at 01:34:08PM +0200, Benjamin Tissoires wrote:
> On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> >> Hi,
> >>
> >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> >> > There are many AML tables reporting wrong initial lid state, and some of
> >> > them never reports lid state. As a proxy layer acting between, ACPI button
> >> > driver is not able to handle all such cases, but need to re-define the
> >> > usage model of the ACPI lid. That is:
> >> > 1. It's initial state is not reliable;
> >> > 2. There may not be open event;
> >> > 3. Userspace should only take action against the close event which is
> >> >    reliable, always sent after a real lid close.
> >> > This patch adds documentation of the usage model.
> >> >
> >> > Link: https://lkml.org/2016/3/7/460
> >> > Link: 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>
> >> > Cc: linux-input@vger.kernel.org
> >> > ---
> >> >  Documentation/acpi/acpi-lid.txt |   62 +++++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 62 insertions(+)
> >> >  create mode 100644 Documentation/acpi/acpi-lid.txt
> >> >
> >> > diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
> >> > new file mode 100644
> >> > index 0000000..7e4f7ed
> >> > --- /dev/null
> >> > +++ b/Documentation/acpi/acpi-lid.txt
> >> > @@ -0,0 +1,62 @@
> >> > +Usage Model of the ACPI Control Method Lid Device
> >> > +
> >> > +Copyright (C) 2016, Intel Corporation
> >> > +Author: Lv Zheng <lv.zheng@intel.com>
> >> > +
> >> > +
> >> > +Abstract:
> >> > +
> >> > +Platforms containing lids convey lid state (open/close) to OSPMs using a
> >> > +control method lid device. To implement this, the AML tables issue
> >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
> >> > +changed. The _LID control method for the lid device must be implemented to
> >> > +report the "current" state of the lid as either "opened" or "closed".
> >> > +
> >> > +This document describes the restrictions and the expections of the Linux
> >> > +ACPI lid device driver.
> >> > +
> >> > +
> >> > +1. Restrictions of the returning value of the _LID control method
> >> > +
> >> > +The _LID control method is described to return the "current" lid state.
> >> > +However the word of "current" has ambiguity, many AML tables return the lid
> >> > +state upon the last lid notification instead of returning the lid state
> >> > +upon the last _LID evaluation. There won't be difference when the _LID
> >> > +control method is evaluated during the runtime, the problem is its initial
> >> > +returning value. When the AML tables implement this control method with
> >> > +cached value, the initial returning value is likely not reliable. There are
> >> > +simply so many examples always retuning "closed" as initial lid state.
> >> > +
> >> > +2. Restrictions of the lid state change notifications
> >> > +
> >> > +There are many AML tables never notifying when the lid device state is
> >> > +changed to "opened". But it is ensured that the AML tables always notify
> >> > +"closed" when the lid state is changed to "closed". This is normally used
> >> > +to trigger some system power saving operations on Windows. Since it is
> >> > +fully tested, this notification is reliable for all AML tables.
> >> > +
> >> > +3. Expections for the userspace users of the ACPI lid device driver
> >> > +
> >> > +The userspace programs should stop relying on
> >> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
> >> > +used for the validation purpose.
> >>
> >> I'd say: this file actually calls the _LID method described above. And
> >> given the previous explanation, it is not reliable enough on some
> >> platforms. So it is strongly advised for user-space program to not
> >> solely rely on this file to determine the actual lid state.
> >>
> >> > +
> >> > +New userspace programs should rely on the lid "closed" notification to
> >> > +trigger some power saving operations and may stop taking actions according
> >> > +to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
> >> > +prepared for the new userspace to implement this ACPI control method lid
> >> > +device specific logics.
> >>
> >> That's not entirely what we discussed before (to prevent regressions):
> >> - if the device doesn't have reliable LID switch state, then there
> >> would be the new input event, and so userspace should only rely on
> >> opened notifications.
> >> - if the device has reliable switch information, the new input event
> >> should not be exported and userspace knows that the current input
> >> switch event is reliable.
> >>
> >> Also, using a new "switch" event is a terrible idea. Switches have a
> >> state (open/close) and you are using this to forward a single open
> >> event. So using a switch just allows you to say to userspace you are
> >> using the "new" LID meaning, but you'll still have to manually reset
> >> the switch and you will have to document how this event is not a
> >> switch.
> >>
> >> Please use a simple KEY_LID_OPEN event you will send through
> >> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
> >> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace knows
> >> how to handle.
> >>
> >> > +
> >> > +During the period the userspace hasn't been switched to use the new
> >> > +SW_ACPI_LID event, Linux users can use the following boot parameter to
> >> > +handle possible issues:
> >> > +  button.lid_init_state=method:
> >> > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> >> > +   reports the initial lid state using the returning value of the _LID
> >> > +   control method.
> >> > +   This can be used to fix some platforms if the _LID control method's
> >> > +   returning value is reliable.
> >> > +  button.lid_init_state=open:
> >> > +   Linux kernel always reports the initial lid state as "opened".
> >> > +   This may fix some platforms if the returning value of the _LID control
> >> > +   method is not reliable.
> >>
> >> This worries me as there is no plan after "During the period the
> >> userspace hasn't been switched to use the new event".
> >>
> >> I really hope you'll keep sending SW_LID for reliable LID platforms,
> >> and not remove it entirely as you will break platforms.
> >
> > How about we leave the kernel alone and userspace (which would have to
> > cope with the new KEY_LID_OPEN anyway) would just have to know that if
> > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then it
> > can't trust the events and it needs additional heuristics.
> >
> 
> I really wished we could leave the kernel alone, but some platform
> need fixes: we are using an EV_SW, and on those platform, we only get
> the close event, which means it gets ignored by the input layer.

OK. Can we then emit missing "open" if we get "close" and the state is
already closed?

Thanks.
Lv Zheng July 12, 2016, 7:06 a.m. UTC | #9
Hi,

> From: Bastien Nocera [mailto:hadess@hadess.net]

> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control

> method lid device restrictions

> 

> On Mon, 2016-07-11 at 03:20 +0000, Zheng, Lv wrote:

> >

> <snip>

> > > This worries me as there is no plan after "During the period the

> > > userspace hasn't been switched to use the new event".

> > >

> > > I really hope you'll keep sending SW_LID for reliable LID

> > > platforms,

> > > and not remove it entirely as you will break platforms.

> >

> > [Lv Zheng]

> > We won't remove SW_LID from the kernel :).

> >

> > And we haven't removed SW_LID from the acpi button driver.

> > We'll just stop sending "initial lid state" from acpi button driver,

> > i.e., the behavior carried out by "button.lid_init_state=ignore".

> >

> > Maybe it is not sufficient, after the userspace has been changed to

> > support the new event, we should stop sending SW_LID from acpi button

> > driver.

> 

> For the affected devices? Sure, but I don't think that's a reasonable

> thing to do for "all" the devices. We have a majority of laptops where

> this isn't a problem, and it's not even a problem any more on one of

> the devices that triggered this discussion (there's a patch for make

> the LID status match reality for the Surface 3).

[Lv Zheng] 
It looks, even with this fixed, there are tables never generating "lid open" event.
Thus the lid notification is definitely not a "switch event".

Thanks and best regards
-Lv
Lv Zheng July 12, 2016, 7:43 a.m. UTC | #10
Hi, Dmitry

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Mon, Jul 11, 2016 at 01:34:08PM +0200, Benjamin Tissoires wrote:
> > On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> > >> Hi,
> > >>
> > >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
> wrote:
> > >> > There are many AML tables reporting wrong initial lid state, and
> some of
> > >> > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > >> > driver is not able to handle all such cases, but need to re-define the
> > >> > usage model of the ACPI lid. That is:
> > >> > 1. It's initial state is not reliable;
> > >> > 2. There may not be open event;
> > >> > 3. Userspace should only take action against the close event which
> is
> > >> >    reliable, always sent after a real lid close.
> > >> > This patch adds documentation of the usage model.
> > >> >
> > >> > Link: https://lkml.org/2016/3/7/460
> > >> > Link: 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>
> > >> > Cc: linux-input@vger.kernel.org
> > >> > ---
> > >> >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> > >> >  1 file changed, 62 insertions(+)
> > >> >  create mode 100644 Documentation/acpi/acpi-lid.txt
> > >> >
> > >> > diff --git a/Documentation/acpi/acpi-lid.txt
> b/Documentation/acpi/acpi-lid.txt
> > >> > new file mode 100644
> > >> > index 0000000..7e4f7ed
> > >> > --- /dev/null
> > >> > +++ b/Documentation/acpi/acpi-lid.txt
> > >> > @@ -0,0 +1,62 @@
> > >> > +Usage Model of the ACPI Control Method Lid Device
> > >> > +
> > >> > +Copyright (C) 2016, Intel Corporation
> > >> > +Author: Lv Zheng <lv.zheng@intel.com>
> > >> > +
> > >> > +
> > >> > +Abstract:
> > >> > +
> > >> > +Platforms containing lids convey lid state (open/close) to OSPMs
> using a
> > >> > +control method lid device. To implement this, the AML tables issue
> > >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
> state has
> > >> > +changed. The _LID control method for the lid device must be
> implemented to
> > >> > +report the "current" state of the lid as either "opened" or "closed".
> > >> > +
> > >> > +This document describes the restrictions and the expections of the
> Linux
> > >> > +ACPI lid device driver.
> > >> > +
> > >> > +
> > >> > +1. Restrictions of the returning value of the _LID control method
> > >> > +
> > >> > +The _LID control method is described to return the "current" lid
> state.
> > >> > +However the word of "current" has ambiguity, many AML tables
> return the lid
> > >> > +state upon the last lid notification instead of returning the lid state
> > >> > +upon the last _LID evaluation. There won't be difference when the
> _LID
> > >> > +control method is evaluated during the runtime, the problem is its
> initial
> > >> > +returning value. When the AML tables implement this control
> method with
> > >> > +cached value, the initial returning value is likely not reliable. There
> are
> > >> > +simply so many examples always retuning "closed" as initial lid
> state.
> > >> > +
> > >> > +2. Restrictions of the lid state change notifications
> > >> > +
> > >> > +There are many AML tables never notifying when the lid device
> state is
> > >> > +changed to "opened". But it is ensured that the AML tables always
> notify
> > >> > +"closed" when the lid state is changed to "closed". This is normally
> used
> > >> > +to trigger some system power saving operations on Windows.
> Since it is
> > >> > +fully tested, this notification is reliable for all AML tables.
> > >> > +
> > >> > +3. Expections for the userspace users of the ACPI lid device driver
> > >> > +
> > >> > +The userspace programs should stop relying on
> > >> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is
> only
> > >> > +used for the validation purpose.
> > >>
> > >> I'd say: this file actually calls the _LID method described above. And
> > >> given the previous explanation, it is not reliable enough on some
> > >> platforms. So it is strongly advised for user-space program to not
> > >> solely rely on this file to determine the actual lid state.
> > >>
> > >> > +
> > >> > +New userspace programs should rely on the lid "closed"
> notification to
> > >> > +trigger some power saving operations and may stop taking actions
> according
> > >> > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > >> > +prepared for the new userspace to implement this ACPI control
> method lid
> > >> > +device specific logics.
> > >>
> > >> That's not entirely what we discussed before (to prevent regressions):
> > >> - if the device doesn't have reliable LID switch state, then there
> > >> would be the new input event, and so userspace should only rely on
> > >> opened notifications.
> > >> - if the device has reliable switch information, the new input event
> > >> should not be exported and userspace knows that the current input
> > >> switch event is reliable.
> > >>
> > >> Also, using a new "switch" event is a terrible idea. Switches have a
> > >> state (open/close) and you are using this to forward a single open
> > >> event. So using a switch just allows you to say to userspace you are
> > >> using the "new" LID meaning, but you'll still have to manually reset
> > >> the switch and you will have to document how this event is not a
> > >> switch.
> > >>
> > >> Please use a simple KEY_LID_OPEN event you will send through
> > >> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
> > >> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace
> knows
> > >> how to handle.
> > >>
> > >> > +
> > >> > +During the period the userspace hasn't been switched to use the
> new
> > >> > +SW_ACPI_LID event, Linux users can use the following boot
> parameter to
> > >> > +handle possible issues:
> > >> > +  button.lid_init_state=method:
> > >> > +   This is the default behavior of the Linux ACPI lid driver, Linux
> kernel
> > >> > +   reports the initial lid state using the returning value of the _LID
> > >> > +   control method.
> > >> > +   This can be used to fix some platforms if the _LID control
> method's
> > >> > +   returning value is reliable.
> > >> > +  button.lid_init_state=open:
> > >> > +   Linux kernel always reports the initial lid state as "opened".
> > >> > +   This may fix some platforms if the returning value of the _LID
> control
> > >> > +   method is not reliable.
> > >>
> > >> This worries me as there is no plan after "During the period the
> > >> userspace hasn't been switched to use the new event".
> > >>
> > >> I really hope you'll keep sending SW_LID for reliable LID platforms,
> > >> and not remove it entirely as you will break platforms.
> > >
> > > How about we leave the kernel alone and userspace (which would have
> to
> > > cope with the new KEY_LID_OPEN anyway) would just have to know
> that if
> > > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
> then it
> > > can't trust the events and it needs additional heuristics.
> > >
> >
> > I really wished we could leave the kernel alone, but some platform
> > need fixes: we are using an EV_SW, and on those platform, we only get
> > the close event, which means it gets ignored by the input layer.
> 
> OK. Can we then emit missing "open" if we get "close" and the state is
> already closed?
[Lv Zheng] 
The problem is systemd/logind thinks SW_LID is a switch event, thus it must be paired.
While the real world is: there are platforms, SW_LID may not be paired for ACPI control method lid device.

As the events are provided by the BIOS tables, the Linux ACPI subsystem actually has no idea on how to get the missing "open" back.
Unless we hire people to wait for the bug reports and fix the BIOS tables.

Thanks and best regards
-Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng July 19, 2016, 7:17 a.m. UTC | #11
Hi, Dmitry

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> > Hi,
> >
> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
> > > There are many AML tables reporting wrong initial lid state, and some
> of
> > > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > > driver is not able to handle all such cases, but need to re-define the
> > > usage model of the ACPI lid. That is:
> > > 1. It's initial state is not reliable;
> > > 2. There may not be open event;
> > > 3. Userspace should only take action against the close event which is
> > >    reliable, always sent after a real lid close.
> > > This patch adds documentation of the usage model.
> > >
> > > Link: https://lkml.org/2016/3/7/460
> > > Link: 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>
> > > Cc: linux-input@vger.kernel.org
> > > ---
> > >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > >  create mode 100644 Documentation/acpi/acpi-lid.txt
> > >
> > > diff --git a/Documentation/acpi/acpi-lid.txt
> b/Documentation/acpi/acpi-lid.txt
> > > new file mode 100644
> > > index 0000000..7e4f7ed
> > > --- /dev/null
> > > +++ b/Documentation/acpi/acpi-lid.txt
> > > @@ -0,0 +1,62 @@
> > > +Usage Model of the ACPI Control Method Lid Device
> > > +
> > > +Copyright (C) 2016, Intel Corporation
> > > +Author: Lv Zheng <lv.zheng@intel.com>
> > > +
> > > +
> > > +Abstract:
> > > +
> > > +Platforms containing lids convey lid state (open/close) to OSPMs
> using a
> > > +control method lid device. To implement this, the AML tables issue
> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state
> has
> > > +changed. The _LID control method for the lid device must be
> implemented to
> > > +report the "current" state of the lid as either "opened" or "closed".
> > > +
> > > +This document describes the restrictions and the expections of the
> Linux
> > > +ACPI lid device driver.
> > > +
> > > +
> > > +1. Restrictions of the returning value of the _LID control method
> > > +
> > > +The _LID control method is described to return the "current" lid state.
> > > +However the word of "current" has ambiguity, many AML tables
> return the lid
> > > +state upon the last lid notification instead of returning the lid state
> > > +upon the last _LID evaluation. There won't be difference when the
> _LID
> > > +control method is evaluated during the runtime, the problem is its
> initial
> > > +returning value. When the AML tables implement this control method
> with
> > > +cached value, the initial returning value is likely not reliable. There are
> > > +simply so many examples always retuning "closed" as initial lid state.
> > > +
> > > +2. Restrictions of the lid state change notifications
> > > +
> > > +There are many AML tables never notifying when the lid device state
> is
> > > +changed to "opened". But it is ensured that the AML tables always
> notify
> > > +"closed" when the lid state is changed to "closed". This is normally
> used
> > > +to trigger some system power saving operations on Windows. Since it
> is
> > > +fully tested, this notification is reliable for all AML tables.
> > > +
> > > +3. Expections for the userspace users of the ACPI lid device driver
> > > +
> > > +The userspace programs should stop relying on
> > > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is
> only
> > > +used for the validation purpose.
> >
> > I'd say: this file actually calls the _LID method described above. And
> > given the previous explanation, it is not reliable enough on some
> > platforms. So it is strongly advised for user-space program to not
> > solely rely on this file to determine the actual lid state.
> >
> > > +
> > > +New userspace programs should rely on the lid "closed" notification
> to
> > > +trigger some power saving operations and may stop taking actions
> according
> > > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > > +prepared for the new userspace to implement this ACPI control
> method lid
> > > +device specific logics.
> >
> > That's not entirely what we discussed before (to prevent regressions):
> > - if the device doesn't have reliable LID switch state, then there
> > would be the new input event, and so userspace should only rely on
> > opened notifications.
> > - if the device has reliable switch information, the new input event
> > should not be exported and userspace knows that the current input
> > switch event is reliable.
> >
> > Also, using a new "switch" event is a terrible idea. Switches have a
> > state (open/close) and you are using this to forward a single open
> > event. So using a switch just allows you to say to userspace you are
> > using the "new" LID meaning, but you'll still have to manually reset
> > the switch and you will have to document how this event is not a
> > switch.
> >
> > Please use a simple KEY_LID_OPEN event you will send through
> > [input_key_event(KEY_LID_OPEN, 1), input_sync(),
> > input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace
> knows
> > how to handle.
> >
> > > +
> > > +During the period the userspace hasn't been switched to use the new
> > > +SW_ACPI_LID event, Linux users can use the following boot parameter
> to
> > > +handle possible issues:
> > > +  button.lid_init_state=method:
> > > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
> > > +   reports the initial lid state using the returning value of the _LID
> > > +   control method.
> > > +   This can be used to fix some platforms if the _LID control method's
> > > +   returning value is reliable.
> > > +  button.lid_init_state=open:
> > > +   Linux kernel always reports the initial lid state as "opened".
> > > +   This may fix some platforms if the returning value of the _LID
> control
> > > +   method is not reliable.
> >
> > This worries me as there is no plan after "During the period the
> > userspace hasn't been switched to use the new event".
> >
> > I really hope you'll keep sending SW_LID for reliable LID platforms,
> > and not remove it entirely as you will break platforms.
> 
> How about we leave the kernel alone and userspace (which would have to
> cope with the new KEY_LID_OPEN anyway) would just have to know that if
> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then
> it
> can't trust the events and it needs additional heuristics.

[Lv Zheng] 
I found a problem with the key event approach.
And need your suggestions.

Some AML tables invoke Notify(lid_device, ...) in several different places.
It may be invoked from different functions.

Finally, it's not guaranteed that one "lid close" action can only trigger one key close notification.
If we use EV_KEY, then there should be many platforms triggering multiple "lid close" events to the user space.

Original switch event based design can automatically eliminate the redundant events.

Does input layer has an event type that can handle such situation?
Or shall ACPI button driver handle this?

Thanks,
-Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires July 19, 2016, 8:40 a.m. UTC | #12
On Tue, Jul 19, 2016 at 9:17 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Dmitry
>
>> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>> method lid device restrictions
>>
>> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
>> > Hi,
>> >
>> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com> wrote:
>> > > There are many AML tables reporting wrong initial lid state, and some
>> of
>> > > them never reports lid state. As a proxy layer acting between, ACPI
>> button
>> > > driver is not able to handle all such cases, but need to re-define the
>> > > usage model of the ACPI lid. That is:
>> > > 1. It's initial state is not reliable;
>> > > 2. There may not be open event;
>> > > 3. Userspace should only take action against the close event which is
>> > >    reliable, always sent after a real lid close.
>> > > This patch adds documentation of the usage model.
>> > >
>> > > Link: https://lkml.org/2016/3/7/460
>> > > Link: 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>
>> > > Cc: linux-input@vger.kernel.org
>> > > ---
>> > >  Documentation/acpi/acpi-lid.txt |   62
>> +++++++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 62 insertions(+)
>> > >  create mode 100644 Documentation/acpi/acpi-lid.txt
>> > >
>> > > diff --git a/Documentation/acpi/acpi-lid.txt
>> b/Documentation/acpi/acpi-lid.txt
>> > > new file mode 100644
>> > > index 0000000..7e4f7ed
>> > > --- /dev/null
>> > > +++ b/Documentation/acpi/acpi-lid.txt
>> > > @@ -0,0 +1,62 @@
>> > > +Usage Model of the ACPI Control Method Lid Device
>> > > +
>> > > +Copyright (C) 2016, Intel Corporation
>> > > +Author: Lv Zheng <lv.zheng@intel.com>
>> > > +
>> > > +
>> > > +Abstract:
>> > > +
>> > > +Platforms containing lids convey lid state (open/close) to OSPMs
>> using a
>> > > +control method lid device. To implement this, the AML tables issue
>> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state
>> has
>> > > +changed. The _LID control method for the lid device must be
>> implemented to
>> > > +report the "current" state of the lid as either "opened" or "closed".
>> > > +
>> > > +This document describes the restrictions and the expections of the
>> Linux
>> > > +ACPI lid device driver.
>> > > +
>> > > +
>> > > +1. Restrictions of the returning value of the _LID control method
>> > > +
>> > > +The _LID control method is described to return the "current" lid state.
>> > > +However the word of "current" has ambiguity, many AML tables
>> return the lid
>> > > +state upon the last lid notification instead of returning the lid state
>> > > +upon the last _LID evaluation. There won't be difference when the
>> _LID
>> > > +control method is evaluated during the runtime, the problem is its
>> initial
>> > > +returning value. When the AML tables implement this control method
>> with
>> > > +cached value, the initial returning value is likely not reliable. There are
>> > > +simply so many examples always retuning "closed" as initial lid state.
>> > > +
>> > > +2. Restrictions of the lid state change notifications
>> > > +
>> > > +There are many AML tables never notifying when the lid device state
>> is
>> > > +changed to "opened". But it is ensured that the AML tables always
>> notify
>> > > +"closed" when the lid state is changed to "closed". This is normally
>> used
>> > > +to trigger some system power saving operations on Windows. Since it
>> is
>> > > +fully tested, this notification is reliable for all AML tables.
>> > > +
>> > > +3. Expections for the userspace users of the ACPI lid device driver
>> > > +
>> > > +The userspace programs should stop relying on
>> > > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is
>> only
>> > > +used for the validation purpose.
>> >
>> > I'd say: this file actually calls the _LID method described above. And
>> > given the previous explanation, it is not reliable enough on some
>> > platforms. So it is strongly advised for user-space program to not
>> > solely rely on this file to determine the actual lid state.
>> >
>> > > +
>> > > +New userspace programs should rely on the lid "closed" notification
>> to
>> > > +trigger some power saving operations and may stop taking actions
>> according
>> > > +to the lid "opened" notification. A new input switch event -
>> SW_ACPI_LID is
>> > > +prepared for the new userspace to implement this ACPI control
>> method lid
>> > > +device specific logics.
>> >
>> > That's not entirely what we discussed before (to prevent regressions):
>> > - if the device doesn't have reliable LID switch state, then there
>> > would be the new input event, and so userspace should only rely on
>> > opened notifications.
>> > - if the device has reliable switch information, the new input event
>> > should not be exported and userspace knows that the current input
>> > switch event is reliable.
>> >
>> > Also, using a new "switch" event is a terrible idea. Switches have a
>> > state (open/close) and you are using this to forward a single open
>> > event. So using a switch just allows you to say to userspace you are
>> > using the "new" LID meaning, but you'll still have to manually reset
>> > the switch and you will have to document how this event is not a
>> > switch.
>> >
>> > Please use a simple KEY_LID_OPEN event you will send through
>> > [input_key_event(KEY_LID_OPEN, 1), input_sync(),
>> > input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace
>> knows
>> > how to handle.
>> >
>> > > +
>> > > +During the period the userspace hasn't been switched to use the new
>> > > +SW_ACPI_LID event, Linux users can use the following boot parameter
>> to
>> > > +handle possible issues:
>> > > +  button.lid_init_state=method:
>> > > +   This is the default behavior of the Linux ACPI lid driver, Linux kernel
>> > > +   reports the initial lid state using the returning value of the _LID
>> > > +   control method.
>> > > +   This can be used to fix some platforms if the _LID control method's
>> > > +   returning value is reliable.
>> > > +  button.lid_init_state=open:
>> > > +   Linux kernel always reports the initial lid state as "opened".
>> > > +   This may fix some platforms if the returning value of the _LID
>> control
>> > > +   method is not reliable.
>> >
>> > This worries me as there is no plan after "During the period the
>> > userspace hasn't been switched to use the new event".
>> >
>> > I really hope you'll keep sending SW_LID for reliable LID platforms,
>> > and not remove it entirely as you will break platforms.
>>
>> How about we leave the kernel alone and userspace (which would have to
>> cope with the new KEY_LID_OPEN anyway) would just have to know that if
>> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0) then
>> it
>> can't trust the events and it needs additional heuristics.
>
> [Lv Zheng]
> I found a problem with the key event approach.
> And need your suggestions.
>
> Some AML tables invoke Notify(lid_device, ...) in several different places.
> It may be invoked from different functions.
>
> Finally, it's not guaranteed that one "lid close" action can only trigger one key close notification.
> If we use EV_KEY, then there should be many platforms triggering multiple "lid close" events to the user space.
>
> Original switch event based design can automatically eliminate the redundant events.
>
> Does input layer has an event type that can handle such situation?
> Or shall ACPI button driver handle this?
>

Keys also have some redundant event elimination, but it's as long as
you are holding the key in the press (or released) position. So in
your case, that would mean sending input_key(1), wait a little while
other notifications are processed, and then sending input_key(0)
(assuming each notification comes in with its own thread). Not sure
you will gain anything from the new implementation you just sent with
the rate-limit.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng July 19, 2016, 8:57 a.m. UTC | #13
Hi, Benjamin

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]

> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control

> method lid device restrictions

> 

> On Tue, Jul 19, 2016 at 9:17 AM, Zheng, Lv <lv.zheng@intel.com> wrote:

> > Hi, Dmitry

> >

> >> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]

> >> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI

> control

> >> method lid device restrictions

> >>

> >> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:

> >> > Hi,

> >> >

> >> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>

> wrote:

> >> > > There are many AML tables reporting wrong initial lid state, and

> some

> >> of

> >> > > them never reports lid state. As a proxy layer acting between, ACPI

> >> button

> >> > > driver is not able to handle all such cases, but need to re-define the

> >> > > usage model of the ACPI lid. That is:

> >> > > 1. It's initial state is not reliable;

> >> > > 2. There may not be open event;

> >> > > 3. Userspace should only take action against the close event which

> is

> >> > >    reliable, always sent after a real lid close.

> >> > > This patch adds documentation of the usage model.

> >> > >

> >> > > Link: https://lkml.org/2016/3/7/460

> >> > > Link: 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>

> >> > > Cc: linux-input@vger.kernel.org

> >> > > ---

> >> > >  Documentation/acpi/acpi-lid.txt |   62

> >> +++++++++++++++++++++++++++++++++++++++

> >> > >  1 file changed, 62 insertions(+)

> >> > >  create mode 100644 Documentation/acpi/acpi-lid.txt

> >> > >

> >> > > diff --git a/Documentation/acpi/acpi-lid.txt

> >> b/Documentation/acpi/acpi-lid.txt

> >> > > new file mode 100644

> >> > > index 0000000..7e4f7ed

> >> > > --- /dev/null

> >> > > +++ b/Documentation/acpi/acpi-lid.txt

> >> > > @@ -0,0 +1,62 @@

> >> > > +Usage Model of the ACPI Control Method Lid Device

> >> > > +

> >> > > +Copyright (C) 2016, Intel Corporation

> >> > > +Author: Lv Zheng <lv.zheng@intel.com>

> >> > > +

> >> > > +

> >> > > +Abstract:

> >> > > +

> >> > > +Platforms containing lids convey lid state (open/close) to OSPMs

> >> using a

> >> > > +control method lid device. To implement this, the AML tables issue

> >> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid

> state

> >> has

> >> > > +changed. The _LID control method for the lid device must be

> >> implemented to

> >> > > +report the "current" state of the lid as either "opened" or "closed".

> >> > > +

> >> > > +This document describes the restrictions and the expections of the

> >> Linux

> >> > > +ACPI lid device driver.

> >> > > +

> >> > > +

> >> > > +1. Restrictions of the returning value of the _LID control method

> >> > > +

> >> > > +The _LID control method is described to return the "current" lid

> state.

> >> > > +However the word of "current" has ambiguity, many AML tables

> >> return the lid

> >> > > +state upon the last lid notification instead of returning the lid state

> >> > > +upon the last _LID evaluation. There won't be difference when the

> >> _LID

> >> > > +control method is evaluated during the runtime, the problem is its

> >> initial

> >> > > +returning value. When the AML tables implement this control

> method

> >> with

> >> > > +cached value, the initial returning value is likely not reliable. There

> are

> >> > > +simply so many examples always retuning "closed" as initial lid

> state.

> >> > > +

> >> > > +2. Restrictions of the lid state change notifications

> >> > > +

> >> > > +There are many AML tables never notifying when the lid device

> state

> >> is

> >> > > +changed to "opened". But it is ensured that the AML tables always

> >> notify

> >> > > +"closed" when the lid state is changed to "closed". This is normally

> >> used

> >> > > +to trigger some system power saving operations on Windows.

> Since it

> >> is

> >> > > +fully tested, this notification is reliable for all AML tables.

> >> > > +

> >> > > +3. Expections for the userspace users of the ACPI lid device driver

> >> > > +

> >> > > +The userspace programs should stop relying on

> >> > > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is

> >> only

> >> > > +used for the validation purpose.

> >> >

> >> > I'd say: this file actually calls the _LID method described above. And

> >> > given the previous explanation, it is not reliable enough on some

> >> > platforms. So it is strongly advised for user-space program to not

> >> > solely rely on this file to determine the actual lid state.

> >> >

> >> > > +

> >> > > +New userspace programs should rely on the lid "closed"

> notification

> >> to

> >> > > +trigger some power saving operations and may stop taking actions

> >> according

> >> > > +to the lid "opened" notification. A new input switch event -

> >> SW_ACPI_LID is

> >> > > +prepared for the new userspace to implement this ACPI control

> >> method lid

> >> > > +device specific logics.

> >> >

> >> > That's not entirely what we discussed before (to prevent regressions):

> >> > - if the device doesn't have reliable LID switch state, then there

> >> > would be the new input event, and so userspace should only rely on

> >> > opened notifications.

> >> > - if the device has reliable switch information, the new input event

> >> > should not be exported and userspace knows that the current input

> >> > switch event is reliable.

> >> >

> >> > Also, using a new "switch" event is a terrible idea. Switches have a

> >> > state (open/close) and you are using this to forward a single open

> >> > event. So using a switch just allows you to say to userspace you are

> >> > using the "new" LID meaning, but you'll still have to manually reset

> >> > the switch and you will have to document how this event is not a

> >> > switch.

> >> >

> >> > Please use a simple KEY_LID_OPEN event you will send through

> >> > [input_key_event(KEY_LID_OPEN, 1), input_sync(),

> >> > input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace

> >> knows

> >> > how to handle.

> >> >

> >> > > +

> >> > > +During the period the userspace hasn't been switched to use the

> new

> >> > > +SW_ACPI_LID event, Linux users can use the following boot

> parameter

> >> to

> >> > > +handle possible issues:

> >> > > +  button.lid_init_state=method:

> >> > > +   This is the default behavior of the Linux ACPI lid driver, Linux

> kernel

> >> > > +   reports the initial lid state using the returning value of the _LID

> >> > > +   control method.

> >> > > +   This can be used to fix some platforms if the _LID control

> method's

> >> > > +   returning value is reliable.

> >> > > +  button.lid_init_state=open:

> >> > > +   Linux kernel always reports the initial lid state as "opened".

> >> > > +   This may fix some platforms if the returning value of the _LID

> >> control

> >> > > +   method is not reliable.

> >> >

> >> > This worries me as there is no plan after "During the period the

> >> > userspace hasn't been switched to use the new event".

> >> >

> >> > I really hope you'll keep sending SW_LID for reliable LID platforms,

> >> > and not remove it entirely as you will break platforms.

> >>

> >> How about we leave the kernel alone and userspace (which would have

> to

> >> cope with the new KEY_LID_OPEN anyway) would just have to know

> that if

> >> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)

> then

> >> it

> >> can't trust the events and it needs additional heuristics.

> >

> > [Lv Zheng]

> > I found a problem with the key event approach.

> > And need your suggestions.

> >

> > Some AML tables invoke Notify(lid_device, ...) in several different places.

> > It may be invoked from different functions.

> >

> > Finally, it's not guaranteed that one "lid close" action can only trigger

> one key close notification.

> > If we use EV_KEY, then there should be many platforms triggering

> multiple "lid close" events to the user space.

> >

> > Original switch event based design can automatically eliminate the

> redundant events.

> >

> > Does input layer has an event type that can handle such situation?

> > Or shall ACPI button driver handle this?

> >

> 

> Keys also have some redundant event elimination, but it's as long as

> you are holding the key in the press (or released) position. So in

> your case, that would mean sending input_key(1), wait a little while

> other notifications are processed, and then sending input_key(0)

> (assuming each notification comes in with its own thread). Not sure

> you will gain anything from the new implementation you just sent with

> the rate-limit.


[Lv Zheng] 
However this key is virtual.
The multiple notifications are just triggered by the AML code.
The Notify(lid_device, xxx) may be invoked in a function.
And this function may be invoked multiple times by other control methods.
So I do not know when it is "released".

Using the feature of the keys, it sounds like that I should setup a timer.
When the state is changed, I should report input_key(1) and prepare the timer.
Then report the input_key(0) when the timer times out.
The side effect is the input_key(0) will be deferred.

I just refreshed the patch as v4 with an ACPI button driver internal workaround.
By adding a time_after() check which looks more lightweight than setting up a timer.
Could you also help to check if that solution is OK?
Thanks in advance.

Best regards
-Lv
Benjamin Tissoires July 19, 2016, 9:07 a.m. UTC | #14
On Tue, Jul 19, 2016 at 10:57 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Benjamin
>
>> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
>> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
>> method lid device restrictions
>>
>> On Tue, Jul 19, 2016 at 9:17 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> > Hi, Dmitry
>> >
>> >> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> >> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI
>> control
>> >> method lid device restrictions
>> >>
>> >> On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
>> >> > Hi,
>> >> >
>> >> > On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
>> wrote:
>> >> > > There are many AML tables reporting wrong initial lid state, and
>> some
>> >> of
>> >> > > them never reports lid state. As a proxy layer acting between, ACPI
>> >> button
>> >> > > driver is not able to handle all such cases, but need to re-define the
>> >> > > usage model of the ACPI lid. That is:
>> >> > > 1. It's initial state is not reliable;
>> >> > > 2. There may not be open event;
>> >> > > 3. Userspace should only take action against the close event which
>> is
>> >> > >    reliable, always sent after a real lid close.
>> >> > > This patch adds documentation of the usage model.
>> >> > >
>> >> > > Link: https://lkml.org/2016/3/7/460
>> >> > > Link: 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>
>> >> > > Cc: linux-input@vger.kernel.org
>> >> > > ---
>> >> > >  Documentation/acpi/acpi-lid.txt |   62
>> >> +++++++++++++++++++++++++++++++++++++++
>> >> > >  1 file changed, 62 insertions(+)
>> >> > >  create mode 100644 Documentation/acpi/acpi-lid.txt
>> >> > >
>> >> > > diff --git a/Documentation/acpi/acpi-lid.txt
>> >> b/Documentation/acpi/acpi-lid.txt
>> >> > > new file mode 100644
>> >> > > index 0000000..7e4f7ed
>> >> > > --- /dev/null
>> >> > > +++ b/Documentation/acpi/acpi-lid.txt
>> >> > > @@ -0,0 +1,62 @@
>> >> > > +Usage Model of the ACPI Control Method Lid Device
>> >> > > +
>> >> > > +Copyright (C) 2016, Intel Corporation
>> >> > > +Author: Lv Zheng <lv.zheng@intel.com>
>> >> > > +
>> >> > > +
>> >> > > +Abstract:
>> >> > > +
>> >> > > +Platforms containing lids convey lid state (open/close) to OSPMs
>> >> using a
>> >> > > +control method lid device. To implement this, the AML tables issue
>> >> > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
>> state
>> >> has
>> >> > > +changed. The _LID control method for the lid device must be
>> >> implemented to
>> >> > > +report the "current" state of the lid as either "opened" or "closed".
>> >> > > +
>> >> > > +This document describes the restrictions and the expections of the
>> >> Linux
>> >> > > +ACPI lid device driver.
>> >> > > +
>> >> > > +
>> >> > > +1. Restrictions of the returning value of the _LID control method
>> >> > > +
>> >> > > +The _LID control method is described to return the "current" lid
>> state.
>> >> > > +However the word of "current" has ambiguity, many AML tables
>> >> return the lid
>> >> > > +state upon the last lid notification instead of returning the lid state
>> >> > > +upon the last _LID evaluation. There won't be difference when the
>> >> _LID
>> >> > > +control method is evaluated during the runtime, the problem is its
>> >> initial
>> >> > > +returning value. When the AML tables implement this control
>> method
>> >> with
>> >> > > +cached value, the initial returning value is likely not reliable. There
>> are
>> >> > > +simply so many examples always retuning "closed" as initial lid
>> state.
>> >> > > +
>> >> > > +2. Restrictions of the lid state change notifications
>> >> > > +
>> >> > > +There are many AML tables never notifying when the lid device
>> state
>> >> is
>> >> > > +changed to "opened". But it is ensured that the AML tables always
>> >> notify
>> >> > > +"closed" when the lid state is changed to "closed". This is normally
>> >> used
>> >> > > +to trigger some system power saving operations on Windows.
>> Since it
>> >> is
>> >> > > +fully tested, this notification is reliable for all AML tables.
>> >> > > +
>> >> > > +3. Expections for the userspace users of the ACPI lid device driver
>> >> > > +
>> >> > > +The userspace programs should stop relying on
>> >> > > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is
>> >> only
>> >> > > +used for the validation purpose.
>> >> >
>> >> > I'd say: this file actually calls the _LID method described above. And
>> >> > given the previous explanation, it is not reliable enough on some
>> >> > platforms. So it is strongly advised for user-space program to not
>> >> > solely rely on this file to determine the actual lid state.
>> >> >
>> >> > > +
>> >> > > +New userspace programs should rely on the lid "closed"
>> notification
>> >> to
>> >> > > +trigger some power saving operations and may stop taking actions
>> >> according
>> >> > > +to the lid "opened" notification. A new input switch event -
>> >> SW_ACPI_LID is
>> >> > > +prepared for the new userspace to implement this ACPI control
>> >> method lid
>> >> > > +device specific logics.
>> >> >
>> >> > That's not entirely what we discussed before (to prevent regressions):
>> >> > - if the device doesn't have reliable LID switch state, then there
>> >> > would be the new input event, and so userspace should only rely on
>> >> > opened notifications.
>> >> > - if the device has reliable switch information, the new input event
>> >> > should not be exported and userspace knows that the current input
>> >> > switch event is reliable.
>> >> >
>> >> > Also, using a new "switch" event is a terrible idea. Switches have a
>> >> > state (open/close) and you are using this to forward a single open
>> >> > event. So using a switch just allows you to say to userspace you are
>> >> > using the "new" LID meaning, but you'll still have to manually reset
>> >> > the switch and you will have to document how this event is not a
>> >> > switch.
>> >> >
>> >> > Please use a simple KEY_LID_OPEN event you will send through
>> >> > [input_key_event(KEY_LID_OPEN, 1), input_sync(),
>> >> > input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace
>> >> knows
>> >> > how to handle.
>> >> >
>> >> > > +
>> >> > > +During the period the userspace hasn't been switched to use the
>> new
>> >> > > +SW_ACPI_LID event, Linux users can use the following boot
>> parameter
>> >> to
>> >> > > +handle possible issues:
>> >> > > +  button.lid_init_state=method:
>> >> > > +   This is the default behavior of the Linux ACPI lid driver, Linux
>> kernel
>> >> > > +   reports the initial lid state using the returning value of the _LID
>> >> > > +   control method.
>> >> > > +   This can be used to fix some platforms if the _LID control
>> method's
>> >> > > +   returning value is reliable.
>> >> > > +  button.lid_init_state=open:
>> >> > > +   Linux kernel always reports the initial lid state as "opened".
>> >> > > +   This may fix some platforms if the returning value of the _LID
>> >> control
>> >> > > +   method is not reliable.
>> >> >
>> >> > This worries me as there is no plan after "During the period the
>> >> > userspace hasn't been switched to use the new event".
>> >> >
>> >> > I really hope you'll keep sending SW_LID for reliable LID platforms,
>> >> > and not remove it entirely as you will break platforms.
>> >>
>> >> How about we leave the kernel alone and userspace (which would have
>> to
>> >> cope with the new KEY_LID_OPEN anyway) would just have to know
>> that if
>> >> switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
>> then
>> >> it
>> >> can't trust the events and it needs additional heuristics.
>> >
>> > [Lv Zheng]
>> > I found a problem with the key event approach.
>> > And need your suggestions.
>> >
>> > Some AML tables invoke Notify(lid_device, ...) in several different places.
>> > It may be invoked from different functions.
>> >
>> > Finally, it's not guaranteed that one "lid close" action can only trigger
>> one key close notification.
>> > If we use EV_KEY, then there should be many platforms triggering
>> multiple "lid close" events to the user space.
>> >
>> > Original switch event based design can automatically eliminate the
>> redundant events.
>> >
>> > Does input layer has an event type that can handle such situation?
>> > Or shall ACPI button driver handle this?
>> >
>>
>> Keys also have some redundant event elimination, but it's as long as
>> you are holding the key in the press (or released) position. So in
>> your case, that would mean sending input_key(1), wait a little while
>> other notifications are processed, and then sending input_key(0)
>> (assuming each notification comes in with its own thread). Not sure
>> you will gain anything from the new implementation you just sent with
>> the rate-limit.
>
> [Lv Zheng]
> However this key is virtual.
> The multiple notifications are just triggered by the AML code.
> The Notify(lid_device, xxx) may be invoked in a function.
> And this function may be invoked multiple times by other control methods.

Yes, I understood. In this particular case, when I sad "pressed" or
"released" please understand logical 1 or logical 0.

> So I do not know when it is "released".

Yep, that's the problem.

>
> Using the feature of the keys, it sounds like that I should setup a timer.
> When the state is changed, I should report input_key(1) and prepare the timer.
> Then report the input_key(0) when the timer times out.
> The side effect is the input_key(0) will be deferred.

That's not an issue. If it were an issue, logind can just get
triggered when it receives the key(1), and ignore the key(0). But
given that the action will likely be power suspend, I don't think even
a 500ms delay will be an issue.

>
> I just refreshed the patch as v4 with an ACPI button driver internal workaround.
> By adding a time_after() check which looks more lightweight than setting up a timer.
> Could you also help to check if that solution is OK?

Yes, that's fine by me. I already sent my rev-by.

Cheers,
Benjamin

> Thanks in advance.
>
> Best regards
> -Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng July 20, 2016, 3:21 a.m. UTC | #15
Hi, Dmitry

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> On Mon, Jul 11, 2016 at 01:34:08PM +0200, Benjamin Tissoires wrote:
> > On Fri, Jul 8, 2016 at 7:51 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Fri, Jul 08, 2016 at 11:17:39AM +0200, Benjamin Tissoires wrote:
> > >> Hi,
> > >>
> > >> On Thu, Jul 7, 2016 at 9:11 AM, Lv Zheng <lv.zheng@intel.com>
> wrote:
> > >> > There are many AML tables reporting wrong initial lid state, and
> some of
> > >> > them never reports lid state. As a proxy layer acting between, ACPI
> button
> > >> > driver is not able to handle all such cases, but need to re-define the
> > >> > usage model of the ACPI lid. That is:
> > >> > 1. It's initial state is not reliable;
> > >> > 2. There may not be open event;
> > >> > 3. Userspace should only take action against the close event which
> is
> > >> >    reliable, always sent after a real lid close.
> > >> > This patch adds documentation of the usage model.
> > >> >
> > >> > Link: https://lkml.org/2016/3/7/460
> > >> > Link: 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>
> > >> > Cc: linux-input@vger.kernel.org
> > >> > ---
> > >> >  Documentation/acpi/acpi-lid.txt |   62
> +++++++++++++++++++++++++++++++++++++++
> > >> >  1 file changed, 62 insertions(+)
> > >> >  create mode 100644 Documentation/acpi/acpi-lid.txt
> > >> >
> > >> > diff --git a/Documentation/acpi/acpi-lid.txt
> b/Documentation/acpi/acpi-lid.txt
> > >> > new file mode 100644
> > >> > index 0000000..7e4f7ed
> > >> > --- /dev/null
> > >> > +++ b/Documentation/acpi/acpi-lid.txt
> > >> > @@ -0,0 +1,62 @@
> > >> > +Usage Model of the ACPI Control Method Lid Device
> > >> > +
> > >> > +Copyright (C) 2016, Intel Corporation
> > >> > +Author: Lv Zheng <lv.zheng@intel.com>
> > >> > +
> > >> > +
> > >> > +Abstract:
> > >> > +
> > >> > +Platforms containing lids convey lid state (open/close) to OSPMs
> using a
> > >> > +control method lid device. To implement this, the AML tables issue
> > >> > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
> state has
> > >> > +changed. The _LID control method for the lid device must be
> implemented to
> > >> > +report the "current" state of the lid as either "opened" or "closed".
> > >> > +
> > >> > +This document describes the restrictions and the expections of the
> Linux
> > >> > +ACPI lid device driver.
> > >> > +
> > >> > +
> > >> > +1. Restrictions of the returning value of the _LID control method
> > >> > +
> > >> > +The _LID control method is described to return the "current" lid
> state.
> > >> > +However the word of "current" has ambiguity, many AML tables
> return the lid
> > >> > +state upon the last lid notification instead of returning the lid state
> > >> > +upon the last _LID evaluation. There won't be difference when the
> _LID
> > >> > +control method is evaluated during the runtime, the problem is its
> initial
> > >> > +returning value. When the AML tables implement this control
> method with
> > >> > +cached value, the initial returning value is likely not reliable. There
> are
> > >> > +simply so many examples always retuning "closed" as initial lid
> state.
> > >> > +
> > >> > +2. Restrictions of the lid state change notifications
> > >> > +
> > >> > +There are many AML tables never notifying when the lid device
> state is
> > >> > +changed to "opened". But it is ensured that the AML tables always
> notify
> > >> > +"closed" when the lid state is changed to "closed". This is normally
> used
> > >> > +to trigger some system power saving operations on Windows.
> Since it is
> > >> > +fully tested, this notification is reliable for all AML tables.
> > >> > +
> > >> > +3. Expections for the userspace users of the ACPI lid device driver
> > >> > +
> > >> > +The userspace programs should stop relying on
> > >> > +/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is
> only
> > >> > +used for the validation purpose.
> > >>
> > >> I'd say: this file actually calls the _LID method described above. And
> > >> given the previous explanation, it is not reliable enough on some
> > >> platforms. So it is strongly advised for user-space program to not
> > >> solely rely on this file to determine the actual lid state.
> > >>
> > >> > +
> > >> > +New userspace programs should rely on the lid "closed"
> notification to
> > >> > +trigger some power saving operations and may stop taking actions
> according
> > >> > +to the lid "opened" notification. A new input switch event -
> SW_ACPI_LID is
> > >> > +prepared for the new userspace to implement this ACPI control
> method lid
> > >> > +device specific logics.
> > >>
> > >> That's not entirely what we discussed before (to prevent regressions):
> > >> - if the device doesn't have reliable LID switch state, then there
> > >> would be the new input event, and so userspace should only rely on
> > >> opened notifications.
> > >> - if the device has reliable switch information, the new input event
> > >> should not be exported and userspace knows that the current input
> > >> switch event is reliable.
> > >>
> > >> Also, using a new "switch" event is a terrible idea. Switches have a
> > >> state (open/close) and you are using this to forward a single open
> > >> event. So using a switch just allows you to say to userspace you are
> > >> using the "new" LID meaning, but you'll still have to manually reset
> > >> the switch and you will have to document how this event is not a
> > >> switch.
> > >>
> > >> Please use a simple KEY_LID_OPEN event you will send through
> > >> [input_key_event(KEY_LID_OPEN, 1), input_sync(),
> > >> input_key_event(KEY_LID_OPEN, 0), input_sync()], which userspace
> knows
> > >> how to handle.
> > >>
> > >> > +
> > >> > +During the period the userspace hasn't been switched to use the
> new
> > >> > +SW_ACPI_LID event, Linux users can use the following boot
> parameter to
> > >> > +handle possible issues:
> > >> > +  button.lid_init_state=method:
> > >> > +   This is the default behavior of the Linux ACPI lid driver, Linux
> kernel
> > >> > +   reports the initial lid state using the returning value of the _LID
> > >> > +   control method.
> > >> > +   This can be used to fix some platforms if the _LID control
> method's
> > >> > +   returning value is reliable.
> > >> > +  button.lid_init_state=open:
> > >> > +   Linux kernel always reports the initial lid state as "opened".
> > >> > +   This may fix some platforms if the returning value of the _LID
> control
> > >> > +   method is not reliable.
> > >>
> > >> This worries me as there is no plan after "During the period the
> > >> userspace hasn't been switched to use the new event".
> > >>
> > >> I really hope you'll keep sending SW_LID for reliable LID platforms,
> > >> and not remove it entirely as you will break platforms.
> > >
> > > How about we leave the kernel alone and userspace (which would have
> to
> > > cope with the new KEY_LID_OPEN anyway) would just have to know
> that if
> > > switch's parent is PNP0C0D:00 (or phys is PNP0C0D/button/input0)
> then it
> > > can't trust the events and it needs additional heuristics.
> > >
> >
> > I really wished we could leave the kernel alone, but some platform
> > need fixes: we are using an EV_SW, and on those platform, we only get
> > the close event, which means it gets ignored by the input layer.
> 
> OK. Can we then emit missing "open" if we get "close" and the state is
> already closed?
[Lv Zheng] 
I've been considering this again.
I think you may mean we can improve SW_LID by reporting SW_LID(open) when a new close event is arrived.
So that we may be able to fix old programs.

Possibly the code should be looked like:

If (!!state != last_state && time_after(jiffies, last_jiffies)) {
	input_report_switch(..., state);
}
input_report_switch(..., !state);

However, there are tables never reporting "open".
If we do things in this way, there will be a very long period between the last close and the next close.

Which means there will be a too long period between the close and the open switch events.
IMO, this cannot fix old user space programs.

Currently, logind has a timeout mechanism.
If it cannot receive "open" within this period, it will suspend the system again right after resuming.
Thus we can still see that suspending right after resuming could be resulted.

However the new key events allow the new user space to fix the issue without changing the timeout logic.

For the old user space programs, I have no idea how we can fix them on such platforms (no open events).

Thanks and best regards
-Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt
new file mode 100644
index 0000000..7e4f7ed
--- /dev/null
+++ b/Documentation/acpi/acpi-lid.txt
@@ -0,0 +1,62 @@ 
+Usage Model of the ACPI Control Method Lid Device
+
+Copyright (C) 2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+Platforms containing lids convey lid state (open/close) to OSPMs using a
+control method lid device. To implement this, the AML tables issue
+Notify(lid_device, 0x80) to notify the OSPMs whenever the lid state has
+changed. The _LID control method for the lid device must be implemented to
+report the "current" state of the lid as either "opened" or "closed".
+
+This document describes the restrictions and the expections of the Linux
+ACPI lid device driver.
+
+
+1. Restrictions of the returning value of the _LID control method
+
+The _LID control method is described to return the "current" lid state.
+However the word of "current" has ambiguity, many AML tables return the lid
+state upon the last lid notification instead of returning the lid state
+upon the last _LID evaluation. There won't be difference when the _LID
+control method is evaluated during the runtime, the problem is its initial
+returning value. When the AML tables implement this control method with
+cached value, the initial returning value is likely not reliable. There are
+simply so many examples always retuning "closed" as initial lid state.
+
+2. Restrictions of the lid state change notifications
+
+There are many AML tables never notifying when the lid device state is
+changed to "opened". But it is ensured that the AML tables always notify
+"closed" when the lid state is changed to "closed". This is normally used
+to trigger some system power saving operations on Windows. Since it is
+fully tested, this notification is reliable for all AML tables.
+
+3. Expections for the userspace users of the ACPI lid device driver
+
+The userspace programs should stop relying on
+/proc/acpi/button/lid/LID0/state to obtain the lid state. This file is only
+used for the validation purpose.
+
+New userspace programs should rely on the lid "closed" notification to
+trigger some power saving operations and may stop taking actions according
+to the lid "opened" notification. A new input switch event - SW_ACPI_LID is
+prepared for the new userspace to implement this ACPI control method lid
+device specific logics.
+
+During the period the userspace hasn't been switched to use the new
+SW_ACPI_LID event, Linux users can use the following boot parameter to
+handle possible issues:
+  button.lid_init_state=method:
+   This is the default behavior of the Linux ACPI lid driver, Linux kernel
+   reports the initial lid state using the returning value of the _LID
+   control method.
+   This can be used to fix some platforms if the _LID control method's
+   returning value is reliable.
+  button.lid_init_state=open:
+   Linux kernel always reports the initial lid state as "opened".
+   This may fix some platforms if the returning value of the _LID control
+   method is not reliable.