| Message ID | 0d28076e25047db356eeedbb2d8ae204f0ec893d.1469168549.git.lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Dmitry I'm considering what the future should be. What if we drop PATCH 02, stop introducing KEY_LID_XXX events. And switch ACPI button driver to lid_init_state=ignore. So that whatever the initial state is close or open, no events will be sent to the userspace. In the meanwhile, userspace tunes its behavior. With PATCH 01, 2nd close can arrive to userspace, so that can be tuned to: 1. User space stops asking kernel to send open notification. 2. And only acts against "close" notification. Thanks and best regards -Lv > From: Zheng, Lv > Subject: [PATCH v5 1/3] ACPI / button: Add missing event to keep SW_LID > running without additional event loss > > There are several possibilities that a lid event can be lost. For example, > EC event queue full, or the resume order of the underlying drivers. > > When the event loss happens, new event may also be lost due to the type > of > the SW_LID (switch event). The 2nd loss is what we want to avoid. > > This patch adds a mechanism to insert lid events as a compensation for > the > switch event nature of the lid events in order to avoid the 2nd loss. > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > Cc: 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 | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 148f4e5..41fd21d 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -104,6 +104,8 @@ struct acpi_button { > struct input_dev *input; > char phys[32]; /* for input device */ > unsigned long pushed; > + int sw_last_state; > + unsigned long sw_last_time; > bool suspended; > }; > > @@ -111,6 +113,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) > -------------------------------------------------------------------------- */ > @@ -133,11 +139,22 @@ static int acpi_lid_evaluate_state(struct > acpi_device *device) > static int acpi_lid_notify_state(struct acpi_device *device, int state) > { > struct acpi_button *button = acpi_driver_data(device); > + unsigned long sw_tout; > int ret; > > - /* input layer checks if event is redundant */ > + /* Send the switch event */ > + sw_tout = button->sw_last_time + > + msecs_to_jiffies(lid_report_interval); > + if (time_after(jiffies, sw_tout) && > + (button->sw_last_state == !!state)) { > + /* Send the complement switch event */ > + input_report_switch(button->input, SW_LID, state); > + input_sync(button->input); > + } > input_report_switch(button->input, SW_LID, !state); > input_sync(button->input); > + button->sw_last_state = !!state; > + button->sw_last_time = jiffies; > > if (state) > pm_wakeup_event(&device->dev, 0); > @@ -407,6 +424,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->sw_last_state = !!acpi_lid_evaluate_state(device); > + button->sw_last_time = jiffies; > } else { > printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid); > error = -ENODEV; > -- > 1.7.10 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, July 22, 2016 02:24:42 PM Lv Zheng wrote: > There are several possibilities that a lid event can be lost. For example, > EC event queue full, or the resume order of the underlying drivers. > > When the event loss happens, new event may also be lost due to the type of > the SW_LID (switch event). The 2nd loss is what we want to avoid. > > This patch adds a mechanism to insert lid events as a compensation for the > switch event nature of the lid events in order to avoid the 2nd loss. Can you please provide a high-level description of the new mechanism here? > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > Cc: 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 | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 148f4e5..41fd21d 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -104,6 +104,8 @@ struct acpi_button { > struct input_dev *input; > char phys[32]; /* for input device */ > unsigned long pushed; > + int sw_last_state; > + unsigned long sw_last_time; > bool suspended; > }; > > @@ -111,6 +113,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) > -------------------------------------------------------------------------- */ > @@ -133,11 +139,22 @@ static int acpi_lid_evaluate_state(struct acpi_device *device) > static int acpi_lid_notify_state(struct acpi_device *device, int state) > { > struct acpi_button *button = acpi_driver_data(device); > + unsigned long sw_tout; > int ret; > > - /* input layer checks if event is redundant */ > + /* Send the switch event */ > + sw_tout = button->sw_last_time + > + msecs_to_jiffies(lid_report_interval); Is it really necessary to use jiffies here? > + if (time_after(jiffies, sw_tout) && > + (button->sw_last_state == !!state)) { The inner parens are not necessary. And why not just button->sw_last_state == state? > + /* Send the complement switch event */ > + input_report_switch(button->input, SW_LID, state); > + input_sync(button->input); > + } > input_report_switch(button->input, SW_LID, !state); > input_sync(button->input); > + button->sw_last_state = !!state; > + button->sw_last_time = jiffies; > > if (state) > pm_wakeup_event(&device->dev, 0); > @@ -407,6 +424,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->sw_last_state = !!acpi_lid_evaluate_state(device); > + button->sw_last_time = jiffies; > } else { > printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid); > error = -ENODEV; > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Sent: Saturday, July 23, 2016 8:37 PM > Subject: Re: [PATCH v5 1/3] ACPI / button: Add missing event to keep > SW_LID running without additional event loss > > On Friday, July 22, 2016 02:24:42 PM Lv Zheng wrote: > > There are several possibilities that a lid event can be lost. For example, > > EC event queue full, or the resume order of the underlying drivers. > > > > When the event loss happens, new event may also be lost due to the > type of > > the SW_LID (switch event). The 2nd loss is what we want to avoid. > > > > This patch adds a mechanism to insert lid events as a compensation for > the > > switch event nature of the lid events in order to avoid the 2nd loss. > > Can you please provide a high-level description of the new mechanism > here? [Lv Zheng] OK. And IMO, this fix is a fix to the original ACPI button driver. It is not dependent on the input layer. Without the final agreement of the ABI change. We still can get this shipped in the upstream. If the ABI is determined to use the new KEY_LID_XX events, I'll send a series including 2 patches. If the ABI is determined to use the old SW_LID event, I'll send a series including only the documentation. Let me send this fix separately. Thanks and best regards -Lv > > > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > > Cc: 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 | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > > index 148f4e5..41fd21d 100644 > > --- a/drivers/acpi/button.c > > +++ b/drivers/acpi/button.c > > @@ -104,6 +104,8 @@ struct acpi_button { > > struct input_dev *input; > > char phys[32]; /* for input device */ > > unsigned long pushed; > > + int sw_last_state; > > + unsigned long sw_last_time; > > bool suspended; > > }; > > > > @@ -111,6 +113,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) > > -------------------------------------------------------------------------- */ > > @@ -133,11 +139,22 @@ static int acpi_lid_evaluate_state(struct > acpi_device *device) > > static int acpi_lid_notify_state(struct acpi_device *device, int state) > > { > > struct acpi_button *button = acpi_driver_data(device); > > + unsigned long sw_tout; > > int ret; > > > > - /* input layer checks if event is redundant */ > > + /* Send the switch event */ > > + sw_tout = button->sw_last_time + > > + msecs_to_jiffies(lid_report_interval); > > Is it really necessary to use jiffies here? > > > + if (time_after(jiffies, sw_tout) && > > + (button->sw_last_state == !!state)) { > > The inner parens are not necessary. > > And why not just button->sw_last_state == state? > > > + /* Send the complement switch event */ > > + input_report_switch(button->input, SW_LID, state); > > + input_sync(button->input); > > + } > > input_report_switch(button->input, SW_LID, !state); > > input_sync(button->input); > > + button->sw_last_state = !!state; > > + button->sw_last_time = jiffies; > > > > if (state) > > pm_wakeup_event(&device->dev, 0); > > @@ -407,6 +424,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->sw_last_state = !!acpi_lid_evaluate_state(device); > > + button->sw_last_time = jiffies; > > } else { > > printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid); > > error = -ENODEV; > > > > Thanks, > Rafael
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 148f4e5..41fd21d 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -104,6 +104,8 @@ struct acpi_button { struct input_dev *input; char phys[32]; /* for input device */ unsigned long pushed; + int sw_last_state; + unsigned long sw_last_time; bool suspended; }; @@ -111,6 +113,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) -------------------------------------------------------------------------- */ @@ -133,11 +139,22 @@ static int acpi_lid_evaluate_state(struct acpi_device *device) static int acpi_lid_notify_state(struct acpi_device *device, int state) { struct acpi_button *button = acpi_driver_data(device); + unsigned long sw_tout; int ret; - /* input layer checks if event is redundant */ + /* Send the switch event */ + sw_tout = button->sw_last_time + + msecs_to_jiffies(lid_report_interval); + if (time_after(jiffies, sw_tout) && + (button->sw_last_state == !!state)) { + /* Send the complement switch event */ + input_report_switch(button->input, SW_LID, state); + input_sync(button->input); + } input_report_switch(button->input, SW_LID, !state); input_sync(button->input); + button->sw_last_state = !!state; + button->sw_last_time = jiffies; if (state) pm_wakeup_event(&device->dev, 0); @@ -407,6 +424,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->sw_last_state = !!acpi_lid_evaluate_state(device); + button->sw_last_time = jiffies; } else { printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid); error = -ENODEV;
There are several possibilities that a lid event can be lost. For example, EC event queue full, or the resume order of the underlying drivers. When the event loss happens, new event may also be lost due to the type of the SW_LID (switch event). The 2nd loss is what we want to avoid. This patch adds a mechanism to insert lid events as a compensation for the switch event nature of the lid events in order to avoid the 2nd loss. Signed-off-by: Lv Zheng <lv.zheng@intel.com> Cc: 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 | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)