From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751512AbcGLHgD (ORCPT ); Tue, 12 Jul 2016 03:36:03 -0400 Received: from mga11.intel.com ([192.55.52.93]:58978 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750787AbcGLHf6 (ORCPT ); Tue, 12 Jul 2016 03:35:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,350,1464678000"; d="scan'208";a="993459601" From: "Zheng, Lv" To: Benjamin Tissoires CC: "Wysocki, Rafael J" , "Rafael J. Wysocki" , "Brown, Len" , Lv Zheng , "linux-kernel@vger.kernel.org" , ACPI Devel Maling List , "Bastien Nocera:" , linux-input , Dmitry Torokhov Subject: RE: [PATCH v2 4/4] ACPI / button: Add document for ACPI control method lid device restrictions Thread-Topic: [PATCH v2 4/4] ACPI / button: Add document for ACPI control method lid device restrictions Thread-Index: AQHR2B7y+AgHsCraR0Gb3MzkbQGo+KANvMuAgATXRrCAAAhHgIAAAVCAgAHQP0A= Date: Tue, 12 Jul 2016 07:34:28 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E883BC02897@SHSMSX101.ccr.corp.intel.com> References: <3f24a00df89f06661a64af6b4679a99bfff09aa7.1467875143.git.lv.zheng@intel.com> <1AE640813FDE7649BE1B193DEA596E883BC02198@SHSMSX101.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjhmOTI2ZGQtZTJiYS00YjllLWE2ZDktNTI3NjI5MGU4MzJiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImVRVVlKT3VSd0RTOGV6TzZ3blVUeWx1TlVIdXFma1l2cXk3dGk2dkpRV1k9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u6C7a8CF025691 Hi, > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- > owner@vger.kernel.org] 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 > wrote: > > On Mon, Jul 11, 2016 at 5:20 AM, Zheng, Lv 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 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 > >>> > Cc: Bastien Nocera: > >>> > Cc: Benjamin Tissoires > >>> > 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 > >>> > + > >>> > + > >>> > +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