Message-ID: <1AE640813FDE7649BE1B193DEA596E883BC05AAA@SHSMSX101.ccr.corp.intel.com>
Date:	Fri, 22 Jul 2016 00:24:50 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Brown, Len" <len.brown@...el.com>, Lv Zheng <zetalog@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@...il.com>,
	"Bastien Nocera:" <hadess@...ess.net>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>
Subject: RE: [PATCH v4 2/2] ACPI / button: Add document for ACPI control
 method lid device restrictions

Hi, Dmitry


Thanks for the review.

> From: Dmitry Torokhov [mailto:dmitry.torokhov@...il.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@...el.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > Cc: Benjamin Tissoires <benjamin.tissoires@...il.com>
> > Cc: Bastien Nocera: <hadess@...ess.net>
> > Cc: linux-input@...r.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@...el.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

Powered by blists - more mailing lists