Hi, Rafael > From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxx> > > Suggested-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> > > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx> > > Cc: Bastien Nocera: <hadess@xxxxxxxxxx> > > Cc: linux-input@xxxxxxxxxxxxxxx > > --- > > 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
