[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. > >>> >>> > + >>> > +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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
