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? > 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
