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

[Zheng, Lv]

Hi,

> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Benjamin Tissoires
> Subject: Re: [PATCH v2 4/4] ACPI / button: Add document for ACPI control
> method lid device restrictions
> 
> [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@xxxxxxxxx> wrote:
> > On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote:
> >> Hi,
> >>
> >>> From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxx]
> >>> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >>> > Cc: Bastien Nocera: <hadess@xxxxxxxxxx>
> >>> > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>
> >>> > Cc: linux-input@xxxxxxxxxxxxxxx
> >>> > ---
> >>> >  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@xxxxxxxxx>
> >>> > +
> >>> > +
> >>> > +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.
[Lv Zheng] 
Got it!
Thanks.

> 
> >
> >>>
> >>> > +
> >>> > +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).
[Lv Zheng] 
Exactly.
The "button.lid_init_state=ignore" is the original behavior of the ACPI button driver.

> 
> >>
> >> 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.
[Lv Zheng] 
OK.
We'll provide the dmidecode files for those platforms via an off-list way after the agreement is reached.
Thanks for the help.

Cheers
-Lv