| Message ID | 84ca7337d20620946b671f212abfc82698ddcbf4.1468915808.git.lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 19, 2016 at 10:11 AM, Lv Zheng <lv.zheng@intel.com> wrote: > This patch adds documentation for the usage model of the control method lid > device. > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > Cc: Bastien Nocera: <hadess@hadess.net> > Cc: linux-input@vger.kernel.org > --- Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cheers, Benjamin > Documentation/acpi/acpi-lid.txt | 89 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 89 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..2addedc > --- /dev/null > +++ b/Documentation/acpi/acpi-lid.txt > @@ -0,0 +1,89 @@ > +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". Thus the "opened" notification is not guaranteed. > + > +But it is guaranteed that the AML tables always notify "closed" when the > +lid state is changed to "closed". The "closed" notification is normally > +used to trigger some system power saving operations on Windows. Since it is > +fully tested, the "closed" notification is reliable for all AML tables. > + > +3. Expections for the userspace users of the ACPI lid device driver > + > +The ACPI button driver exports the lid state to the userspace via the > +following file: > + /proc/acpi/button/lid/LID0/state > +This file actually calls the _LID control method described above. And given > +the previous explanation, it is not reliable enough on some platforms. So > +it is advised for the userspace program to not to solely rely on this file > +to determine the actual lid state. > + > +The ACPI button driver emits 2 kinds of events to the user space: > + SW_LID > + When the lid state/event is reliable, the userspace can behave > + according to this input switch event. > + This is a mode prepared for backward compatiblity. > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > + When the lid state/event is not reliable, the userspace should behave > + according to these 2 input key events. > + New userspace programs may only be prepared for the input key events. > + > +If the userspace hasn't been prepared to handle the new input lid key > +events, Linux users can use the following kernel parameters to handle the > +possible issues triggered because of the unreliable lid state/event: > +A. button.lid_init_state=method: > + When this option is specified, the ACPI button driver reports the > + initial lid state using the returning value of the _LID control method. > + This option can be used to fix some platforms where the _LID control > + method's returning value is reliable but the initial lid state > + notification is missing. > + This option is the default behavior during the period the userspace > + isn't ready to handle the new usage model. > +B. button.lid_init_state=open: > + When this option is specified, the ACPI button driver always reports the > + initial lid state as "opened". > + This may fix some platforms where the returning value of the _LID > + control method is not reliable and the initial lid state notification is > + missing. > + > +If the userspace has been prepared to handle the new input lid key events, > +Linux users should always use the following kernel parameter: > +C. button.lid_init_state=ignore: > + When this option is specified, the ACPI button driver never reports the > + initial lid state. However, the platform may automatically report a > + correct initial lid state and there is no "open" event missing. When > + this is the case (everything is correctly implemented by the platform > + firmware), the old input switch event based userspace can still work. > + Otherwise, the userspace programs may only work based on the input key > + events. > + This option will be the default behavior after the userspace is ready to > + handle the new usage model. > -- > 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
On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > This patch adds documentation for the usage model of the control method lid > device. > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > Cc: Bastien Nocera: <hadess@hadess.net> > Cc: linux-input@vger.kernel.org > --- > Documentation/acpi/acpi-lid.txt | 89 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 89 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..2addedc > --- /dev/null > +++ b/Documentation/acpi/acpi-lid.txt > @@ -0,0 +1,89 @@ > +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 Can this be fixed in the next ACPI revision? > +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". Thus the "opened" notification is not guaranteed. > + > +But it is guaranteed that the AML tables always notify "closed" when the > +lid state is changed to "closed". The "closed" notification is normally > +used to trigger some system power saving operations on Windows. Since it is > +fully tested, the "closed" notification is reliable for all AML tables. > + > +3. Expections for the userspace users of the ACPI lid device driver > + > +The ACPI button driver exports the lid state to the userspace via the > +following file: > + /proc/acpi/button/lid/LID0/state > +This file actually calls the _LID control method described above. And given > +the previous explanation, it is not reliable enough on some platforms. So > +it is advised for the userspace program to not to solely rely on this file > +to determine the actual lid state. > + > +The ACPI button driver emits 2 kinds of events to the user space: > + SW_LID > + When the lid state/event is reliable, the userspace can behave > + according to this input switch event. > + This is a mode prepared for backward compatiblity. > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > + When the lid state/event is not reliable, the userspace should behave > + according to these 2 input key events. > + New userspace programs may only be prepared for the input key events. No, absolutely not. If some x86 vendors managed to mess up their firmware implementations that does not mean that everyone now has to abandon working perfectly well for them SW_LID events and rush to switch to a brand new event. Apparently were are a few issues, main is that some systems not reporting "open" event. This can be dealt with by userspace "writing" to the lid's evdev device EV_SW/SW_LID/0 event upon system resume (and startup) for selected systems. This will mean that if system wakes up not because LID is open we'll incorrectly assume that it is, but we can either add more smarts to the process emitting SW_LID event or simply say "well, tough, the hardware is crappy" and bug vendor to see if they can fix the issue (if not for current firmware them for next). As an additional workaround, we can toggle the LID switch off and on when we get notification, much like your proposed patch does for the key events. Speaking of ACPI in general, does Intel have a test suite for ACPI implementors? Could include tests for proper LID behavior? 1. The "swallowing" of input events because kernel state disagrees with the reality Thanks.
Hi, Dmitry Thanks for the review. > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > method lid device restrictions > > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > > This patch adds documentation for the usage model of the control > method lid > > device. > > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > Cc: Bastien Nocera: <hadess@hadess.net> > > Cc: linux-input@vger.kernel.org > > --- > > Documentation/acpi/acpi-lid.txt | 89 > +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 89 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..2addedc > > --- /dev/null > > +++ b/Documentation/acpi/acpi-lid.txt > > @@ -0,0 +1,89 @@ > > +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 > > Can this be fixed in the next ACPI revision? [Lv Zheng] Even this is fixed in the ACPI specification, there are platforms already doing this. Especially platforms from Microsoft. So the de-facto standard OS won't care about the change. And we can still see such platforms. Here is an example from Surface 3: DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) { Scope (_SB) { Device (PCI0) { Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID Device (SPI1) { Name (_HID, "8086228E") // _HID: Hardware ID Device (NTRG) { Name (_HID, "MSHW0037") // _HID: Hardware ID } } } Device (LID) { Name (_HID, EisaId ("PNP0C0D")) Name (LIDB, Zero) Method (_LID, 0, NotSerialized) { Return (LIDB) } } Device (GPO0) { Name (_HID, "INT33FF") // _HID: Hardware ID OperationRegion (GPOR, GeneralPurposeIo, Zero, One) Field (GPOR, ByteAcc, NoLock, Preserve) { Connection ( GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone, "\\_SB.GPO0", 0x00, ResourceConsumer, , ) { // Pin list 0x004C } ), HELD, 1 } Method (_E4C, 0, Serialized) { If (LEqual(HELD, One)) { Store(One, ^^LID.LIDB) There is no "open" event generated by "Surface 3". } Else { Store(Zero, ^^LID.LIDB) Notify (LID, 0x80) There is only "close" event generated by "Surface 3". Thus they are not paired. } Notify (^^PCI0.SPI1.NTRG, One) // Device Check } } } } > > > +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". Thus the "opened" notification is not guaranteed. > > + > > +But it is guaranteed that the AML tables always notify "closed" when > the > > +lid state is changed to "closed". The "closed" notification is normally > > +used to trigger some system power saving operations on Windows. > Since it is > > +fully tested, the "closed" notification is reliable for all AML tables. > > + > > +3. Expections for the userspace users of the ACPI lid device driver > > + > > +The ACPI button driver exports the lid state to the userspace via the > > +following file: > > + /proc/acpi/button/lid/LID0/state > > +This file actually calls the _LID control method described above. And > given > > +the previous explanation, it is not reliable enough on some platforms. > So > > +it is advised for the userspace program to not to solely rely on this file > > +to determine the actual lid state. > > + > > +The ACPI button driver emits 2 kinds of events to the user space: > > + SW_LID > > + When the lid state/event is reliable, the userspace can behave > > + according to this input switch event. > > + This is a mode prepared for backward compatiblity. > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > > + When the lid state/event is not reliable, the userspace should behave > > + according to these 2 input key events. > > + New userspace programs may only be prepared for the input key > events. > > No, absolutely not. If some x86 vendors managed to mess up their > firmware implementations that does not mean that everyone now has to > abandon working perfectly well for them SW_LID events and rush to > switch > to a brand new event. [Lv Zheng] However there is no clear wording in the ACPI specification asking the vendors to achieve paired lid events. > > Apparently were are a few issues, main is that some systems not reporting > "open" event. This can be dealt with by userspace "writing" to the > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and > startup) > for selected systems. This will mean that if system wakes up not because > LID is open we'll incorrectly assume that it is, but we can either add > more smarts to the process emitting SW_LID event or simply say "well, > tough, the hardware is crappy" and bug vendor to see if they can fix the > issue (if not for current firmware them for next). [Lv Zheng] The problem is there is no vendor actually caring about fixing this "issue". Because Windows works well with their firmware. Then finally becomes a big table customization business for our team. > > As an additional workaround, we can toggle the LID switch off and on > when we get notification, much like your proposed patch does for the key > events. [Lv Zheng] I think this is doable, I'll refresh my patchset to address your this comment. By inserting open/close events when next close/open event arrives after a certain period, this may fix some issues for the old programs. Where user may be required to open/close lid twice to trigger 2nd suspend. However, this still cannot fix the problems like "Surface 3". We'll still need a new usage model for such platforms (no open event). I'll send the next version out today, hope you can take a look to see if it's acceptable from your point of view. > > Speaking of ACPI in general, does Intel have a test suite for ACPI > implementors? Could include tests for proper LID behavior? > 1. The "swallowing" of input events because kernel state disagrees with > the reality [Lv Zheng] I think there is test suit in UEFI forum can cover this. However, most of the firmware vendors will just test their firmware with Windows. Thanks -Lv > > Thanks. > > -- > Dmitry -- 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
Hi Lv, On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > Hi, Dmitry > > > Thanks for the review. > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > > method lid device restrictions > > > > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > > > This patch adds documentation for the usage model of the control > > method lid > > > device. > > > > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > > Cc: Bastien Nocera: <hadess@hadess.net> > > > Cc: linux-input@vger.kernel.org > > > --- > > > Documentation/acpi/acpi-lid.txt | 89 > > +++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 89 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..2addedc > > > --- /dev/null > > > +++ b/Documentation/acpi/acpi-lid.txt > > > @@ -0,0 +1,89 @@ > > > +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 > > > > Can this be fixed in the next ACPI revision? > [Lv Zheng] > Even this is fixed in the ACPI specification, there are platforms already doing this. > Especially platforms from Microsoft. > So the de-facto standard OS won't care about the change. > And we can still see such platforms. > > Here is an example from Surface 3: > > DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) > { > Scope (_SB) > { > Device (PCI0) > { > Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > Device (SPI1) > { > Name (_HID, "8086228E") // _HID: Hardware ID > Device (NTRG) > { > Name (_HID, "MSHW0037") // _HID: Hardware ID > } > } > } > > Device (LID) > { > Name (_HID, EisaId ("PNP0C0D")) > Name (LIDB, Zero) Start with lid closed? In any case might be wrong. > Method (_LID, 0, NotSerialized) > { > Return (LIDB) So "_LID" returns the last state read by "_EC4". "_EC4" is edge-triggered and will be evaluated every time gpio changes state. > } > } > > Device (GPO0) > { > Name (_HID, "INT33FF") // _HID: Hardware ID > OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > Field (GPOR, ByteAcc, NoLock, Preserve) > { > Connection ( > GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone, > "\\_SB.GPO0", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x004C > } > ), > HELD, 1 Is it possible to read state of this GPIO from userspace on startup to correct the initial state? > } > Method (_E4C, 0, Serialized) > { > If (LEqual(HELD, One)) > { > Store(One, ^^LID.LIDB) > > There is no "open" event generated by "Surface 3". Right so we update the state correctly, we just forgot to send the notification. Nothing that polling can't fix. > > } > Else > { > Store(Zero, ^^LID.LIDB) > Notify (LID, 0x80) > > There is only "close" event generated by "Surface 3". > Thus they are not paired. > > } > Notify (^^PCI0.SPI1.NTRG, One) // Device Check > } > } > } > } > > > > > > +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". Thus the "opened" notification is not guaranteed. > > > + > > > +But it is guaranteed that the AML tables always notify "closed" when > > the > > > +lid state is changed to "closed". The "closed" notification is normally > > > +used to trigger some system power saving operations on Windows. > > Since it is > > > +fully tested, the "closed" notification is reliable for all AML tables. > > > + > > > +3. Expections for the userspace users of the ACPI lid device driver > > > + > > > +The ACPI button driver exports the lid state to the userspace via the > > > +following file: > > > + /proc/acpi/button/lid/LID0/state > > > +This file actually calls the _LID control method described above. And > > given > > > +the previous explanation, it is not reliable enough on some platforms. > > So > > > +it is advised for the userspace program to not to solely rely on this file > > > +to determine the actual lid state. > > > + > > > +The ACPI button driver emits 2 kinds of events to the user space: > > > + SW_LID > > > + When the lid state/event is reliable, the userspace can behave > > > + according to this input switch event. > > > + This is a mode prepared for backward compatiblity. > > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > > > + When the lid state/event is not reliable, the userspace should behave > > > + according to these 2 input key events. > > > + New userspace programs may only be prepared for the input key > > events. > > > > No, absolutely not. If some x86 vendors managed to mess up their > > firmware implementations that does not mean that everyone now has to > > abandon working perfectly well for them SW_LID events and rush to > > switch > > to a brand new event. > [Lv Zheng] > However there is no clear wording in the ACPI specification asking the vendors to achieve paired lid events. > > > > > Apparently were are a few issues, main is that some systems not reporting > > "open" event. This can be dealt with by userspace "writing" to the > > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and > > startup) > > for selected systems. This will mean that if system wakes up not because > > LID is open we'll incorrectly assume that it is, but we can either add > > more smarts to the process emitting SW_LID event or simply say "well, > > tough, the hardware is crappy" and bug vendor to see if they can fix the > > issue (if not for current firmware them for next). > [Lv Zheng] > The problem is there is no vendor actually caring about fixing this "issue". > Because Windows works well with their firmware. > Then finally becomes a big table customization business for our team. Well, OK. But you do not expect that we will redo up and down the stack lid handling just because MS messed up DSDT on Surface 3? No, let them know (they now care about Linux, right?) so Surface 4 works and quirk the behavior for Surface 3. > > > > > As an additional workaround, we can toggle the LID switch off and on > > when we get notification, much like your proposed patch does for the key > > events. > [Lv Zheng] > I think this is doable, I'll refresh my patchset to address your this comment. > By inserting open/close events when next close/open event arrives after a certain period, > this may fix some issues for the old programs. > Where user may be required to open/close lid twice to trigger 2nd suspend. > > However, this still cannot fix the problems like "Surface 3". > We'll still need a new usage model for such platforms (no open event). No, for surface 3 you simply need to add polling of "_LID" method to the button driver. What are the other devices that mess up lid handling? Thanks.
On Fri, Jul 22, 2016 at 6:37 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Lv, > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: >> Hi, Dmitry >> >> >> Thanks for the review. >> >> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] >> > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control >> > method lid device restrictions >> > >> > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: >> > > This patch adds documentation for the usage model of the control >> > method lid >> > > device. >> > > >> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> >> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> >> > > Cc: Bastien Nocera: <hadess@hadess.net> >> > > Cc: linux-input@vger.kernel.org >> > > --- >> > > Documentation/acpi/acpi-lid.txt | 89 >> > +++++++++++++++++++++++++++++++++++++++ >> > > 1 file changed, 89 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..2addedc >> > > --- /dev/null >> > > +++ b/Documentation/acpi/acpi-lid.txt >> > > @@ -0,0 +1,89 @@ >> > > +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 >> > >> > Can this be fixed in the next ACPI revision? >> [Lv Zheng] >> Even this is fixed in the ACPI specification, there are platforms already doing this. >> Especially platforms from Microsoft. >> So the de-facto standard OS won't care about the change. >> And we can still see such platforms. >> >> Here is an example from Surface 3: >> >> DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) >> { >> Scope (_SB) >> { >> Device (PCI0) >> { >> Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID >> Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID >> Device (SPI1) >> { >> Name (_HID, "8086228E") // _HID: Hardware ID >> Device (NTRG) >> { >> Name (_HID, "MSHW0037") // _HID: Hardware ID >> } >> } >> } >> >> Device (LID) >> { >> Name (_HID, EisaId ("PNP0C0D")) >> Name (LIDB, Zero) > > Start with lid closed? In any case might be wrong. Actually the initial value doesn't matter if the gpiochip triggers the _EC4 at boot, which it should (https://github.com/hadess/fedora-surface3-kernel/commit/13200f81662c1c0b58137947c3e6c000fe62a2ba, still unsubmitted) > >> Method (_LID, 0, NotSerialized) >> { >> Return (LIDB) > > So "_LID" returns the last state read by "_EC4". "_EC4" is > edge-triggered and will be evaluated every time gpio changes state. That's assuming the change happens while the system is on. If you go into suspend by closing the LID. Open it while on suspend and then hit the power button given that the system doesn't wake up when the lid is opened, the edge change was made while the system is asleep, and we are screwed (from an ACPI point of view). See my next comment for a solution. > >> } >> } >> >> Device (GPO0) >> { >> Name (_HID, "INT33FF") // _HID: Hardware ID >> OperationRegion (GPOR, GeneralPurposeIo, Zero, One) >> Field (GPOR, ByteAcc, NoLock, Preserve) >> { >> Connection ( >> GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone, >> "\\_SB.GPO0", 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x004C >> } >> ), >> HELD, 1 > > Is it possible to read state of this GPIO from userspace on startup to > correct the initial state? > >> } >> Method (_E4C, 0, Serialized) >> { >> If (LEqual(HELD, One)) >> { >> Store(One, ^^LID.LIDB) >> >> There is no "open" event generated by "Surface 3". > > Right so we update the state correctly, we just forgot to send the > notification. Nothing that polling can't fix. Actually, I have a better (though more hackish) way of avoiding polling: https://github.com/hadess/fedora-surface3-kernel/blob/5e5775b9bdc308d665064387e0b144ee48e7b243/0002-WIP-add-custom-surface3-platform-device-for-controll.patch Given that the notification is forwarded to the touchscreen anyway, we can unregister the generic (and buggy) acpi button driver for the LID and create our own based on this specific DSDT. We can also make sure the LID state is also correct because of the WMI method which allows to read the actual value of the GPIO connected to the cover without using the cached (and most of the time wrong) acpi LID.LIDB value. I still yet have to submit this, but with this patch, but we can consider the Surface 3 as working and not an issue anymore. > >> >> } >> Else >> { >> Store(Zero, ^^LID.LIDB) >> Notify (LID, 0x80) >> >> There is only "close" event generated by "Surface 3". >> Thus they are not paired. >> >> } >> Notify (^^PCI0.SPI1.NTRG, One) // Device Check >> } >> } >> } >> } >> >> > >> > > +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". Thus the "opened" notification is not guaranteed. >> > > + >> > > +But it is guaranteed that the AML tables always notify "closed" when >> > the >> > > +lid state is changed to "closed". The "closed" notification is normally >> > > +used to trigger some system power saving operations on Windows. >> > Since it is >> > > +fully tested, the "closed" notification is reliable for all AML tables. >> > > + >> > > +3. Expections for the userspace users of the ACPI lid device driver >> > > + >> > > +The ACPI button driver exports the lid state to the userspace via the >> > > +following file: >> > > + /proc/acpi/button/lid/LID0/state >> > > +This file actually calls the _LID control method described above. And >> > given >> > > +the previous explanation, it is not reliable enough on some platforms. >> > So >> > > +it is advised for the userspace program to not to solely rely on this file >> > > +to determine the actual lid state. >> > > + >> > > +The ACPI button driver emits 2 kinds of events to the user space: >> > > + SW_LID >> > > + When the lid state/event is reliable, the userspace can behave >> > > + according to this input switch event. >> > > + This is a mode prepared for backward compatiblity. >> > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE >> > > + When the lid state/event is not reliable, the userspace should behave >> > > + according to these 2 input key events. >> > > + New userspace programs may only be prepared for the input key >> > events. >> > >> > No, absolutely not. If some x86 vendors managed to mess up their >> > firmware implementations that does not mean that everyone now has to >> > abandon working perfectly well for them SW_LID events and rush to >> > switch >> > to a brand new event. >> [Lv Zheng] >> However there is no clear wording in the ACPI specification asking the vendors to achieve paired lid events. >> >> > >> > Apparently were are a few issues, main is that some systems not reporting >> > "open" event. This can be dealt with by userspace "writing" to the >> > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and >> > startup) >> > for selected systems. This will mean that if system wakes up not because >> > LID is open we'll incorrectly assume that it is, but we can either add >> > more smarts to the process emitting SW_LID event or simply say "well, >> > tough, the hardware is crappy" and bug vendor to see if they can fix the >> > issue (if not for current firmware them for next). >> [Lv Zheng] >> The problem is there is no vendor actually caring about fixing this "issue". >> Because Windows works well with their firmware. >> Then finally becomes a big table customization business for our team. > > Well, OK. But you do not expect that we will redo up and down the stack > lid handling just because MS messed up DSDT on Surface 3? No, let them > know (they now care about Linux, right?) so Surface 4 works and quirk > the behavior for Surface 3. > From what I understood, it was more than just the Surface 3. Other laptops were having issues and Lv's team gave up on fixing those machines. >> >> > >> > As an additional workaround, we can toggle the LID switch off and on >> > when we get notification, much like your proposed patch does for the key >> > events. I really don't like this approach. The problem being that we will fix the notifications to user space, but nothing will tell userspace that the LID state is known to be wrong. OTOH, I already agreed for a hwdb in userspace so I guess this point is moot. Having both events (one SW for reliable HW, always correct, and one KEY for unreliable HW) allows userspace to make a clear distinction between the working and non working events and they can continue to keep using the polling of the SW node without extra addition. Anyway, if the kernel doesn't want to (or can't) fix the actual issue (by making sure the DSDT is reliable), userspace needs to be changed so any solution will be acceptable. >> [Lv Zheng] >> I think this is doable, I'll refresh my patchset to address your this comment. >> By inserting open/close events when next close/open event arrives after a certain period, >> this may fix some issues for the old programs. >> Where user may be required to open/close lid twice to trigger 2nd suspend. >> >> However, this still cannot fix the problems like "Surface 3". >> We'll still need a new usage model for such platforms (no open event). > > No, for surface 3 you simply need to add polling of "_LID" method to the > button driver. > > What are the other devices that mess up lid handling? > I also would be interested in knowing how much issues you are facing compared to the average number of "good" laptops. IIRC, you talked about 3 (counting the Surface 3), but I believe you had more in mind. 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
Hi, Dmitry Thanks for the review. > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > method lid device restrictions > > Hi Lv, > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > > Hi, Dmitry > > > > > > Thanks for the review. > > > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI > control > > > method lid device restrictions > > > > > > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > > > > This patch adds documentation for the usage model of the control > > > method lid > > > > device. > > > > > > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > > > Cc: Bastien Nocera: <hadess@hadess.net> > > > > Cc: linux-input@vger.kernel.org > > > > --- > > > > Documentation/acpi/acpi-lid.txt | 89 > > > +++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 89 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..2addedc > > > > --- /dev/null > > > > +++ b/Documentation/acpi/acpi-lid.txt > > > > @@ -0,0 +1,89 @@ > > > > +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 > > > > > > Can this be fixed in the next ACPI revision? > > [Lv Zheng] > > Even this is fixed in the ACPI specification, there are platforms already > doing this. > > Especially platforms from Microsoft. > > So the de-facto standard OS won't care about the change. > > And we can still see such platforms. > > > > Here is an example from Surface 3: > > > > DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) > > { > > Scope (_SB) > > { > > Device (PCI0) > > { > > Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > > Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > > Device (SPI1) > > { > > Name (_HID, "8086228E") // _HID: Hardware ID > > Device (NTRG) > > { > > Name (_HID, "MSHW0037") // _HID: Hardware ID > > } > > } > > } > > > > Device (LID) > > { > > Name (_HID, EisaId ("PNP0C0D")) > > Name (LIDB, Zero) > > Start with lid closed? In any case might be wrong. [Lv Zheng] And we validated with qemu that during boot, Windows7 evaluates _LID once but doesn't get suspended because of this value. So we think Windows only suspends against "notification" not _LID evaluation result. > > > Method (_LID, 0, NotSerialized) > > { > > Return (LIDB) > > So "_LID" returns the last state read by "_EC4". "_EC4" is > edge-triggered and will be evaluated every time gpio changes state. [Lv Zheng] Right. > > > } > > } > > > > Device (GPO0) > > { > > Name (_HID, "INT33FF") // _HID: Hardware ID > > OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > > Field (GPOR, ByteAcc, NoLock, Preserve) > > { > > Connection ( > > GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone, > > "\\_SB.GPO0", 0x00, ResourceConsumer, , > > ) > > { // Pin list > > 0x004C > > } > > ), > > HELD, 1 > > Is it possible to read state of this GPIO from userspace on startup to > correct the initial state? [Lv Zheng] I think Benjamin has a proposal of fixing this in GPIO driver. > > > } > > Method (_E4C, 0, Serialized) > > { > > If (LEqual(HELD, One)) > > { > > Store(One, ^^LID.LIDB) > > > > There is no "open" event generated by "Surface 3". > > Right so we update the state correctly, we just forgot to send the > notification. Nothing that polling can't fix. > [Lv Zheng] However, polling is not efficient, and not power efficient. OTOH, according to the validation result, Windows never poll _LID. > > > > } > > Else > > { > > Store(Zero, ^^LID.LIDB) > > Notify (LID, 0x80) > > > > There is only "close" event generated by "Surface 3". > > Thus they are not paired. > > > > } > > Notify (^^PCI0.SPI1.NTRG, One) // Device Check > > } > > } > > } > > } > > > > > > > > > +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". Thus the "opened" notification is not > guaranteed. > > > > + > > > > +But it is guaranteed that the AML tables always notify "closed" > when > > > the > > > > +lid state is changed to "closed". The "closed" notification is normally > > > > +used to trigger some system power saving operations on Windows. > > > Since it is > > > > +fully tested, the "closed" notification is reliable for all AML tables. > > > > + > > > > +3. Expections for the userspace users of the ACPI lid device driver > > > > + > > > > +The ACPI button driver exports the lid state to the userspace via the > > > > +following file: > > > > + /proc/acpi/button/lid/LID0/state > > > > +This file actually calls the _LID control method described above. And > > > given > > > > +the previous explanation, it is not reliable enough on some > platforms. > > > So > > > > +it is advised for the userspace program to not to solely rely on this > file > > > > +to determine the actual lid state. > > > > + > > > > +The ACPI button driver emits 2 kinds of events to the user space: > > > > + SW_LID > > > > + When the lid state/event is reliable, the userspace can behave > > > > + according to this input switch event. > > > > + This is a mode prepared for backward compatiblity. > > > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > > > > + When the lid state/event is not reliable, the userspace should > behave > > > > + according to these 2 input key events. > > > > + New userspace programs may only be prepared for the input key > > > events. > > > > > > No, absolutely not. If some x86 vendors managed to mess up their > > > firmware implementations that does not mean that everyone now has > to > > > abandon working perfectly well for them SW_LID events and rush to > > > switch > > > to a brand new event. > > [Lv Zheng] > > However there is no clear wording in the ACPI specification asking the > vendors to achieve paired lid events. > > > > > > > > Apparently were are a few issues, main is that some systems not > reporting > > > "open" event. This can be dealt with by userspace "writing" to the > > > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and > > > startup) > > > for selected systems. This will mean that if system wakes up not > because > > > LID is open we'll incorrectly assume that it is, but we can either add > > > more smarts to the process emitting SW_LID event or simply say "well, > > > tough, the hardware is crappy" and bug vendor to see if they can fix > the > > > issue (if not for current firmware them for next). > > [Lv Zheng] > > The problem is there is no vendor actually caring about fixing this "issue". > > Because Windows works well with their firmware. > > Then finally becomes a big table customization business for our team. > > Well, OK. But you do not expect that we will redo up and down the stack > lid handling just because MS messed up DSDT on Surface 3? No, let them > know (they now care about Linux, right?) so Surface 4 works and quirk > the behavior for Surface 3. [Lv Zheng] I think there are other platforms broken. > > > > > > > > > As an additional workaround, we can toggle the LID switch off and on > > > when we get notification, much like your proposed patch does for the > key > > > events. > > [Lv Zheng] > > I think this is doable, I'll refresh my patchset to address your this > comment. > > By inserting open/close events when next close/open event arrives after > a certain period, > > this may fix some issues for the old programs. > > Where user may be required to open/close lid twice to trigger 2nd > suspend. > > > > However, this still cannot fix the problems like "Surface 3". > > We'll still need a new usage model for such platforms (no open event). > > No, for surface 3 you simply need to add polling of "_LID" method to the > button driver. > > What are the other devices that mess up lid handling? [Lv Zheng] The patch lists 3 of them. Which are known to me because they all occurred after I started to look at the lid issues. According to my teammates. They've been fixing such wrong "DSDT" using customization for years. For example, by adding _LID(); Notify(lid) into _WAK() or EC._REG(). 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
Hi, > From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com] > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > method lid device restrictions > > On Fri, Jul 22, 2016 at 6:37 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > Hi Lv, > > > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > >> Hi, Dmitry > >> > >> > >> Thanks for the review. > >> > >> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > >> > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI > control > >> > method lid device restrictions > >> > > >> > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > >> > > This patch adds documentation for the usage model of the control > >> > method lid > >> > > device. > >> > > > >> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > >> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > >> > > Cc: Bastien Nocera: <hadess@hadess.net> > >> > > Cc: linux-input@vger.kernel.org > >> > > --- > >> > > Documentation/acpi/acpi-lid.txt | 89 > >> > +++++++++++++++++++++++++++++++++++++++ > >> > > 1 file changed, 89 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..2addedc > >> > > --- /dev/null > >> > > +++ b/Documentation/acpi/acpi-lid.txt > >> > > @@ -0,0 +1,89 @@ > >> > > +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 > >> > > >> > Can this be fixed in the next ACPI revision? > >> [Lv Zheng] > >> Even this is fixed in the ACPI specification, there are platforms already > doing this. > >> Especially platforms from Microsoft. > >> So the de-facto standard OS won't care about the change. > >> And we can still see such platforms. > >> > >> Here is an example from Surface 3: > >> > >> DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) > >> { > >> Scope (_SB) > >> { > >> Device (PCI0) > >> { > >> Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > >> Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > >> Device (SPI1) > >> { > >> Name (_HID, "8086228E") // _HID: Hardware ID > >> Device (NTRG) > >> { > >> Name (_HID, "MSHW0037") // _HID: Hardware ID > >> } > >> } > >> } > >> > >> Device (LID) > >> { > >> Name (_HID, EisaId ("PNP0C0D")) > >> Name (LIDB, Zero) > > > > Start with lid closed? In any case might be wrong. > > Actually the initial value doesn't matter if the gpiochip triggers the > _EC4 at boot, which it should > (https://github.com/hadess/fedora-surface3- > kernel/commit/13200f81662c1c0b58137947c3e6c000fe62a2ba, > still unsubmitted) > > > > >> Method (_LID, 0, NotSerialized) > >> { > >> Return (LIDB) > > > > So "_LID" returns the last state read by "_EC4". "_EC4" is > > edge-triggered and will be evaluated every time gpio changes state. > > That's assuming the change happens while the system is on. If you go > into suspend by closing the LID. Open it while on suspend and then hit > the power button given that the system doesn't wake up when the lid is > opened, the edge change was made while the system is asleep, and we > are screwed (from an ACPI point of view). See my next comment for a > solution. > [Lv Zheng] I actually not sure if polling can fix all issues. For example. If a platform reporting "close" after resuming. Then polling _LID will always return "close". And the userspace can still get the "close" not "open". So it seems polling is not working for such platforms (cached value, initial close). Surface 3 is not the only platform caching an initial close value. There are 2 traditional platforms listed in the patch description. > > > >> } > >> } > >> > >> Device (GPO0) > >> { > >> Name (_HID, "INT33FF") // _HID: Hardware ID > >> OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > >> Field (GPOR, ByteAcc, NoLock, Preserve) > >> { > >> Connection ( > >> GpioIo (Shared, PullNone, 0x0000, 0x0000, > IoRestrictionNone, > >> "\\_SB.GPO0", 0x00, ResourceConsumer, , > >> ) > >> { // Pin list > >> 0x004C > >> } > >> ), > >> HELD, 1 > > > > Is it possible to read state of this GPIO from userspace on startup to > > correct the initial state? > > > >> } > >> Method (_E4C, 0, Serialized) > >> { > >> If (LEqual(HELD, One)) > >> { > >> Store(One, ^^LID.LIDB) > >> > >> There is no "open" event generated by "Surface 3". > > > > Right so we update the state correctly, we just forgot to send the > > notification. Nothing that polling can't fix. > > Actually, I have a better (though more hackish) way of avoiding polling: > https://github.com/hadess/fedora-surface3- > kernel/blob/5e5775b9bdc308d665064387e0b144ee48e7b243/0002-WIP- > add-custom-surface3-platform-device-for-controll.patch > > Given that the notification is forwarded to the touchscreen anyway, we > can unregister the generic (and buggy) acpi button driver for the LID > and create our own based on this specific DSDT. > We can also make sure the LID state is also correct because of the WMI > method which allows to read the actual value of the GPIO connected to > the cover without using the cached (and most of the time wrong) acpi > LID.LIDB value. > > I still yet have to submit this, but with this patch, but we can > consider the Surface 3 as working and not an issue anymore. > [Lv Zheng] That could make surface 3 dependent on WMI driver, not ACPI button driver. Will this affect other buttons? For example, power button/sleep button. Our approach is to make ACPI button driver working. Though this may lead to ABI changes. > > > >> > >> } > >> Else > >> { > >> Store(Zero, ^^LID.LIDB) > >> Notify (LID, 0x80) > >> > >> There is only "close" event generated by "Surface 3". > >> Thus they are not paired. > >> > >> } > >> Notify (^^PCI0.SPI1.NTRG, One) // Device Check > >> } > >> } > >> } > >> } > >> > >> > > >> > > +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". Thus the "opened" notification is not > guaranteed. > >> > > + > >> > > +But it is guaranteed that the AML tables always notify "closed" > when > >> > the > >> > > +lid state is changed to "closed". The "closed" notification is > normally > >> > > +used to trigger some system power saving operations on Windows. > >> > Since it is > >> > > +fully tested, the "closed" notification is reliable for all AML tables. > >> > > + > >> > > +3. Expections for the userspace users of the ACPI lid device driver > >> > > + > >> > > +The ACPI button driver exports the lid state to the userspace via > the > >> > > +following file: > >> > > + /proc/acpi/button/lid/LID0/state > >> > > +This file actually calls the _LID control method described above. > And > >> > given > >> > > +the previous explanation, it is not reliable enough on some > platforms. > >> > So > >> > > +it is advised for the userspace program to not to solely rely on this > file > >> > > +to determine the actual lid state. > >> > > + > >> > > +The ACPI button driver emits 2 kinds of events to the user space: > >> > > + SW_LID > >> > > + When the lid state/event is reliable, the userspace can behave > >> > > + according to this input switch event. > >> > > + This is a mode prepared for backward compatiblity. > >> > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > >> > > + When the lid state/event is not reliable, the userspace should > behave > >> > > + according to these 2 input key events. > >> > > + New userspace programs may only be prepared for the input key > >> > events. > >> > > >> > No, absolutely not. If some x86 vendors managed to mess up their > >> > firmware implementations that does not mean that everyone now > has to > >> > abandon working perfectly well for them SW_LID events and rush to > >> > switch > >> > to a brand new event. > >> [Lv Zheng] > >> However there is no clear wording in the ACPI specification asking the > vendors to achieve paired lid events. > >> > >> > > >> > Apparently were are a few issues, main is that some systems not > reporting > >> > "open" event. This can be dealt with by userspace "writing" to the > >> > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and > >> > startup) > >> > for selected systems. This will mean that if system wakes up not > because > >> > LID is open we'll incorrectly assume that it is, but we can either add > >> > more smarts to the process emitting SW_LID event or simply say > "well, > >> > tough, the hardware is crappy" and bug vendor to see if they can fix > the > >> > issue (if not for current firmware them for next). > >> [Lv Zheng] > >> The problem is there is no vendor actually caring about fixing this > "issue". > >> Because Windows works well with their firmware. > >> Then finally becomes a big table customization business for our team. > > > > Well, OK. But you do not expect that we will redo up and down the stack > > lid handling just because MS messed up DSDT on Surface 3? No, let them > > know (they now care about Linux, right?) so Surface 4 works and quirk > > the behavior for Surface 3. > > > > From what I understood, it was more than just the Surface 3. Other > laptops were having issues and Lv's team gave up on fixing those > machines. > > >> > >> > > >> > As an additional workaround, we can toggle the LID switch off and on > >> > when we get notification, much like your proposed patch does for the > key > >> > events. > > I really don't like this approach. The problem being that we will fix > the notifications to user space, but nothing will tell userspace that > the LID state is known to be wrong. > OTOH, I already agreed for a hwdb in userspace so I guess this point is > moot. > > Having both events (one SW for reliable HW, always correct, and one > KEY for unreliable HW) allows userspace to make a clear distinction > between the working and non working events and they can continue to > keep using the polling of the SW node without extra addition. > [Lv Zheng] I think this solution is good and fair for all of the vendors. :-) > Anyway, if the kernel doesn't want to (or can't) fix the actual issue > (by making sure the DSDT is reliable), userspace needs to be changed > so any solution will be acceptable. [Lv Zheng] I think the answer is "can't". If we introduced too many workarounds into acpi button driver, in order to make something working while the platform firmware doesn't expect it to be working, then we'll start to worry about breaking good laptops. > > >> [Lv Zheng] > >> I think this is doable, I'll refresh my patchset to address your this > comment. > >> By inserting open/close events when next close/open event arrives > after a certain period, > >> this may fix some issues for the old programs. > >> Where user may be required to open/close lid twice to trigger 2nd > suspend. > >> > >> However, this still cannot fix the problems like "Surface 3". > >> We'll still need a new usage model for such platforms (no open event). > > > > No, for surface 3 you simply need to add polling of "_LID" method to the > > button driver. > > > > What are the other devices that mess up lid handling? > > > > I also would be interested in knowing how much issues you are facing > compared to the average number of "good" laptops. IIRC, you talked > about 3 (counting the Surface 3), but I believe you had more in mind. [Lv Zheng] Yes. However they happened before I started to look at the lid issues. I think Rui has several such experiences. +Rui. Thanks and best regards -Lv
On Fri, Jul 22, 2016 at 10:47 AM, Zheng, Lv <lv.zheng@intel.com> wrote: > Hi, > >> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com] >> Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control >> method lid device restrictions >> >> On Fri, Jul 22, 2016 at 6:37 AM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > Hi Lv, >> > >> > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: >> >> Hi, Dmitry >> >> >> >> >> >> Thanks for the review. >> >> >> >> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] >> >> > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI >> control >> >> > method lid device restrictions >> >> > >> >> > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: >> >> > > This patch adds documentation for the usage model of the control >> >> > method lid >> >> > > device. >> >> > > >> >> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> >> >> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> >> >> > > Cc: Bastien Nocera: <hadess@hadess.net> >> >> > > Cc: linux-input@vger.kernel.org >> >> > > --- >> >> > > Documentation/acpi/acpi-lid.txt | 89 >> >> > +++++++++++++++++++++++++++++++++++++++ >> >> > > 1 file changed, 89 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..2addedc >> >> > > --- /dev/null >> >> > > +++ b/Documentation/acpi/acpi-lid.txt >> >> > > @@ -0,0 +1,89 @@ >> >> > > +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 >> >> > >> >> > Can this be fixed in the next ACPI revision? >> >> [Lv Zheng] >> >> Even this is fixed in the ACPI specification, there are platforms already >> doing this. >> >> Especially platforms from Microsoft. >> >> So the de-facto standard OS won't care about the change. >> >> And we can still see such platforms. >> >> >> >> Here is an example from Surface 3: >> >> >> >> DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) >> >> { >> >> Scope (_SB) >> >> { >> >> Device (PCI0) >> >> { >> >> Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID >> >> Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID >> >> Device (SPI1) >> >> { >> >> Name (_HID, "8086228E") // _HID: Hardware ID >> >> Device (NTRG) >> >> { >> >> Name (_HID, "MSHW0037") // _HID: Hardware ID >> >> } >> >> } >> >> } >> >> >> >> Device (LID) >> >> { >> >> Name (_HID, EisaId ("PNP0C0D")) >> >> Name (LIDB, Zero) >> > >> > Start with lid closed? In any case might be wrong. >> >> Actually the initial value doesn't matter if the gpiochip triggers the >> _EC4 at boot, which it should >> (https://github.com/hadess/fedora-surface3- >> kernel/commit/13200f81662c1c0b58137947c3e6c000fe62a2ba, >> still unsubmitted) >> >> > >> >> Method (_LID, 0, NotSerialized) >> >> { >> >> Return (LIDB) >> > >> > So "_LID" returns the last state read by "_EC4". "_EC4" is >> > edge-triggered and will be evaluated every time gpio changes state. >> >> That's assuming the change happens while the system is on. If you go >> into suspend by closing the LID. Open it while on suspend and then hit >> the power button given that the system doesn't wake up when the lid is >> opened, the edge change was made while the system is asleep, and we >> are screwed (from an ACPI point of view). See my next comment for a >> solution. >> > [Lv Zheng] > I actually not sure if polling can fix all issues. > For example. > If a platform reporting "close" after resuming. > Then polling _LID will always return "close". > And the userspace can still get the "close" not "open". > So it seems polling is not working for such platforms (cached value, initial close). > Surface 3 is not the only platform caching an initial close value. > There are 2 traditional platforms listed in the patch description. > >> > >> >> } >> >> } >> >> >> >> Device (GPO0) >> >> { >> >> Name (_HID, "INT33FF") // _HID: Hardware ID >> >> OperationRegion (GPOR, GeneralPurposeIo, Zero, One) >> >> Field (GPOR, ByteAcc, NoLock, Preserve) >> >> { >> >> Connection ( >> >> GpioIo (Shared, PullNone, 0x0000, 0x0000, >> IoRestrictionNone, >> >> "\\_SB.GPO0", 0x00, ResourceConsumer, , >> >> ) >> >> { // Pin list >> >> 0x004C >> >> } >> >> ), >> >> HELD, 1 >> > >> > Is it possible to read state of this GPIO from userspace on startup to >> > correct the initial state? >> > >> >> } >> >> Method (_E4C, 0, Serialized) >> >> { >> >> If (LEqual(HELD, One)) >> >> { >> >> Store(One, ^^LID.LIDB) >> >> >> >> There is no "open" event generated by "Surface 3". >> > >> > Right so we update the state correctly, we just forgot to send the >> > notification. Nothing that polling can't fix. >> >> Actually, I have a better (though more hackish) way of avoiding polling: >> https://github.com/hadess/fedora-surface3- >> kernel/blob/5e5775b9bdc308d665064387e0b144ee48e7b243/0002-WIP- >> add-custom-surface3-platform-device-for-controll.patch >> >> Given that the notification is forwarded to the touchscreen anyway, we >> can unregister the generic (and buggy) acpi button driver for the LID >> and create our own based on this specific DSDT. >> We can also make sure the LID state is also correct because of the WMI >> method which allows to read the actual value of the GPIO connected to >> the cover without using the cached (and most of the time wrong) acpi >> LID.LIDB value. >> >> I still yet have to submit this, but with this patch, but we can >> consider the Surface 3 as working and not an issue anymore. >> > [Lv Zheng] > That could make surface 3 dependent on WMI driver, not ACPI button driver. > Will this affect other buttons? > For example, power button/sleep button. TLDR: no, there is no impact on other buttons. There are 2 reasons why the impact is limited: - the patch only removes the input node that contains the LID, and it is the only one event in the input node - power/sleep, volume +/- are not handled by ACPI as this is a reduced platform and these buttons are not notified by ACPI. So we need an adaptation of the GPIO button array for it to be working (patch already submitted but I found a non-acpi platform issue, and then not enough time to fix and send an updated version). > > Our approach is to make ACPI button driver working. > Though this may lead to ABI changes. Yes, I know you want to fix ACPI button for future non working tablets/laptops. This is why I gave my rev-by in this series. > >> > >> >> >> >> } >> >> Else >> >> { >> >> Store(Zero, ^^LID.LIDB) >> >> Notify (LID, 0x80) >> >> >> >> There is only "close" event generated by "Surface 3". >> >> Thus they are not paired. >> >> >> >> } >> >> Notify (^^PCI0.SPI1.NTRG, One) // Device Check >> >> } >> >> } >> >> } >> >> } >> >> >> >> > >> >> > > +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". Thus the "opened" notification is not >> guaranteed. >> >> > > + >> >> > > +But it is guaranteed that the AML tables always notify "closed" >> when >> >> > the >> >> > > +lid state is changed to "closed". The "closed" notification is >> normally >> >> > > +used to trigger some system power saving operations on Windows. >> >> > Since it is >> >> > > +fully tested, the "closed" notification is reliable for all AML tables. >> >> > > + >> >> > > +3. Expections for the userspace users of the ACPI lid device driver >> >> > > + >> >> > > +The ACPI button driver exports the lid state to the userspace via >> the >> >> > > +following file: >> >> > > + /proc/acpi/button/lid/LID0/state >> >> > > +This file actually calls the _LID control method described above. >> And >> >> > given >> >> > > +the previous explanation, it is not reliable enough on some >> platforms. >> >> > So >> >> > > +it is advised for the userspace program to not to solely rely on this >> file >> >> > > +to determine the actual lid state. >> >> > > + >> >> > > +The ACPI button driver emits 2 kinds of events to the user space: >> >> > > + SW_LID >> >> > > + When the lid state/event is reliable, the userspace can behave >> >> > > + according to this input switch event. >> >> > > + This is a mode prepared for backward compatiblity. >> >> > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE >> >> > > + When the lid state/event is not reliable, the userspace should >> behave >> >> > > + according to these 2 input key events. >> >> > > + New userspace programs may only be prepared for the input key >> >> > events. >> >> > >> >> > No, absolutely not. If some x86 vendors managed to mess up their >> >> > firmware implementations that does not mean that everyone now >> has to >> >> > abandon working perfectly well for them SW_LID events and rush to >> >> > switch >> >> > to a brand new event. >> >> [Lv Zheng] >> >> However there is no clear wording in the ACPI specification asking the >> vendors to achieve paired lid events. >> >> >> >> > >> >> > Apparently were are a few issues, main is that some systems not >> reporting >> >> > "open" event. This can be dealt with by userspace "writing" to the >> >> > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and >> >> > startup) >> >> > for selected systems. This will mean that if system wakes up not >> because >> >> > LID is open we'll incorrectly assume that it is, but we can either add >> >> > more smarts to the process emitting SW_LID event or simply say >> "well, >> >> > tough, the hardware is crappy" and bug vendor to see if they can fix >> the >> >> > issue (if not for current firmware them for next). >> >> [Lv Zheng] >> >> The problem is there is no vendor actually caring about fixing this >> "issue". >> >> Because Windows works well with their firmware. >> >> Then finally becomes a big table customization business for our team. >> > >> > Well, OK. But you do not expect that we will redo up and down the stack >> > lid handling just because MS messed up DSDT on Surface 3? No, let them >> > know (they now care about Linux, right?) so Surface 4 works and quirk >> > the behavior for Surface 3. >> > >> >> From what I understood, it was more than just the Surface 3. Other >> laptops were having issues and Lv's team gave up on fixing those >> machines. >> >> >> >> >> > >> >> > As an additional workaround, we can toggle the LID switch off and on >> >> > when we get notification, much like your proposed patch does for the >> key >> >> > events. >> >> I really don't like this approach. The problem being that we will fix >> the notifications to user space, but nothing will tell userspace that >> the LID state is known to be wrong. >> OTOH, I already agreed for a hwdb in userspace so I guess this point is >> moot. >> >> Having both events (one SW for reliable HW, always correct, and one >> KEY for unreliable HW) allows userspace to make a clear distinction >> between the working and non working events and they can continue to >> keep using the polling of the SW node without extra addition. >> > [Lv Zheng] > I think this solution is good and fair for all of the vendors. :-) > >> Anyway, if the kernel doesn't want to (or can't) fix the actual issue >> (by making sure the DSDT is reliable), userspace needs to be changed >> so any solution will be acceptable. > [Lv Zheng] > I think the answer is "can't". > If we introduced too many workarounds into acpi button driver, > in order to make something working while the platform firmware doesn't expect it to be working, > then we'll start to worry about breaking good laptops. Then you just need to amend the documentation to say that the fallback of the KEY events is not the "future" but a way to get events on some reduced platforms and it will not be the default. Please make sure userspace knows that the default is the good SW_LID, and some particular cases will need to be handled through the KEY events, not the other way around. [few thoughts later] How about: - you send only one patch with the SW_LID ON/OFF or OFF/ON when we receive the notification on buggy platform - in the same patch, you add the documentation saying that on most platforms, LID is reliable but some don't provide a reliable LID state, but you guarantee to send an event when the state changes - in userspace, we add the hwdb which says "on this particular platform, don't rely on the actual state, but wait for events" -> this basically removes the polling on these platforms. Bastien, Dmitry? I still don't like relying on userspace to actually set the SW_LID back to open on resume, as we should not rely on some userspace program to set the value (but if logind really wants it, it's up to them). Cheers, Benjamin > >> >> >> [Lv Zheng] >> >> I think this is doable, I'll refresh my patchset to address your this >> comment. >> >> By inserting open/close events when next close/open event arrives >> after a certain period, >> >> this may fix some issues for the old programs. >> >> Where user may be required to open/close lid twice to trigger 2nd >> suspend. >> >> >> >> However, this still cannot fix the problems like "Surface 3". >> >> We'll still need a new usage model for such platforms (no open event). >> > >> > No, for surface 3 you simply need to add polling of "_LID" method to the >> > button driver. >> > >> > What are the other devices that mess up lid handling? >> > >> >> I also would be interested in knowing how much issues you are facing >> compared to the average number of "good" laptops. IIRC, you talked >> about 3 (counting the Surface 3), but I believe you had more in mind. > > [Lv Zheng] > Yes. > However they happened before I started to look at the lid issues. > I think Rui has several such experiences. > +Rui. > > 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
On Fri, Jul 22, 2016 at 08:55:00AM +0200, Benjamin Tissoires wrote: > On Fri, Jul 22, 2016 at 6:37 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > Hi Lv, > > > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > >> Hi, Dmitry > >> > >> > >> Thanks for the review. > >> > >> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > >> > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > >> > method lid device restrictions > >> > > >> > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > >> > > This patch adds documentation for the usage model of the control > >> > method lid > >> > > device. > >> > > > >> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > >> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > >> > > Cc: Bastien Nocera: <hadess@hadess.net> > >> > > Cc: linux-input@vger.kernel.org > >> > > --- > >> > > Documentation/acpi/acpi-lid.txt | 89 > >> > +++++++++++++++++++++++++++++++++++++++ > >> > > 1 file changed, 89 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..2addedc > >> > > --- /dev/null > >> > > +++ b/Documentation/acpi/acpi-lid.txt > >> > > @@ -0,0 +1,89 @@ > >> > > +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 > >> > > >> > Can this be fixed in the next ACPI revision? > >> [Lv Zheng] > >> Even this is fixed in the ACPI specification, there are platforms already doing this. > >> Especially platforms from Microsoft. > >> So the de-facto standard OS won't care about the change. > >> And we can still see such platforms. > >> > >> Here is an example from Surface 3: > >> > >> DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) > >> { > >> Scope (_SB) > >> { > >> Device (PCI0) > >> { > >> Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > >> Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > >> Device (SPI1) > >> { > >> Name (_HID, "8086228E") // _HID: Hardware ID > >> Device (NTRG) > >> { > >> Name (_HID, "MSHW0037") // _HID: Hardware ID > >> } > >> } > >> } > >> > >> Device (LID) > >> { > >> Name (_HID, EisaId ("PNP0C0D")) > >> Name (LIDB, Zero) > > > > Start with lid closed? In any case might be wrong. > > Actually the initial value doesn't matter if the gpiochip triggers the > _EC4 at boot, which it should > (https://github.com/hadess/fedora-surface3-kernel/commit/13200f81662c1c0b58137947c3e6c000fe62a2ba, > still unsubmitted) > > > > >> Method (_LID, 0, NotSerialized) > >> { > >> Return (LIDB) > > > > So "_LID" returns the last state read by "_EC4". "_EC4" is > > edge-triggered and will be evaluated every time gpio changes state. > > That's assuming the change happens while the system is on. If you go > into suspend by closing the LID. Open it while on suspend and then hit > the power button given that the system doesn't wake up when the lid is > opened, the edge change was made while the system is asleep, and we > are screwed (from an ACPI point of view). See my next comment for a > solution. Can we extend the patch you referenced above and do similar thing on resume? Won't help Surface but might others. > > > > >> } > >> } > >> > >> Device (GPO0) > >> { > >> Name (_HID, "INT33FF") // _HID: Hardware ID > >> OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > >> Field (GPOR, ByteAcc, NoLock, Preserve) > >> { > >> Connection ( > >> GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone, > >> "\\_SB.GPO0", 0x00, ResourceConsumer, , > >> ) > >> { // Pin list > >> 0x004C > >> } > >> ), > >> HELD, 1 > > > > Is it possible to read state of this GPIO from userspace on startup to > > correct the initial state? > > > >> } > >> Method (_E4C, 0, Serialized) > >> { > >> If (LEqual(HELD, One)) > >> { > >> Store(One, ^^LID.LIDB) > >> > >> There is no "open" event generated by "Surface 3". > > > > Right so we update the state correctly, we just forgot to send the > > notification. Nothing that polling can't fix. > > Actually, I have a better (though more hackish) way of avoiding polling: > https://github.com/hadess/fedora-surface3-kernel/blob/5e5775b9bdc308d665064387e0b144ee48e7b243/0002-WIP-add-custom-surface3-platform-device-for-controll.patch > > Given that the notification is forwarded to the touchscreen anyway, we > can unregister the generic (and buggy) acpi button driver for the LID > and create our own based on this specific DSDT. > We can also make sure the LID state is also correct because of the WMI > method which allows to read the actual value of the GPIO connected to > the cover without using the cached (and most of the time wrong) acpi > LID.LIDB value. > > I still yet have to submit this, but with this patch, but we can > consider the Surface 3 as working and not an issue anymore. > > > > >> > >> } > >> Else > >> { > >> Store(Zero, ^^LID.LIDB) > >> Notify (LID, 0x80) > >> > >> There is only "close" event generated by "Surface 3". > >> Thus they are not paired. > >> > >> } > >> Notify (^^PCI0.SPI1.NTRG, One) // Device Check > >> } > >> } > >> } > >> } > >> > >> > > >> > > +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". Thus the "opened" notification is not guaranteed. > >> > > + > >> > > +But it is guaranteed that the AML tables always notify "closed" when > >> > the > >> > > +lid state is changed to "closed". The "closed" notification is normally > >> > > +used to trigger some system power saving operations on Windows. > >> > Since it is > >> > > +fully tested, the "closed" notification is reliable for all AML tables. > >> > > + > >> > > +3. Expections for the userspace users of the ACPI lid device driver > >> > > + > >> > > +The ACPI button driver exports the lid state to the userspace via the > >> > > +following file: > >> > > + /proc/acpi/button/lid/LID0/state > >> > > +This file actually calls the _LID control method described above. And > >> > given > >> > > +the previous explanation, it is not reliable enough on some platforms. > >> > So > >> > > +it is advised for the userspace program to not to solely rely on this file > >> > > +to determine the actual lid state. > >> > > + > >> > > +The ACPI button driver emits 2 kinds of events to the user space: > >> > > + SW_LID > >> > > + When the lid state/event is reliable, the userspace can behave > >> > > + according to this input switch event. > >> > > + This is a mode prepared for backward compatiblity. > >> > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > >> > > + When the lid state/event is not reliable, the userspace should behave > >> > > + according to these 2 input key events. > >> > > + New userspace programs may only be prepared for the input key > >> > events. > >> > > >> > No, absolutely not. If some x86 vendors managed to mess up their > >> > firmware implementations that does not mean that everyone now has to > >> > abandon working perfectly well for them SW_LID events and rush to > >> > switch > >> > to a brand new event. > >> [Lv Zheng] > >> However there is no clear wording in the ACPI specification asking the vendors to achieve paired lid events. > >> > >> > > >> > Apparently were are a few issues, main is that some systems not reporting > >> > "open" event. This can be dealt with by userspace "writing" to the > >> > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and > >> > startup) > >> > for selected systems. This will mean that if system wakes up not because > >> > LID is open we'll incorrectly assume that it is, but we can either add > >> > more smarts to the process emitting SW_LID event or simply say "well, > >> > tough, the hardware is crappy" and bug vendor to see if they can fix the > >> > issue (if not for current firmware them for next). > >> [Lv Zheng] > >> The problem is there is no vendor actually caring about fixing this "issue". > >> Because Windows works well with their firmware. > >> Then finally becomes a big table customization business for our team. > > > > Well, OK. But you do not expect that we will redo up and down the stack > > lid handling just because MS messed up DSDT on Surface 3? No, let them > > know (they now care about Linux, right?) so Surface 4 works and quirk > > the behavior for Surface 3. > > > > From what I understood, it was more than just the Surface 3. Other > laptops were having issues and Lv's team gave up on fixing those > machines. > > >> > >> > > >> > As an additional workaround, we can toggle the LID switch off and on > >> > when we get notification, much like your proposed patch does for the key > >> > events. > > I really don't like this approach. The problem being that we will fix > the notifications to user space, but nothing will tell userspace that > the LID state is known to be wrong. > OTOH, I already agreed for a hwdb in userspace so I guess this point is moot. Yeah, I do not like this too much either, I would prefer if we could dig out and communicate real state of LID to userspace. It sounds we have reasonable way for the Surfaces (I assume the others will have the same issues as 3), and we need to figure out what the other troublemakers are. > > Having both events (one SW for reliable HW, always correct, and one > KEY for unreliable HW) allows userspace to make a clear distinction > between the working and non working events and they can continue to > keep using the polling of the SW node without extra addition. At this point I would prefer not adding any "unreliable" events just yet and concentrate on finding working solution for SW_LID. Thanks.
On Fri, Jul 22, 2016 at 08:37:50AM +0000, Zheng, Lv wrote: > Hi, Dmitry > > Thanks for the review. > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > > method lid device restrictions > > > > Hi Lv, > > > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > > > Hi, Dmitry > > > > > > > > > Thanks for the review. > > > > > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI > > control > > > > method lid device restrictions > > > > > > > > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > > > > > This patch adds documentation for the usage model of the control > > > > method lid > > > > > device. > > > > > > > > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > > > > Cc: Bastien Nocera: <hadess@hadess.net> > > > > > Cc: linux-input@vger.kernel.org > > > > > --- > > > > > Documentation/acpi/acpi-lid.txt | 89 > > > > +++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 89 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..2addedc > > > > > --- /dev/null > > > > > +++ b/Documentation/acpi/acpi-lid.txt > > > > > @@ -0,0 +1,89 @@ > > > > > +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 > > > > > > > > Can this be fixed in the next ACPI revision? > > > [Lv Zheng] > > > Even this is fixed in the ACPI specification, there are platforms already > > doing this. > > > Especially platforms from Microsoft. > > > So the de-facto standard OS won't care about the change. > > > And we can still see such platforms. > > > > > > Here is an example from Surface 3: > > > > > > DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", 0x01072009) > > > { > > > Scope (_SB) > > > { > > > Device (PCI0) > > > { > > > Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > > > Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > > > Device (SPI1) > > > { > > > Name (_HID, "8086228E") // _HID: Hardware ID > > > Device (NTRG) > > > { > > > Name (_HID, "MSHW0037") // _HID: Hardware ID > > > } > > > } > > > } > > > > > > Device (LID) > > > { > > > Name (_HID, EisaId ("PNP0C0D")) > > > Name (LIDB, Zero) > > > > Start with lid closed? In any case might be wrong. > [Lv Zheng] > And we validated with qemu that during boot, Windows7 evaluates _LID once but doesn't get suspended because of this value. > So we think Windows only suspends against "notification" not _LID evaluation result. > > > > > > Method (_LID, 0, NotSerialized) > > > { > > > Return (LIDB) > > > > So "_LID" returns the last state read by "_EC4". "_EC4" is > > edge-triggered and will be evaluated every time gpio changes state. > [Lv Zheng] > Right. > > > > > > } > > > } > > > > > > Device (GPO0) > > > { > > > Name (_HID, "INT33FF") // _HID: Hardware ID > > > OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > > > Field (GPOR, ByteAcc, NoLock, Preserve) > > > { > > > Connection ( > > > GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone, > > > "\\_SB.GPO0", 0x00, ResourceConsumer, , > > > ) > > > { // Pin list > > > 0x004C > > > } > > > ), > > > HELD, 1 > > > > Is it possible to read state of this GPIO from userspace on startup to > > correct the initial state? > [Lv Zheng] > I think Benjamin has a proposal of fixing this in GPIO driver. > > > > > > } > > > Method (_E4C, 0, Serialized) > > > { > > > If (LEqual(HELD, One)) > > > { > > > Store(One, ^^LID.LIDB) > > > > > > There is no "open" event generated by "Surface 3". > > > > Right so we update the state correctly, we just forgot to send the > > notification. Nothing that polling can't fix. > > > [Lv Zheng] > However, polling is not efficient, and not power efficient. We would not need to do this by default, and polling on a relaxed schedule (so that wakeups for polling coincide with other wakeups) should not be too bad (as in fractions of percent of power spent). > OTOH, according to the validation result, Windows never poll _LID. > > > > > > > } > > > Else > > > { > > > Store(Zero, ^^LID.LIDB) > > > Notify (LID, 0x80) > > > > > > There is only "close" event generated by "Surface 3". > > > Thus they are not paired. > > > > > > } > > > Notify (^^PCI0.SPI1.NTRG, One) // Device Check > > > } > > > } > > > } > > > } > > > > > > > > > > > > +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". Thus the "opened" notification is not > > guaranteed. > > > > > + > > > > > +But it is guaranteed that the AML tables always notify "closed" > > when > > > > the > > > > > +lid state is changed to "closed". The "closed" notification is normally > > > > > +used to trigger some system power saving operations on Windows. > > > > Since it is > > > > > +fully tested, the "closed" notification is reliable for all AML tables. > > > > > + > > > > > +3. Expections for the userspace users of the ACPI lid device driver > > > > > + > > > > > +The ACPI button driver exports the lid state to the userspace via the > > > > > +following file: > > > > > + /proc/acpi/button/lid/LID0/state > > > > > +This file actually calls the _LID control method described above. And > > > > given > > > > > +the previous explanation, it is not reliable enough on some > > platforms. > > > > So > > > > > +it is advised for the userspace program to not to solely rely on this > > file > > > > > +to determine the actual lid state. > > > > > + > > > > > +The ACPI button driver emits 2 kinds of events to the user space: > > > > > + SW_LID > > > > > + When the lid state/event is reliable, the userspace can behave > > > > > + according to this input switch event. > > > > > + This is a mode prepared for backward compatiblity. > > > > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > > > > > + When the lid state/event is not reliable, the userspace should > > behave > > > > > + according to these 2 input key events. > > > > > + New userspace programs may only be prepared for the input key > > > > events. > > > > > > > > No, absolutely not. If some x86 vendors managed to mess up their > > > > firmware implementations that does not mean that everyone now has > > to > > > > abandon working perfectly well for them SW_LID events and rush to > > > > switch > > > > to a brand new event. > > > [Lv Zheng] > > > However there is no clear wording in the ACPI specification asking the > > vendors to achieve paired lid events. > > > > > > > > > > > Apparently were are a few issues, main is that some systems not > > reporting > > > > "open" event. This can be dealt with by userspace "writing" to the > > > > lid's evdev device EV_SW/SW_LID/0 event upon system resume (and > > > > startup) > > > > for selected systems. This will mean that if system wakes up not > > because > > > > LID is open we'll incorrectly assume that it is, but we can either add > > > > more smarts to the process emitting SW_LID event or simply say "well, > > > > tough, the hardware is crappy" and bug vendor to see if they can fix > > the > > > > issue (if not for current firmware them for next). > > > [Lv Zheng] > > > The problem is there is no vendor actually caring about fixing this "issue". > > > Because Windows works well with their firmware. > > > Then finally becomes a big table customization business for our team. > > > > Well, OK. But you do not expect that we will redo up and down the stack > > lid handling just because MS messed up DSDT on Surface 3? No, let them > > know (they now care about Linux, right?) so Surface 4 works and quirk > > the behavior for Surface 3. > [Lv Zheng] > I think there are other platforms broken. Probably. I think we should deal with them as they come. > > > > > > > > > > > > > > As an additional workaround, we can toggle the LID switch off and on > > > > when we get notification, much like your proposed patch does for the > > key > > > > events. > > > [Lv Zheng] > > > I think this is doable, I'll refresh my patchset to address your this > > comment. > > > By inserting open/close events when next close/open event arrives after > > a certain period, > > > this may fix some issues for the old programs. > > > Where user may be required to open/close lid twice to trigger 2nd > > suspend. > > > > > > However, this still cannot fix the problems like "Surface 3". > > > We'll still need a new usage model for such platforms (no open event). > > > > No, for surface 3 you simply need to add polling of "_LID" method to the > > button driver. > > > > What are the other devices that mess up lid handling? > [Lv Zheng] > The patch lists 3 of them. > Which are known to me because they all occurred after I started to look at the lid issues. > > According to my teammates. > They've been fixing such wrong "DSDT" using customization for years. > For example, by adding _LID(); Notify(lid) into _WAK() or EC._REG(). What did they do with them once they did the fix? Were they submitting fixes to manufacturers? What happened to them? Thanks.
Hi, Dmitry > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- > owner@vger.kernel.org] On Behalf Of Dmitry Torokhov > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > method lid device restrictions > > On Fri, Jul 22, 2016 at 08:37:50AM +0000, Zheng, Lv wrote: > > Hi, Dmitry > > > > Thanks for the review. > > > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI > control > > > method lid device restrictions > > > > > > Hi Lv, > > > > > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > > > > Hi, Dmitry > > > > > > > > > > > > Thanks for the review. > > > > > > > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > > > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI > > > control > > > > > method lid device restrictions > > > > > > > > > > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > > > > > > This patch adds documentation for the usage model of the > control > > > > > method lid > > > > > > device. > > > > > > > > > > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > > > > > Cc: Bastien Nocera: <hadess@hadess.net> > > > > > > Cc: linux-input@vger.kernel.org > > > > > > --- > > > > > > Documentation/acpi/acpi-lid.txt | 89 > > > > > +++++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 89 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..2addedc > > > > > > --- /dev/null > > > > > > +++ b/Documentation/acpi/acpi-lid.txt > > > > > > @@ -0,0 +1,89 @@ > > > > > > +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 > > > > > > > > > > Can this be fixed in the next ACPI revision? > > > > [Lv Zheng] > > > > Even this is fixed in the ACPI specification, there are platforms > already > > > doing this. > > > > Especially platforms from Microsoft. > > > > So the de-facto standard OS won't care about the change. > > > > And we can still see such platforms. > > > > > > > > Here is an example from Surface 3: > > > > > > > > DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", > 0x01072009) > > > > { > > > > Scope (_SB) > > > > { > > > > Device (PCI0) > > > > { > > > > Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > > > > Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > > > > Device (SPI1) > > > > { > > > > Name (_HID, "8086228E") // _HID: Hardware ID > > > > Device (NTRG) > > > > { > > > > Name (_HID, "MSHW0037") // _HID: Hardware ID > > > > } > > > > } > > > > } > > > > > > > > Device (LID) > > > > { > > > > Name (_HID, EisaId ("PNP0C0D")) > > > > Name (LIDB, Zero) > > > > > > Start with lid closed? In any case might be wrong. > > [Lv Zheng] > > And we validated with qemu that during boot, Windows7 evaluates _LID > once but doesn't get suspended because of this value. > > So we think Windows only suspends against "notification" not _LID > evaluation result. > > > > > > > > > Method (_LID, 0, NotSerialized) > > > > { > > > > Return (LIDB) > > > > > > So "_LID" returns the last state read by "_EC4". "_EC4" is > > > edge-triggered and will be evaluated every time gpio changes state. > > [Lv Zheng] > > Right. > > > > > > > > > } > > > > } > > > > > > > > Device (GPO0) > > > > { > > > > Name (_HID, "INT33FF") // _HID: Hardware ID > > > > OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > > > > Field (GPOR, ByteAcc, NoLock, Preserve) > > > > { > > > > Connection ( > > > > GpioIo (Shared, PullNone, 0x0000, 0x0000, > IoRestrictionNone, > > > > "\\_SB.GPO0", 0x00, ResourceConsumer, , > > > > ) > > > > { // Pin list > > > > 0x004C > > > > } > > > > ), > > > > HELD, 1 > > > > > > Is it possible to read state of this GPIO from userspace on startup to > > > correct the initial state? > > [Lv Zheng] > > I think Benjamin has a proposal of fixing this in GPIO driver. > > > > > > > > > } > > > > Method (_E4C, 0, Serialized) > > > > { > > > > If (LEqual(HELD, One)) > > > > { > > > > Store(One, ^^LID.LIDB) > > > > > > > > There is no "open" event generated by "Surface 3". > > > > > > Right so we update the state correctly, we just forgot to send the > > > notification. Nothing that polling can't fix. > > > > > [Lv Zheng] > > However, polling is not efficient, and not power efficient. > > We would not need to do this by default, and polling on a relaxed > schedule (so that wakeups for polling coincide with other wakeups) > should not be too bad (as in fractions of percent of power spent). [Lv Zheng] Yes, this feature is on my radar. Because: 1. There seem to be some gaps in Linux, Linux cannot make some platforms reporting LID notifications and userspace has to poll exported lid state to work it around. 2. Since we've decided that the LID driver should be responsible for sending SW_LID, we should implement the kernel polling quirk instead of the userspace quirk. However, this feature cannot fix the issue related to this patchset. When the platform reports cached "close" after resuming, it seems the polling can still result in "close" to be sent. And the userspace can still suspend the platform right after resume. Or if we start to use lid_init_state=open, we cannot achieve "dark resume" usage model. And after a long run, the polling code may still erroneously decide to send a "close" to the userspace. And the userspace can still suspend the platform erroneously. So we still need the userspace to change to be compliant to this documented new usage model (at least we need such a quirk mechanism). > > > OTOH, according to the validation result, Windows never poll _LID. > > > > > > > > > > } > > > > Else > > > > { > > > > Store(Zero, ^^LID.LIDB) > > > > Notify (LID, 0x80) > > > > > > > > There is only "close" event generated by "Surface 3". > > > > Thus they are not paired. > > > > > > > > } > > > > Notify (^^PCI0.SPI1.NTRG, One) // Device Check > > > > } > > > > } > > > > } > > > > } > > > > > > > > > > > > > > > +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". Thus the "opened" notification is not > > > guaranteed. > > > > > > + > > > > > > +But it is guaranteed that the AML tables always notify "closed" > > > when > > > > > the > > > > > > +lid state is changed to "closed". The "closed" notification is > normally > > > > > > +used to trigger some system power saving operations on > Windows. > > > > > Since it is > > > > > > +fully tested, the "closed" notification is reliable for all AML > tables. > > > > > > + > > > > > > +3. Expections for the userspace users of the ACPI lid device > driver > > > > > > + > > > > > > +The ACPI button driver exports the lid state to the userspace via > the > > > > > > +following file: > > > > > > + /proc/acpi/button/lid/LID0/state > > > > > > +This file actually calls the _LID control method described above. > And > > > > > given > > > > > > +the previous explanation, it is not reliable enough on some > > > platforms. > > > > > So > > > > > > +it is advised for the userspace program to not to solely rely on > this > > > file > > > > > > +to determine the actual lid state. > > > > > > + > > > > > > +The ACPI button driver emits 2 kinds of events to the user space: > > > > > > + SW_LID > > > > > > + When the lid state/event is reliable, the userspace can behave > > > > > > + according to this input switch event. > > > > > > + This is a mode prepared for backward compatiblity. > > > > > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > > > > > > + When the lid state/event is not reliable, the userspace should > > > behave > > > > > > + according to these 2 input key events. > > > > > > + New userspace programs may only be prepared for the input > key > > > > > events. > > > > > > > > > > No, absolutely not. If some x86 vendors managed to mess up their > > > > > firmware implementations that does not mean that everyone now > has > > > to > > > > > abandon working perfectly well for them SW_LID events and rush > to > > > > > switch > > > > > to a brand new event. > > > > [Lv Zheng] > > > > However there is no clear wording in the ACPI specification asking > the > > > vendors to achieve paired lid events. > > > > > > > > > > > > > > Apparently were are a few issues, main is that some systems not > > > reporting > > > > > "open" event. This can be dealt with by userspace "writing" to the > > > > > lid's evdev device EV_SW/SW_LID/0 event upon system resume > (and > > > > > startup) > > > > > for selected systems. This will mean that if system wakes up not > > > because > > > > > LID is open we'll incorrectly assume that it is, but we can either add > > > > > more smarts to the process emitting SW_LID event or simply say > "well, > > > > > tough, the hardware is crappy" and bug vendor to see if they can fix > > > the > > > > > issue (if not for current firmware them for next). > > > > [Lv Zheng] > > > > The problem is there is no vendor actually caring about fixing this > "issue". > > > > Because Windows works well with their firmware. > > > > Then finally becomes a big table customization business for our team. > > > > > > Well, OK. But you do not expect that we will redo up and down the > stack > > > lid handling just because MS messed up DSDT on Surface 3? No, let > them > > > know (they now care about Linux, right?) so Surface 4 works and quirk > > > the behavior for Surface 3. > > [Lv Zheng] > > I think there are other platforms broken. > > Probably. I think we should deal with them as they come. [Lv Zheng] This seems to be what the ACPI team has been doing for years. However, if the userspace has been changed to be compliant to the new documented usage model. I think we needn't do that for the users, users can just specify a userspace option to work it around themselves. > > > > > > > > > > > > > > > > > > > > As an additional workaround, we can toggle the LID switch off and > on > > > > > when we get notification, much like your proposed patch does for > the > > > key > > > > > events. > > > > [Lv Zheng] > > > > I think this is doable, I'll refresh my patchset to address your this > > > comment. > > > > By inserting open/close events when next close/open event arrives > after > > > a certain period, > > > > this may fix some issues for the old programs. > > > > Where user may be required to open/close lid twice to trigger 2nd > > > suspend. > > > > > > > > However, this still cannot fix the problems like "Surface 3". > > > > We'll still need a new usage model for such platforms (no open > event). > > > > > > No, for surface 3 you simply need to add polling of "_LID" method to > the > > > button driver. > > > > > > What are the other devices that mess up lid handling? > > [Lv Zheng] > > The patch lists 3 of them. > > Which are known to me because they all occurred after I started to look > at the lid issues. > > > > According to my teammates. > > They've been fixing such wrong "DSDT" using customization for years. > > For example, by adding _LID(); Notify(lid) into _WAK() or EC._REG(). > > What did they do with them once they did the fix? Were they submitting > fixes to manufacturers? What happened to them? [Lv Zheng] I really don't know. However, I think the ending of the story is likely: The users who report such breakage happily get their platforms working. And they'll never report it again. Users using the same platforms can find the quirk via web search. Or the best ending is: The reporters have reported this to the Linux contribution vendors. And the knowledge is documented in their published laptop knowledge base or included in the distribution. I think no manufacturer really cares about such fixes. But I'm really not the correct one to answer this question. I'm new to ACPI bug fixing work. IMO, if the userspace can have an option to implement a mode compliant to this documented usage model. And always automatically enable this option when PNP0C0D is detected. No one will need any kind of quirks. But as Benjamin suggested, we may use the hwdb to enable this option for those buggy platforms. So that all other platforms are compliant to the unified Linux lid model. 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
Hi, Dmitry > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi- > owner@vger.kernel.org] On Behalf Of Dmitry Torokhov > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > method lid device restrictions > > On Fri, Jul 22, 2016 at 08:55:00AM +0200, Benjamin Tissoires wrote: > > On Fri, Jul 22, 2016 at 6:37 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > Hi Lv, > > > > > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote: > > >> Hi, Dmitry > > >> > > >> > > >> Thanks for the review. > > >> > > >> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] > > >> > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI > control > > >> > method lid device restrictions > > >> > > > >> > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote: > > >> > > This patch adds documentation for the usage model of the control > > >> > method lid > > >> > > device. > > >> > > > > >> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > >> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > >> > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > >> > > Cc: Bastien Nocera: <hadess@hadess.net> > > >> > > Cc: linux-input@vger.kernel.org > > >> > > --- > > >> > > Documentation/acpi/acpi-lid.txt | 89 > > >> > +++++++++++++++++++++++++++++++++++++++ > > >> > > 1 file changed, 89 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..2addedc > > >> > > --- /dev/null > > >> > > +++ b/Documentation/acpi/acpi-lid.txt > > >> > > @@ -0,0 +1,89 @@ > > >> > > +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 > > >> > > > >> > Can this be fixed in the next ACPI revision? > > >> [Lv Zheng] > > >> Even this is fixed in the ACPI specification, there are platforms already > doing this. > > >> Especially platforms from Microsoft. > > >> So the de-facto standard OS won't care about the change. > > >> And we can still see such platforms. > > >> > > >> Here is an example from Surface 3: > > >> > > >> DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ", > 0x01072009) > > >> { > > >> Scope (_SB) > > >> { > > >> Device (PCI0) > > >> { > > >> Name (_HID, EisaId ("PNP0A08")) // _HID: Hardware ID > > >> Name (_CID, EisaId ("PNP0A03")) // _CID: Compatible ID > > >> Device (SPI1) > > >> { > > >> Name (_HID, "8086228E") // _HID: Hardware ID > > >> Device (NTRG) > > >> { > > >> Name (_HID, "MSHW0037") // _HID: Hardware ID > > >> } > > >> } > > >> } > > >> > > >> Device (LID) > > >> { > > >> Name (_HID, EisaId ("PNP0C0D")) > > >> Name (LIDB, Zero) > > > > > > Start with lid closed? In any case might be wrong. > > > > Actually the initial value doesn't matter if the gpiochip triggers the > > _EC4 at boot, which it should > > (https://github.com/hadess/fedora-surface3- > kernel/commit/13200f81662c1c0b58137947c3e6c000fe62a2ba, > > still unsubmitted) > > > > > > > >> Method (_LID, 0, NotSerialized) > > >> { > > >> Return (LIDB) > > > > > > So "_LID" returns the last state read by "_EC4". "_EC4" is > > > edge-triggered and will be evaluated every time gpio changes state. > > > > That's assuming the change happens while the system is on. If you go > > into suspend by closing the LID. Open it while on suspend and then hit > > the power button given that the system doesn't wake up when the lid is > > opened, the edge change was made while the system is asleep, and we > > are screwed (from an ACPI point of view). See my next comment for a > > solution. > > Can we extend the patch you referenced above and do similar thing on > resume? Won't help Surface but might others. > > > > > > > > >> } > > >> } > > >> > > >> Device (GPO0) > > >> { > > >> Name (_HID, "INT33FF") // _HID: Hardware ID > > >> OperationRegion (GPOR, GeneralPurposeIo, Zero, One) > > >> Field (GPOR, ByteAcc, NoLock, Preserve) > > >> { > > >> Connection ( > > >> GpioIo (Shared, PullNone, 0x0000, 0x0000, > IoRestrictionNone, > > >> "\\_SB.GPO0", 0x00, ResourceConsumer, , > > >> ) > > >> { // Pin list > > >> 0x004C > > >> } > > >> ), > > >> HELD, 1 > > > > > > Is it possible to read state of this GPIO from userspace on startup to > > > correct the initial state? > > > > > >> } > > >> Method (_E4C, 0, Serialized) > > >> { > > >> If (LEqual(HELD, One)) > > >> { > > >> Store(One, ^^LID.LIDB) > > >> > > >> There is no "open" event generated by "Surface 3". > > > > > > Right so we update the state correctly, we just forgot to send the > > > notification. Nothing that polling can't fix. > > > > Actually, I have a better (though more hackish) way of avoiding polling: > > https://github.com/hadess/fedora-surface3- > kernel/blob/5e5775b9bdc308d665064387e0b144ee48e7b243/0002-WIP- > add-custom-surface3-platform-device-for-controll.patch > > > > Given that the notification is forwarded to the touchscreen anyway, we > > can unregister the generic (and buggy) acpi button driver for the LID > > and create our own based on this specific DSDT. > > We can also make sure the LID state is also correct because of the WMI > > method which allows to read the actual value of the GPIO connected to > > the cover without using the cached (and most of the time wrong) acpi > > LID.LIDB value. > > > > I still yet have to submit this, but with this patch, but we can > > consider the Surface 3 as working and not an issue anymore. > > > > > > > >> > > >> } > > >> Else > > >> { > > >> Store(Zero, ^^LID.LIDB) > > >> Notify (LID, 0x80) > > >> > > >> There is only "close" event generated by "Surface 3". > > >> Thus they are not paired. > > >> > > >> } > > >> Notify (^^PCI0.SPI1.NTRG, One) // Device Check > > >> } > > >> } > > >> } > > >> } > > >> > > >> > > > >> > > +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". Thus the "opened" notification is not > guaranteed. > > >> > > + > > >> > > +But it is guaranteed that the AML tables always notify "closed" > when > > >> > the > > >> > > +lid state is changed to "closed". The "closed" notification is > normally > > >> > > +used to trigger some system power saving operations on > Windows. > > >> > Since it is > > >> > > +fully tested, the "closed" notification is reliable for all AML tables. > > >> > > + > > >> > > +3. Expections for the userspace users of the ACPI lid device driver > > >> > > + > > >> > > +The ACPI button driver exports the lid state to the userspace via > the > > >> > > +following file: > > >> > > + /proc/acpi/button/lid/LID0/state > > >> > > +This file actually calls the _LID control method described above. > And > > >> > given > > >> > > +the previous explanation, it is not reliable enough on some > platforms. > > >> > So > > >> > > +it is advised for the userspace program to not to solely rely on > this file > > >> > > +to determine the actual lid state. > > >> > > + > > >> > > +The ACPI button driver emits 2 kinds of events to the user space: > > >> > > + SW_LID > > >> > > + When the lid state/event is reliable, the userspace can behave > > >> > > + according to this input switch event. > > >> > > + This is a mode prepared for backward compatiblity. > > >> > > + KEY_EVENT_OPEN/KEY_EVENT_CLOSE > > >> > > + When the lid state/event is not reliable, the userspace should > behave > > >> > > + according to these 2 input key events. > > >> > > + New userspace programs may only be prepared for the input > key > > >> > events. > > >> > > > >> > No, absolutely not. If some x86 vendors managed to mess up their > > >> > firmware implementations that does not mean that everyone now > has to > > >> > abandon working perfectly well for them SW_LID events and rush to > > >> > switch > > >> > to a brand new event. > > >> [Lv Zheng] > > >> However there is no clear wording in the ACPI specification asking the > vendors to achieve paired lid events. > > >> > > >> > > > >> > Apparently were are a few issues, main is that some systems not > reporting > > >> > "open" event. This can be dealt with by userspace "writing" to the > > >> > lid's evdev device EV_SW/SW_LID/0 event upon system resume > (and > > >> > startup) > > >> > for selected systems. This will mean that if system wakes up not > because > > >> > LID is open we'll incorrectly assume that it is, but we can either add > > >> > more smarts to the process emitting SW_LID event or simply say > "well, > > >> > tough, the hardware is crappy" and bug vendor to see if they can fix > the > > >> > issue (if not for current firmware them for next). > > >> [Lv Zheng] > > >> The problem is there is no vendor actually caring about fixing this > "issue". > > >> Because Windows works well with their firmware. > > >> Then finally becomes a big table customization business for our team. > > > > > > Well, OK. But you do not expect that we will redo up and down the > stack > > > lid handling just because MS messed up DSDT on Surface 3? No, let > them > > > know (they now care about Linux, right?) so Surface 4 works and quirk > > > the behavior for Surface 3. > > > > > > > From what I understood, it was more than just the Surface 3. Other > > laptops were having issues and Lv's team gave up on fixing those > > machines. > > > > >> > > >> > > > >> > As an additional workaround, we can toggle the LID switch off and > on > > >> > when we get notification, much like your proposed patch does for > the key > > >> > events. > > > > I really don't like this approach. The problem being that we will fix > > the notifications to user space, but nothing will tell userspace that > > the LID state is known to be wrong. > > OTOH, I already agreed for a hwdb in userspace so I guess this point is > moot. > > Yeah, I do not like this too much either, I would prefer if we could dig > out and communicate real state of LID to userspace. It sounds we have > reasonable way for the Surfaces (I assume the others will have the same > issues as 3), and we need to figure out what the other troublemakers > are. [Lv Zheng] Sorry for interruption. Unlike other platforms, surface 3 is a hardware reduced platform. So this might be a special case. Then what about the others? On traditional x86 platforms, when lid is opened, firmware is responsible to resume the platform. And the OS is waken up via a waking vector setting through FACS table. Then if the firmware forgot to prepare an "open" event after resuming, it would likely be that there wouldn't be an "open" event sent by the platform. Even worse, if the platform firmware implements _LID method with a cached value, and the default value of it is "close". Evaluating _LID after resuming could always result in "close". However OS is lucky that, if it doesn't receive lid notification, it needn't evaluate _LID. So OS can be immune to such "wrong close after resuming". > > > > > Having both events (one SW for reliable HW, always correct, and one > > KEY for unreliable HW) allows userspace to make a clear distinction > > between the working and non working events and they can continue to > > keep using the polling of the SW node without extra addition. > > At this point I would prefer not adding any "unreliable" events just yet > and concentrate on finding working solution for SW_LID. > [Lv Zheng] TBH, I think no user cares about the state of the lid. It's visible. On Windows, there is even no such a tray icon indicating lid state. When the lid is open, users can confirm that via their eyes. If the lid is close, users cannot see the state icon through the lid cover. So the actual "digitized" state of the lid is meaningless to the users. It only means something to the BIOS validators. OTOH, on traditional platforms, lid open is handled by BIOS, OS doesn't care about it. There is a power option in Windows configuring lid close event. But there is no similar configurable option for lid open event on Windows. Based on these facts, I'm not sure what is the solution we are still trying to find. If it is not the solution recommended in this document: 1. Never be strict to lid open event. 2. Never be strict to lid state. 3. But always be strict to lid close event. Hope the information is useful for understanding the situation. 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
On Fri, 2016-07-22 at 11:08 +0200, Benjamin Tissoires wrote: > <snip> > Then you just need to amend the documentation to say that the > fallback > of the KEY events is not the "future" but a way to get events on some > reduced platforms and it will not be the default. > Please make sure userspace knows that the default is the good SW_LID, > and some particular cases will need to be handled through the KEY > events, not the other way around. > > [few thoughts later] > > How about: > - you send only one patch with the SW_LID ON/OFF or OFF/ON when we > receive the notification on buggy platform > - in the same patch, you add the documentation saying that on most > platforms, LID is reliable but some don't provide a reliable LID > state, but you guarantee to send an event when the state changes > - in userspace, we add the hwdb which says "on this particular > platform, don't rely on the actual state, but wait for events" -> > this > basically removes the polling on these platforms. > > Bastien, Dmitry? > > I still don't like relying on userspace to actually set the SW_LID > back to open on resume, as we should not rely on some userspace > program to set the value (but if logind really wants it, it's up to > them). From my point of view, I would only send the events that can actually be generated by the system, not any synthetic ones, because user-space would have no way to know that this was synthetic, and how accurate it would be. So we'd have a separate API, or a separate event for the "close to Windows behaviour" devices. We'd then use hwdb in udev to tag the machines that don't have a reliable LID status, in user-space, so we can have a quick turn around for those machines. That should hopefully give us a way to tag test systems, so we can test the new behaviour, though we'll certainly need to have some changes made in the stack. As Benjamin mentioned, it would be nice to have a list of devices that don't work today, because of this problem. Cheers -- 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
Hi, Bastien > From: Bastien Nocera [mailto:hadess@hadess.net] > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control > method lid device restrictions > > On Fri, 2016-07-22 at 11:08 +0200, Benjamin Tissoires wrote: > > > <snip> > > Then you just need to amend the documentation to say that the > > fallback > > of the KEY events is not the "future" but a way to get events on some > > reduced platforms and it will not be the default. > > Please make sure userspace knows that the default is the good SW_LID, > > and some particular cases will need to be handled through the KEY > > events, not the other way around. > > > > [few thoughts later] > > > > How about: > > - you send only one patch with the SW_LID ON/OFF or OFF/ON when we > > receive the notification on buggy platform > > - in the same patch, you add the documentation saying that on most > > platforms, LID is reliable but some don't provide a reliable LID > > state, but you guarantee to send an event when the state changes > > - in userspace, we add the hwdb which says "on this particular > > platform, don't rely on the actual state, but wait for events" -> > > this > > basically removes the polling on these platforms. > > > > Bastien, Dmitry? > > > > I still don't like relying on userspace to actually set the SW_LID > > back to open on resume, as we should not rely on some userspace > > program to set the value (but if logind really wants it, it's up to > > them). > > From my point of view, I would only send the events that can actually > be generated by the system, not any synthetic ones, because user-space > would have no way to know that this was synthetic, and how accurate it > would be. > > So we'd have a separate API, or a separate event for the "close to > Windows behaviour" devices. We'd then use hwdb in udev to tag the > machines that don't have a reliable LID status, in user-space, so we > can have a quick turn around for those machines. > > That should hopefully give us a way to tag test systems, so we can test > the new behaviour, though we'll certainly need to have some changes > made in the stack. [Lv Zheng] That's the original motivation of PATCH 02. However, the PATCH 01 is valid fix. Without it, running SW_LID on such buggy platforms could cause no event. For example, if a platform always reports close, and never reports open. Then after the first SW_LID(close), userspace could never see the follow-up SW_LID(close). Thus that fix is required. Then after upstreaming PATCH 01, we can see something redundant to KEY_LID_XXX approach. Since with PATCH 01, we managed to ensure that platform triggered event will always be delivered to the userspace. Since: 1. Open event is not reliable 2. Close event is reliable We finally can see that: 1. All platform triggered close event can be seen by the userspace as SW_LID(close). 2. On the buggy platforms, SW_LID(open) is meaningless. It then looks like the KEY_LID_XXX is redundant to the improved SW_LID now. As with the key event approach, we still cannot guarantee to send "open" when the state is changed to "opened". __Unless we start to fix the buggy firmware tables__. And what we want to do - delivering reliable "close" to userspace can also be achieved with the SW_LID improvement. Thus, finally, there's no difference between the new userspace behaviors: 1. SW_LID with reliable close: userspace matches hwdb and stops acting upon open 2. KEY_LID_xxx with reliable close: userspace matches hwdb and starts acting only upon KEY_LID_CLOSE So we just need you and Dmitry to reach an agreement here. And this doesn't look like a big conflict. IMO, since SW_LID(CLOSE) is reliable now, we needn't introduce the new KEY_LID_xxx events. That means we can leave the kernel input layer unchanged. And limits this issue to the ACPI subsystem and the userspace programs. What do you think? > > As Benjamin mentioned, it would be nice to have a list of devices that > don't work today, because of this problem. [Lv Zheng] We'll try to find that. Before working out the full list, you can use the above mentioned 3 platforms to test. Cheers
diff --git a/Documentation/acpi/acpi-lid.txt b/Documentation/acpi/acpi-lid.txt new file mode 100644 index 0000000..2addedc --- /dev/null +++ b/Documentation/acpi/acpi-lid.txt @@ -0,0 +1,89 @@ +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". Thus the "opened" notification is not guaranteed. + +But it is guaranteed that the AML tables always notify "closed" when the +lid state is changed to "closed". The "closed" notification is normally +used to trigger some system power saving operations on Windows. Since it is +fully tested, the "closed" notification is reliable for all AML tables. + +3. Expections for the userspace users of the ACPI lid device driver + +The ACPI button driver exports the lid state to the userspace via the +following file: + /proc/acpi/button/lid/LID0/state +This file actually calls the _LID control method described above. And given +the previous explanation, it is not reliable enough on some platforms. So +it is advised for the userspace program to not to solely rely on this file +to determine the actual lid state. + +The ACPI button driver emits 2 kinds of events to the user space: + SW_LID + When the lid state/event is reliable, the userspace can behave + according to this input switch event. + This is a mode prepared for backward compatiblity. + KEY_EVENT_OPEN/KEY_EVENT_CLOSE + When the lid state/event is not reliable, the userspace should behave + according to these 2 input key events. + New userspace programs may only be prepared for the input key events. + +If the userspace hasn't been prepared to handle the new input lid key +events, Linux users can use the following kernel parameters to handle the +possible issues triggered because of the unreliable lid state/event: +A. button.lid_init_state=method: + When this option is specified, the ACPI button driver reports the + initial lid state using the returning value of the _LID control method. + This option can be used to fix some platforms where the _LID control + method's returning value is reliable but the initial lid state + notification is missing. + This option is the default behavior during the period the userspace + isn't ready to handle the new usage model. +B. button.lid_init_state=open: + When this option is specified, the ACPI button driver always reports the + initial lid state as "opened". + This may fix some platforms where the returning value of the _LID + control method is not reliable and the initial lid state notification is + missing. + +If the userspace has been prepared to handle the new input lid key events, +Linux users should always use the following kernel parameter: +C. button.lid_init_state=ignore: + When this option is specified, the ACPI button driver never reports the + initial lid state. However, the platform may automatically report a + correct initial lid state and there is no "open" event missing. When + this is the case (everything is correctly implemented by the platform + firmware), the old input switch event based userspace can still work. + Otherwise, the userspace programs may only work based on the input key + events. + This option will be the default behavior after the userspace is ready to + handle the new usage model.
This patch adds documentation for the usage model of the control method lid device. Signed-off-by: Lv Zheng <lv.zheng@intel.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com> Cc: Bastien Nocera: <hadess@hadess.net> Cc: linux-input@vger.kernel.org --- Documentation/acpi/acpi-lid.txt | 89 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 Documentation/acpi/acpi-lid.txt