From: "Zheng, Lv" <lv.zheng@intel.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
"Brown, Len" <len.brown@intel.com>, Lv Zheng <zetalog@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
Benjamin Tissoires <benjamin.tissoires@gmail.com>,
"Bastien Nocera:" <hadess@hadess.net>,
"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>
Subject: RE: [PATCH v7 1/2] ACPI / button: Fix an issue that the platform triggered reliable events may not be delivered to the userspace
Date: Wed, 17 Aug 2016 04:45:06 +0000 [thread overview]
Message-ID: <1AE640813FDE7649BE1B193DEA596E883BC0EBA8@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <1830278.Y8XYXFSbDK@vostro.rjw.lan>
Hi, Rafael
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v7 1/2] ACPI / button: Fix an issue that the platform triggered reliable events
> may not be delivered to the userspace
>
> On Tuesday, July 26, 2016 05:52:24 PM Lv Zheng wrote:
> > On most platforms, _LID returning value, lid open/close events are all
> > reliable, but there are exceptions. Some AML tables report wrong initial
> > lid state (Link 1), and some of them never report lid open state (Link 2).
> > The usage model on such buggy platforms is:
> > 1. The initial lid state returned from _LID is not reliable;
> > 2. The lid open event is not reliable;
> > 3. The lid close event is always reliable, used by the platform firmware to
> > trigger OSPM power saving operations.
> > This usage model is not compliant to the Linux SW_LID model as the Linux
> > userspace is very strict to the reliability of the open events.
> >
> > In order not to trigger issues on such buggy platforms, the ACPI button
> > driver currently implements a lid_init_state=open quirk to send additional
> > "open" event after resuming. However, this is still not sufficient because:
> > 1. Some special usage models (e.x., the dark resume scenario) cannot be
> > supported by this mode.
> > 2. If a "close" event is not used to trigger "suspend", then the subsequent
> > "close" events cannot be seen by the userspace.
> > So we need to stop sending the additional "open" event and switch the
> > driver to lid_init_state=ignore mode and make sure the platform triggered
> > events can be reliably delivered to the userspace. The userspace programs
> > then can be changed to not to be strict to the "open" events on such buggy
> > platforms.
> >
> > Why will the subsequent "close" events be lost? This is because the input
> > layer automatically filters redundant events for switch events. Thus given
> > that the buggy AML tables do not guarantee paired "open"/"close" events,
> > the ACPI button driver currently is not able to guarantee that the platform
> > triggered reliable events can be always be seen by the userspace via
> > SW_LID.
> >
> > This patch adds a mechanism to insert lid events as a compensation for the
> > platform triggered ones to form a complete event switches in order to make
> > sure that the platform triggered events can always be reliably delivered
> > to the userspace. This essentially guarantees that the platform triggered
> > reliable "close" events will always be relibly delivered to the userspace.
> >
> > However this mechanism is not suitable for lid_init_state=open/method as
> > it should not send the complement switch event for the unreliable initial
> > lid state notification. 2 unreliable events can trigger unexpected
> > behavior. Thus this patch only implements this mechanism for
> > lid_init_state=ignore.
> >
> > Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=89211
> > https://bugzilla.kernel.org/show_bug.cgi?id=106151
> > Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=106941
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Suggested-by: 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
> > ---
> > drivers/acpi/button.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 148f4e5..dca1806 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -19,6 +19,8 @@
> > * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > */
> >
> > +#define pr_fmt(fmt) "ACPI : button: " fmt
> > +
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/init.h>
> > @@ -104,6 +106,8 @@ struct acpi_button {
> > struct input_dev *input;
> > char phys[32]; /* for input device */
> > unsigned long pushed;
> > + int last_state;
> > + unsigned long last_time;
>
> Why don't you use ktime_t here?
OK.
I'll update the patch with ktime interfaces.
And send it after tests.
Thanks,
Lv
>
> > bool suspended;
> > };
> >
> > @@ -111,6 +115,10 @@ static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> > static struct acpi_device *lid_device;
> > static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> >
> > +static unsigned long lid_report_interval __read_mostly = 500;
> > +module_param(lid_report_interval, ulong, 0644);
> > +MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
> > +
> > /* --------------------------------------------------------------------------
> > FS Interface (/proc)
> > -------------------------------------------------------------------------- */
> > @@ -135,9 +143,48 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
> > struct acpi_button *button = acpi_driver_data(device);
> > int ret;
> >
> > - /* input layer checks if event is redundant */
> > + if (button->last_state == !!state &&
> > + time_after(jiffies, button->last_time +
> > + msecs_to_jiffies(lid_report_interval))) {
>
> And ktime_after() here?
>
> > + /* Complain the buggy firmware */
> > + pr_warn_once("The lid device is not compliant to SW_LID.\n");
> > +
> > + /*
> > + * Send the unreliable complement switch event:
> > + *
> > + * On most platforms, the lid device is reliable. However
> > + * there are exceptions:
> > + * 1. Platforms returning initial lid state as "close" by
> > + * default after booting/resuming:
> > + * https://bugzilla.kernel.org/show_bug.cgi?id=89211
> > + * https://bugzilla.kernel.org/show_bug.cgi?id=106151
> > + * 2. Platforms never reporting "open" events:
> > + * https://bugzilla.kernel.org/show_bug.cgi?id=106941
> > + * On these buggy platforms, the usage model of the ACPI
> > + * lid device actually is:
> > + * 1. The initial returning value of _LID may not be
> > + * reliable.
> > + * 2. The open event may not be reliable.
> > + * 3. The close event is reliable.
> > + *
> > + * But SW_LID is typed as input switch event, the input
> > + * layer checks if the event is redundant. Hence if the
> > + * state is not switched, the userspace cannot see this
> > + * platform triggered reliable event. By inserting a
> > + * complement switch event, it then is guaranteed that the
> > + * platform triggered reliable one can always be seen by
> > + * the userspace.
> > + */
> > + if (lid_init_state == ACPI_BUTTON_LID_INIT_IGNORE) {
> > + input_report_switch(button->input, SW_LID, state);
> > + input_sync(button->input);
> > + }
> > + }
> > + /* Send the platform triggered reliable event */
> > input_report_switch(button->input, SW_LID, !state);
> > input_sync(button->input);
> > + button->last_state = !!state;
> > + button->last_time = jiffies;
>
> And ktime_get() here?
>
> >
> > if (state)
> > pm_wakeup_event(&device->dev, 0);
> > @@ -407,6 +454,8 @@ static int acpi_button_add(struct acpi_device *device)
> > strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
> > sprintf(class, "%s/%s",
> > ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
> > + button->last_state = !!acpi_lid_evaluate_state(device);
> > + button->last_time = jiffies;
>
> And here?
>
> > } else {
> > printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
> > error = -ENODEV;
> >
>
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2016-08-17 4:48 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-17 8:27 [RFC PATCH 1/2] ACPI / button: Send "open" state after boot/resume Lv Zheng
2016-05-17 23:36 ` Rafael J. Wysocki
2016-05-18 1:25 ` Zheng, Lv
2016-05-18 22:56 ` Rafael J. Wysocki
2016-05-19 1:50 ` Zheng, Lv
2016-05-19 13:21 ` Rafael J. Wysocki
2016-05-26 13:31 ` Benjamin Tissoires
2016-05-30 1:39 ` Zheng, Lv
2016-05-18 12:57 ` Bastien Nocera
2016-05-18 21:41 ` Rafael J. Wysocki
2016-05-19 1:59 ` Zheng, Lv
2016-05-27 7:15 ` [PATCH v2 0/3] ACPI / button: Clarify initial lid state Lv Zheng
2016-05-27 7:15 ` [PATCH v2 1/3] ACPI / button: Remove initial lid state notification Lv Zheng
2016-05-27 7:15 ` [PATCH v2 2/3] ACPI / button: Refactor functions to eliminate redundant code Lv Zheng
2016-05-27 7:16 ` [PATCH v2 3/3] ACPI / button: Send "open" state after boot/resume Lv Zheng
2016-05-30 8:10 ` Benjamin Tissoires
2016-05-31 2:55 ` Zheng, Lv
2016-05-31 14:47 ` Benjamin Tissoires
2016-06-01 1:17 ` Zheng, Lv
2016-06-01 7:51 ` Zheng, Lv
2016-06-01 8:07 ` Benjamin Tissoires
2016-05-27 22:10 ` [PATCH v2 0/3] ACPI / button: Clarify initial lid state Valdis.Kletnieks
2016-06-01 10:10 ` [PATCH v3 1/3] ACPI / button: Remove initial lid state notification Lv Zheng
2016-06-23 0:36 ` Rafael J. Wysocki
2016-06-23 0:57 ` Zheng, Lv
2016-06-01 10:10 ` [PATCH v3 2/3] ACPI / button: Refactor functions to eliminate redundant code Lv Zheng
2016-06-01 10:10 ` [PATCH v3 3/3] ACPI / button: Add quirks for initial lid state notification Lv Zheng
2016-06-01 11:01 ` Bastien Nocera
2016-06-02 1:08 ` Zheng, Lv
2016-06-02 14:01 ` Bastien Nocera
2016-06-02 15:25 ` Benjamin Tissoires
2016-06-03 0:41 ` Zheng, Lv
2016-07-19 8:11 ` [PATCH v4 1/2] ACPI / button: Add KEY_LID_OPEN/KEY_LID_CLOSE for new usage model Lv Zheng
2016-07-19 8:46 ` Benjamin Tissoires
2016-07-21 13:35 ` Rafael J. Wysocki
2016-07-21 20:33 ` Dmitry Torokhov
2016-07-19 8:11 ` [PATCH v4 2/2] ACPI / button: Add document for ACPI control method lid device restrictions Lv Zheng
2016-07-19 8:44 ` Benjamin Tissoires
2016-07-21 20:32 ` Dmitry Torokhov
2016-07-22 0:24 ` Zheng, Lv
2016-07-22 4:37 ` Dmitry Torokhov
2016-07-22 6:55 ` Benjamin Tissoires
2016-07-22 8:47 ` Zheng, Lv
2016-07-22 9:08 ` Benjamin Tissoires
2016-07-22 9:38 ` Zheng, Lv
2016-07-24 11:28 ` Bastien Nocera
2016-07-25 0:38 ` Zheng, Lv
2016-07-22 17:02 ` Dmitry Torokhov
2016-07-23 12:17 ` Zheng, Lv
2016-07-22 8:37 ` Zheng, Lv
2016-07-22 17:22 ` Dmitry Torokhov
2016-07-23 11:57 ` Zheng, Lv
2016-07-22 6:24 ` [PATCH v5 1/3] ACPI / button: Add missing event to keep SW_LID running without additional event loss Lv Zheng
2016-07-22 10:26 ` Zheng, Lv
2016-07-23 12:37 ` Rafael J. Wysocki
2016-07-25 0:24 ` Zheng, Lv
2016-07-22 6:24 ` [PATCH v5 2/3] ACPI / button: Add KEY_LID_OPEN/KEY_LID_CLOSE for new usage model Lv Zheng
2016-07-22 6:24 ` [PATCH v5 3/3] ACPI / button: Add document for ACPI control method lid device restrictions Lv Zheng
2016-07-25 1:14 ` [PATCH v6] ACPI / button: Fix an issue that the platform triggered "close" event may not be delivered to the userspace Lv Zheng
2016-07-26 9:52 ` [PATCH v7 1/2] ACPI / button: Fix an issue that the platform triggered reliable events " Lv Zheng
2016-08-17 0:19 ` Rafael J. Wysocki
2016-08-17 4:45 ` Zheng, Lv [this message]
2016-07-26 9:52 ` [PATCH v7 2/2] ACPI / button: Add document for ACPI control method lid device restrictions Lv Zheng
2016-08-17 8:22 ` [PATCH v8 1/2] ACPI / button: Fix an issue in button.lid_init_state=ignore mode Lv Zheng
2016-09-12 22:10 ` Rafael J. Wysocki
2016-08-17 8:23 ` [PATCH v8 2/2] ACPI / button: Add document for ACPI control method lid device restrictions Lv Zheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1AE640813FDE7649BE1B193DEA596E883BC0EBA8@SHSMSX101.ccr.corp.intel.com \
--to=lv.zheng@intel.com \
--cc=benjamin.tissoires@gmail.com \
--cc=hadess@hadess.net \
--cc=len.brown@intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=rjw@rjwysocki.net \
--cc=zetalog@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).