| Message ID | 45932b46ea84bfbab2120092f23aff2c522a3945.1498034513.git.lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Jun 21 2017 or thereabouts, Lv Zheng wrote: > There are platform variations implementing ACPI lid device in different > ways: > 1. Some platforms send "open" events to OS and the events arrive before > button driver is resumed; > 2. Some platforms send "open" events to OS, but the events arrive after > button driver is resumed, ex., Samsung N210+; > 3. Some platforms never send "open" events to OS, but send "open" events to > update the cached _LID return value, and the update events arrive before > button driver is resumed; > 4. Some platforms never send "open" events to OS, but send "open" events to > update the cached _LID return value, but the update events arrive after > button driver is resumed, ex., Surface Pro 3; > 5. Some platforms never send "open" events, _LID returns value sticks to > "close", ex., Surface Pro 1. > Currently, all cases work find with systemd 233, but only case 1 works fine > with systemd 229. > > Case 2,4 can be treated as an order issue: > If the button driver always evaluates _LID after seeing a LID > notification, there shouldn't be such a problem. > > Note that this order issue cannot be fixed by sorting OS drivers' resume > code: > 1. When acpi.ec_freeze_events=Y, the order depends on whether PNP0C0D(LID) > or PNP0C09(EC) appears first in the namespace (probe order). > 2. Even when acpi.ec_freeze_events=N, which forces OS to handle EC events > as early as possible, the order issue can still be reproduced due to > platform delays (reproduce ratio is lower than case 1). > 3. Some platform simply has a very big delay for LID open events. > > This patch tries to fix this issue for systemd 229 by defer sending initial > lid state 10 seconds later after resume, which ensures EC events can always > arrive before button driver evaluates _LID. It finally only fixes case 2 > platforms as fixing case 4 platforms needs additional efforts (see known > issue below). > > The users can configure wait timeout via button.lid_notify_timeout. > > Known issu: > 1. systemd/logind 229 still mis-bahaves with case 3,4 platforms > After seeing a LID "close" event, systemd 229 will wait several seconds > (HoldoffTimeoutSec) before suspending the platform. Thus on case 4 > platforms, if users close lid, and re-open it during the > HoldoffTimeoutSec period, there is still no "open" events seen by the > userspace. Thus systemd still considers the last state as "close" and > suspends the platform after the HoldoffTimeoutSec times out. > > Cc: <systemd-devel@lists.freedesktop.org> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: Peter Hutterer <peter.hutterer@who-t.net> > Signed-off-by: Lv Zheng <lv.zheng@intel.com> > --- NACK on this one (at least in this current form): You are presenting a device to user space with an unknown state. If you want to delay the query of the state, you also need to delay the call of input_register_device(). Cheers, Benjamin > drivers/acpi/button.c | 36 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index e19f530..67a0d78 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -28,6 +28,7 @@ > #include <linux/proc_fs.h> > #include <linux/seq_file.h> > #include <linux/input.h> > +#include <linux/timer.h> > #include <linux/slab.h> > #include <linux/acpi.h> > #include <acpi/button.h> > @@ -79,6 +80,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids); > static int acpi_button_add(struct acpi_device *device); > static int acpi_button_remove(struct acpi_device *device); > static void acpi_button_notify(struct acpi_device *device, u32 event); > +static void acpi_lid_timeout(ulong arg); > > #ifdef CONFIG_PM_SLEEP > static int acpi_button_suspend(struct device *dev); > @@ -104,6 +106,7 @@ static struct acpi_driver acpi_button_driver = { > struct acpi_button { > unsigned int type; > struct input_dev *input; > + struct timer_list lid_timer; > char phys[32]; /* for input device */ > unsigned long pushed; > int last_state; > @@ -119,6 +122,10 @@ 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"); > > +static unsigned long lid_notify_timeout __read_mostly = 10; > +module_param(lid_notify_timeout, ulong, 0644); > +MODULE_PARM_DESC(lid_notify_timeout, "Timeout (s) before receiving lid notification"); > + > /* -------------------------------------------------------------------------- > FS Interface (/proc) > -------------------------------------------------------------------------- */ > @@ -371,6 +378,15 @@ static int acpi_lid_update_state(struct acpi_device *device) > return acpi_lid_notify_state(device, state); > } > > +static void acpi_lid_start_timer(struct acpi_device *device, > + unsigned long msecs) > +{ > + struct acpi_button *button = acpi_driver_data(device); > + > + mod_timer(&button->lid_timer, > + jiffies + msecs_to_jiffies(msecs)); > +} > + > static void acpi_lid_initialize_state(struct acpi_device *device) > { > switch (lid_init_state) { > @@ -386,6 +402,13 @@ static void acpi_lid_initialize_state(struct acpi_device *device) > } > } > > +static void acpi_lid_timeout(ulong arg) > +{ > + struct acpi_device *device = (struct acpi_device *)arg; > + > + acpi_lid_initialize_state(device); > +} > + > static void acpi_button_notify(struct acpi_device *device, u32 event) > { > struct acpi_button *button = acpi_driver_data(device); > @@ -432,6 +455,8 @@ static int acpi_button_suspend(struct device *dev) > struct acpi_device *device = to_acpi_device(dev); > struct acpi_button *button = acpi_driver_data(device); > > + if (button->type == ACPI_BUTTON_TYPE_LID) > + del_timer(&button->lid_timer); > button->suspended = true; > return 0; > } > @@ -443,7 +468,8 @@ static int acpi_button_resume(struct device *dev) > > button->suspended = false; > if (button->type == ACPI_BUTTON_TYPE_LID) > - acpi_lid_initialize_state(device); > + acpi_lid_start_timer(device, > + lid_notify_timeout * MSEC_PER_SEC); > return 0; > } > #endif > @@ -490,6 +516,9 @@ static int acpi_button_add(struct acpi_device *device) > ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID); > button->last_state = !!acpi_lid_evaluate_state(device); > button->last_time = ktime_get(); > + init_timer(&button->lid_timer); > + setup_timer(&button->lid_timer, > + acpi_lid_timeout, (ulong)device); > } else { > printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid); > error = -ENODEV; > @@ -526,12 +555,13 @@ static int acpi_button_add(struct acpi_device *device) > if (error) > goto err_remove_fs; > if (button->type == ACPI_BUTTON_TYPE_LID) { > - acpi_lid_initialize_state(device); > /* > * This assumes there's only one lid device, or if there are > * more we only care about the last one... > */ > lid_device = device; > + acpi_lid_start_timer(device, > + lid_notify_timeout * MSEC_PER_SEC); > } > > printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device)); > @@ -551,6 +581,8 @@ static int acpi_button_remove(struct acpi_device *device) > struct acpi_button *button = acpi_driver_data(device); > > acpi_button_remove_fs(device); > + if (button->type == ACPI_BUTTON_TYPE_LID) > + del_timer_sync(&button->lid_timer); > input_unregister_device(button->input); > kfree(button); > return 0; > -- > 2.7.4 > -- 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
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index e19f530..67a0d78 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -28,6 +28,7 @@ #include <linux/proc_fs.h> #include <linux/seq_file.h> #include <linux/input.h> +#include <linux/timer.h> #include <linux/slab.h> #include <linux/acpi.h> #include <acpi/button.h> @@ -79,6 +80,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids); static int acpi_button_add(struct acpi_device *device); static int acpi_button_remove(struct acpi_device *device); static void acpi_button_notify(struct acpi_device *device, u32 event); +static void acpi_lid_timeout(ulong arg); #ifdef CONFIG_PM_SLEEP static int acpi_button_suspend(struct device *dev); @@ -104,6 +106,7 @@ static struct acpi_driver acpi_button_driver = { struct acpi_button { unsigned int type; struct input_dev *input; + struct timer_list lid_timer; char phys[32]; /* for input device */ unsigned long pushed; int last_state; @@ -119,6 +122,10 @@ 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"); +static unsigned long lid_notify_timeout __read_mostly = 10; +module_param(lid_notify_timeout, ulong, 0644); +MODULE_PARM_DESC(lid_notify_timeout, "Timeout (s) before receiving lid notification"); + /* -------------------------------------------------------------------------- FS Interface (/proc) -------------------------------------------------------------------------- */ @@ -371,6 +378,15 @@ static int acpi_lid_update_state(struct acpi_device *device) return acpi_lid_notify_state(device, state); } +static void acpi_lid_start_timer(struct acpi_device *device, + unsigned long msecs) +{ + struct acpi_button *button = acpi_driver_data(device); + + mod_timer(&button->lid_timer, + jiffies + msecs_to_jiffies(msecs)); +} + static void acpi_lid_initialize_state(struct acpi_device *device) { switch (lid_init_state) { @@ -386,6 +402,13 @@ static void acpi_lid_initialize_state(struct acpi_device *device) } } +static void acpi_lid_timeout(ulong arg) +{ + struct acpi_device *device = (struct acpi_device *)arg; + + acpi_lid_initialize_state(device); +} + static void acpi_button_notify(struct acpi_device *device, u32 event) { struct acpi_button *button = acpi_driver_data(device); @@ -432,6 +455,8 @@ static int acpi_button_suspend(struct device *dev) struct acpi_device *device = to_acpi_device(dev); struct acpi_button *button = acpi_driver_data(device); + if (button->type == ACPI_BUTTON_TYPE_LID) + del_timer(&button->lid_timer); button->suspended = true; return 0; } @@ -443,7 +468,8 @@ static int acpi_button_resume(struct device *dev) button->suspended = false; if (button->type == ACPI_BUTTON_TYPE_LID) - acpi_lid_initialize_state(device); + acpi_lid_start_timer(device, + lid_notify_timeout * MSEC_PER_SEC); return 0; } #endif @@ -490,6 +516,9 @@ static int acpi_button_add(struct acpi_device *device) ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID); button->last_state = !!acpi_lid_evaluate_state(device); button->last_time = ktime_get(); + init_timer(&button->lid_timer); + setup_timer(&button->lid_timer, + acpi_lid_timeout, (ulong)device); } else { printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid); error = -ENODEV; @@ -526,12 +555,13 @@ static int acpi_button_add(struct acpi_device *device) if (error) goto err_remove_fs; if (button->type == ACPI_BUTTON_TYPE_LID) { - acpi_lid_initialize_state(device); /* * This assumes there's only one lid device, or if there are * more we only care about the last one... */ lid_device = device; + acpi_lid_start_timer(device, + lid_notify_timeout * MSEC_PER_SEC); } printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device)); @@ -551,6 +581,8 @@ static int acpi_button_remove(struct acpi_device *device) struct acpi_button *button = acpi_driver_data(device); acpi_button_remove_fs(device); + if (button->type == ACPI_BUTTON_TYPE_LID) + del_timer_sync(&button->lid_timer); input_unregister_device(button->input); kfree(button); return 0;
There are platform variations implementing ACPI lid device in different ways: 1. Some platforms send "open" events to OS and the events arrive before button driver is resumed; 2. Some platforms send "open" events to OS, but the events arrive after button driver is resumed, ex., Samsung N210+; 3. Some platforms never send "open" events to OS, but send "open" events to update the cached _LID return value, and the update events arrive before button driver is resumed; 4. Some platforms never send "open" events to OS, but send "open" events to update the cached _LID return value, but the update events arrive after button driver is resumed, ex., Surface Pro 3; 5. Some platforms never send "open" events, _LID returns value sticks to "close", ex., Surface Pro 1. Currently, all cases work find with systemd 233, but only case 1 works fine with systemd 229. Case 2,4 can be treated as an order issue: If the button driver always evaluates _LID after seeing a LID notification, there shouldn't be such a problem. Note that this order issue cannot be fixed by sorting OS drivers' resume code: 1. When acpi.ec_freeze_events=Y, the order depends on whether PNP0C0D(LID) or PNP0C09(EC) appears first in the namespace (probe order). 2. Even when acpi.ec_freeze_events=N, which forces OS to handle EC events as early as possible, the order issue can still be reproduced due to platform delays (reproduce ratio is lower than case 1). 3. Some platform simply has a very big delay for LID open events. This patch tries to fix this issue for systemd 229 by defer sending initial lid state 10 seconds later after resume, which ensures EC events can always arrive before button driver evaluates _LID. It finally only fixes case 2 platforms as fixing case 4 platforms needs additional efforts (see known issue below). The users can configure wait timeout via button.lid_notify_timeout. Known issu: 1. systemd/logind 229 still mis-bahaves with case 3,4 platforms After seeing a LID "close" event, systemd 229 will wait several seconds (HoldoffTimeoutSec) before suspending the platform. Thus on case 4 platforms, if users close lid, and re-open it during the HoldoffTimeoutSec period, there is still no "open" events seen by the userspace. Thus systemd still considers the last state as "close" and suspends the platform after the HoldoffTimeoutSec times out. Cc: <systemd-devel@lists.freedesktop.org> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: Peter Hutterer <peter.hutterer@who-t.net> Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/button.c | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)