Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671515 > unrolled thread

[RFC PATCH v6 0/5] ACPI: button: Fix button.lid_init_state=method mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH v6 0/5] ACPI: button: Fix button.lid_init_state=method mode Lv Zheng <lv.zheng@intel.com> - 2017-06-21 11:00 +0200
    [RFC PATCH v6 2/5] ACPI: button: Add an optional workaround to fix an event missing issue for old userspace Lv Zheng <lv.zheng@intel.com> - 2017-06-21 11:00 +0200
      Re: [RFC PATCH v6 2/5] ACPI: button: Add an optional workaround to  fix an event missing issue for old userspace Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-06-23 16:10 +0200

#1671515 — [RFC PATCH v6 0/5] ACPI: button: Fix button.lid_init_state=method mode

There are platform variations implementing ACPI lid device in different
way:
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.

This series tries to provide solutions to fix all cases for old userspace
programs. These solutions include:
1. Fix order issue in case 2,4. Enabled by default.
2. Fix event missing issue in case 3,4. As newer systemd doesn't require
   this fix, and this fix is not power friendly, it is not enabled by
   default, but can be enabled by:
   2.1. button.lid_periodic_update=1: periodically sends _LID value to
        the input layer.
3. Fix persistate close issue in case 5, as newer systemd doesn't require
   this fix, it is not enabled by default, but can be enabled in 2 means:
   3.1. button.lid_init_state=ignore: only adds complement open events.
   3.2. lid_unreliable=1: dymamically adds/removes input node.

Benjamin Tissoires (2):
  ACPI: button: extract input creation/destruction helpers
  ACPI: button: Add an optional workaround to fix a persistent close
    issue for old userspace

Lv Zheng (3):
  ACPI: button: Add a workaround to fix an order issue for old userspace
  ACPI: button: Add an optional workaround to fix an event missing issue
    for old userspace
  ACPI: button: Rework lid_init_state=ignore mode

 drivers/acpi/button.c | 302 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 186 insertions(+), 116 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1671516 — [RFC PATCH v6 2/5] ACPI: button: Add an optional workaround to fix an event missing issue for old userspace

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 fine with systemd 233, but only case 1,2 work
fine with systemd 229.

Case 3,4 can be treated as an event missing issue:
   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.

Note that not only systemd 229, desktop managers (ex.,
gnome-settings-daemon) also suffer from this issue.

This patch tries to fix this issue by periodically sending _LID return
value to userspace, which ensures to trigger a SW_LID event when the
underlying hardware state has changed. As adding a periodic timer is not a
power friendly way, this patch prepares an option for users to enable on
failure platforms for old userspace programs.

Users can configure update interval via button.lid_update_interval.
This should be configured to a smaller value than HoldoffTimeoutSec in
/etc/systemd/logind.conf.

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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 67a0d78..a8b119e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -126,6 +126,14 @@ 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");
 
+static bool lid_periodic_update __read_mostly = false;
+module_param(lid_periodic_update, bool, 0644);
+MODULE_PARM_DESC(lid_periodic_update, "Periodically sending lid state updates");
+
+static unsigned long lid_update_interval __read_mostly = 1 * MSEC_PER_SEC;
+module_param(lid_update_interval, ulong, 0644);
+MODULE_PARM_DESC(lid_update_interval, "Interval (ms) between lid periodic updates");
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -395,6 +403,8 @@ static void acpi_lid_initialize_state(struct acpi_device *device)
 		break;
 	case ACPI_BUTTON_LID_INIT_METHOD:
 		(void)acpi_lid_update_state(device);
+		if (lid_periodic_update)
+			acpi_lid_start_timer(device, lid_update_interval);
 		break;
 	case ACPI_BUTTON_LID_INIT_IGNORE:
 	default:
@@ -560,8 +570,11 @@ static int acpi_button_add(struct acpi_device *device)
 		 * more we only care about the last one...
 		 */
 		lid_device = device;
-		acpi_lid_start_timer(device,
-			lid_notify_timeout * MSEC_PER_SEC);
+		if (lid_periodic_update)
+			acpi_lid_initialize_state(device);
+		else
+			acpi_lid_start_timer(device,
+				lid_notify_timeout * MSEC_PER_SEC);
 	}
 
 	printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1673592 — Re: [RFC PATCH v6 2/5] ACPI: button: Add an optional workaround to fix an event missing issue for old userspace

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 fine with systemd 233, but only case 1,2 work
> fine with systemd 229.
> 
> Case 3,4 can be treated as an event missing issue:
>    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.
> 
> Note that not only systemd 229, desktop managers (ex.,
> gnome-settings-daemon) also suffer from this issue.
> 
> This patch tries to fix this issue by periodically sending _LID return
> value to userspace, which ensures to trigger a SW_LID event when the
> underlying hardware state has changed. As adding a periodic timer is not a
> power friendly way, this patch prepares an option for users to enable on
> failure platforms for old userspace programs.
> 
> Users can configure update interval via button.lid_update_interval.
> This should be configured to a smaller value than HoldoffTimeoutSec in
> /etc/systemd/logind.conf.
> 
> 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: what is the point if you just want to provide an event to user
space? We already explained to you that the events do not matter, only
the state is. If user space has an issue with a state not being in sync,
it's a user space bug and it has to be fixed in user space.

This patch could be useful if the purpose was to monitor the changes
while the state is unreliable (in case 4):
- if _LID returns a different value than right after suspend, it is
  expected to be something reliable, and so we could then re-create the
  input node and present it to user space
- if _LID still returns the same value after a somewhat long delay
  (10-20 seconds), then we can consider the value to be correct and
  re-create the input node.

Polling for polling and hoping user space will have more chances of
seeing an event for an EV_SW is moot.

Cheers,
Benjamin

>  drivers/acpi/button.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 67a0d78..a8b119e 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -126,6 +126,14 @@ 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");
>  
> +static bool lid_periodic_update __read_mostly = false;
> +module_param(lid_periodic_update, bool, 0644);
> +MODULE_PARM_DESC(lid_periodic_update, "Periodically sending lid state updates");
> +
> +static unsigned long lid_update_interval __read_mostly = 1 * MSEC_PER_SEC;
> +module_param(lid_update_interval, ulong, 0644);
> +MODULE_PARM_DESC(lid_update_interval, "Interval (ms) between lid periodic updates");
> +
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
>     -------------------------------------------------------------------------- */
> @@ -395,6 +403,8 @@ static void acpi_lid_initialize_state(struct acpi_device *device)
>  		break;
>  	case ACPI_BUTTON_LID_INIT_METHOD:
>  		(void)acpi_lid_update_state(device);
> +		if (lid_periodic_update)
> +			acpi_lid_start_timer(device, lid_update_interval);
>  		break;
>  	case ACPI_BUTTON_LID_INIT_IGNORE:
>  	default:
> @@ -560,8 +570,11 @@ static int acpi_button_add(struct acpi_device *device)
>  		 * more we only care about the last one...
>  		 */
>  		lid_device = device;
> -		acpi_lid_start_timer(device,
> -			lid_notify_timeout * MSEC_PER_SEC);
> +		if (lid_periodic_update)
> +			acpi_lid_initialize_state(device);
> +		else
> +			acpi_lid_start_timer(device,
> +				lid_notify_timeout * MSEC_PER_SEC);
>  	}
>  
>  	printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
> -- 
> 2.7.4
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web