Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754174AbdFWOC5 (ORCPT ); Fri, 23 Jun 2017 10:02:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39052 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751217AbdFWOCz (ORCPT ); Fri, 23 Jun 2017 10:02:55 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D4266C0587DB Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=benjamin.tissoires@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D4266C0587DB Date: Fri, 23 Jun 2017 16:02:48 +0200 From: Benjamin Tissoires To: Lv Zheng Cc: "Rafael J . Wysocki" , "Rafael J . Wysocki" , Len Brown , Lv Zheng , linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, systemd-devel@lists.freedesktop.org, Peter Hutterer Subject: Re: [RFC PATCH v6 1/5] ACPI: button: Add a workaround to fix an order issue for old userspace Message-ID: <20170623140248.GO26073@mail.corp.redhat.com> References: <2a779ae8c280c968b3237ac4a3d9580df7262a46.1493951798.git.lv.zheng@intel.com> <45932b46ea84bfbab2120092f23aff2c522a3945.1498034513.git.lv.zheng@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <45932b46ea84bfbab2120092f23aff2c522a3945.1498034513.git.lv.zheng@intel.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 23 Jun 2017 14:02:55 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7627 Lines: 190 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: > Cc: Benjamin Tissoires > Cc: Peter Hutterer > Signed-off-by: Lv Zheng > --- 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 > #include > #include > +#include > #include > #include > #include > @@ -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 >