RE: [PATCH v7 1/2] ACPI / button: Fix an issue that the platform triggered reliable events may not be delivered to the userspace
Hi, Rafael
> From: linux-acpi-ow...@vger.kernel.org
> [mailto:linux-acpi-ow...@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
> > Suggested-by: Dmitry Torokhov
> > Cc: Benjamin Tissoires
> > Cc: Bastien Nocera:
> > Cc: linux-in...@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
> > #include
> > #include
> > @@ -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_
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
> Suggested-by: Dmitry Torokhov
> Cc: Benjamin Tissoires
> Cc: Bastien Nocera:
> Cc: linux-in...@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
> #include
> #include
> @@ -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?
> 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 dev
