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

Zheng, Lv
2016-07-22T00:25:01Z

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