Merge lp:~sforshee/powerd/fix-display-timing into lp:powerd

[Seth Forshee]

Synchronously wait on the framebuffer state to change when entering/exiting suspend with kernels using the earlysuspend implementation to avoid races which can lead to attempting to use the display device while it is suspended.

Synchronously wait on the framebuffer state to change when entering/exiting suspend with kernels using the earlysuspend implementation to avoid races which can lead to attempting to use the display device while it is suspended.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:106
http://jenkins.qa.ubuntu.com/job/powerd-ci/133/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/powerd-trusty-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/powerd-trusty-armhf-ci/3/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/powerd-ci/133/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

gave this a spin on the n4, looks like the issue is resolved.

review: Approve (testing)

[lp:~sforshee/powerd/fix-display-timing]

107. By Seth Forshee on 2014-01-06

libsuspend: Fix earlysuspend error checking on writes

earlysuspend_{enter,exit}() could wait on the framebuffer state
to change in some cases when the write to /sys/power/state
failed. Erroneously waiting on the framebuffer state to change
could potentially block mainloop execution indefinitely, so be
conservative about doing so. Only wait if exactly the expected
number of bytes is written.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:107
http://jenkins.qa.ubuntu.com/job/powerd-ci/134/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/powerd-trusty-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/powerd-trusty-armhf-ci/4/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/powerd-ci/134/rebuild

review: Approve (continuous-integration)

[Ricardo Salveti (rsalveti)]

Code looks good and works as expected.

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2013-12-06 22:11:52 +0000
+++ CMakeLists.txt	2014-01-06 16:20:16 +0000
@@ -17,6 +17,8 @@
 
 set(GDBUS_NAME powerd-dbus)
 
+find_package(Threads REQUIRED)
+
 find_package(PkgConfig)
 pkg_check_modules(ANDROID_HEADERS android-headers)
 pkg_check_modules(HYBRIS_IS libis)
@@ -107,6 +109,7 @@
   powerd
   suspend
 
+  ${CMAKE_THREAD_LIBS_INIT}
   ${HYBRIS_IS_LIBRARIES}
   ${HYBRIS_SF_LIBRARIES}
   ${GLIB_LIBRARIES}

=== modified file 'libsuspend/earlysuspend.c'
--- libsuspend/earlysuspend.c	2013-05-23 14:45:14 +0000
+++ libsuspend/earlysuspend.c	2014-01-06 16:20:16 +0000
@@ -18,9 +18,34 @@
 
 #include <stdio.h>
 #include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <pthread.h>
 #include "common.h"
 #include "sysfs.h"
 
+/* Use to suppress unused parameter warnings */
+#define __unused __attribute__ ((unused))
+
+enum fb_state {
+    FB_SLEEP,
+    FB_AWAKE,
+
+    NUM_FB_STATES
+};
+static enum fb_state fb_state = FB_AWAKE;
+
+static const char *fb_file_names[] = {
+    [FB_SLEEP] = "/sys/power/wait_for_fb_sleep",
+    [FB_AWAKE] = "/sys/power/wait_for_fb_wake"
+};
+
+static pthread_t fb_monitor_thread;
+static pthread_mutex_t fb_state_mutex = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t fb_state_cond = PTHREAD_COND_INITIALIZER;
+static int wait_for_fb = 0;
+
 static const char state_path[] = "/sys/power/state";
 static const char wakelock_path[] = "/sys/power/wake_lock";
 static const char wakeunlock_path[] = "/sys/power/wake_unlock";
@@ -29,15 +54,83 @@
 static const char mem_str[] = "mem";
 static const char on_str[] = "on";
 
+static int wait_for_file(const char *fname)
+{
+    int fd, ret;
+    char buf;
+
+    fd = open(fname, O_RDONLY);
+    if (fd == -1)
+        return -errno;
+
+    do {
+        ret = read(fd, &buf, 1);
+    } while (ret == -1 && errno == EINTR);
+
+    close(fd);
+    return ret == -1 ? -errno : 0;
+}
+
+static void *fb_monitor_thread_func(__unused void *unused)
+{
+    enum fb_state next_state;
+    int ret;
+
+    while (1) {
+        next_state = fb_state == FB_SLEEP ? FB_AWAKE : FB_SLEEP;
+        ret = wait_for_file(fb_file_names[next_state]);
+        if (ret)
+            continue;
+
+        pthread_mutex_lock(&fb_state_mutex);
+        fb_state = next_state;
+        pthread_cond_signal(&fb_state_cond);
+        pthread_mutex_unlock(&fb_state_mutex);
+    }
+
+    wait_for_fb = 0;
+    return NULL;
+}
+
+/* Returns 1 if fb monitor thread started, 0 otherwise */
+static int start_fb_monitor_thread(void)
+{
+    if (access(fb_file_names[FB_SLEEP], F_OK))
+        return 0;
+    if (access(fb_file_names[FB_AWAKE], F_OK))
+        return 0;
+
+    return !pthread_create(&fb_monitor_thread, NULL,
+                           fb_monitor_thread_func, NULL);
+}
+
 static int earlysuspend_enter(void)
 {
-    int ret = sysfs_write(state_path, mem_str, ARRAY_SIZE(mem_str) - 1);
+    int ret;
+    int len = ARRAY_SIZE(mem_str) - 1;
+   
+    ret = sysfs_write(state_path, mem_str, len);
+    if (ret == len && wait_for_fb) {
+        pthread_mutex_lock(&fb_state_mutex);
+        while (fb_state != FB_SLEEP)
+            pthread_cond_wait(&fb_state_cond, &fb_state_mutex);
+        pthread_mutex_unlock(&fb_state_mutex);
+    }
     return ret < 0 ? ret : 0;
 }
 
 static int earlysuspend_exit(void)
 {
-    int ret = sysfs_write(state_path, on_str, ARRAY_SIZE(on_str) - 1);
+    int ret;
+    int len = ARRAY_SIZE(on_str) - 1;
+   
+    ret = sysfs_write(state_path, on_str, len);
+    if (ret == len && wait_for_fb) {
+        pthread_mutex_lock(&fb_state_mutex);
+        while (fb_state != FB_AWAKE)
+            pthread_cond_wait(&fb_state_cond, &fb_state_mutex);
+        pthread_mutex_unlock(&fb_state_mutex);
+    }
     return ret < 0 ? ret : 0;
 }
 
@@ -64,8 +157,10 @@
 {
     if (!sysfs_file_exists(autosleep_path) &&
         sysfs_file_exists(wakelock_path) &&
-        sysfs_file_exists(state_path))
+        sysfs_file_exists(state_path)) {
+        wait_for_fb = start_fb_monitor_thread();
         return &earlysuspend_handler;
+    }
 
     return NULL;
 }

=== modified file 'src/display.c'
--- src/display.c	2013-10-10 15:35:12 +0000
+++ src/display.c	2014-01-06 16:20:16 +0000
@@ -77,19 +77,6 @@
  */
 static unsigned screen_off_overrides;
 
-enum fb_state {
-    FB_SLEEP,
-    FB_AWAKE,
-
-    NUM_FB_STATES
-};
-static enum fb_state fb_state = FB_AWAKE;
-
-static const char *fb_file_names[] = {
-    [FB_SLEEP] = "/sys/power/wait_for_fb_sleep",
-    [FB_AWAKE] = "/sys/power/wait_for_fb_wake"
-};
-
 static const char *display_state_strs[POWERD_NUM_DISPLAY_STATES] = {
     [POWERD_DISPLAY_STATE_DONT_CARE]    = "off",
     [POWERD_DISPLAY_STATE_ON]           = "on",
@@ -254,12 +241,12 @@
                 powerd_warn("Request for active state failed");
 
             /*
-             * If the current state is suspend we need to wait for
-             * notification of leaving the active state, otherwise the
+             * If currently in the suspend state we need to wait for
+             * notification of entering the active state, otherwise the
              * screen may fail to turn on for devices using earlysuspend.
              * Otherwise we can turn on the screen right away.
              */
-            if (fb_state == FB_AWAKE)
+            if (!powerd_suspend_active())
                 turn_on_display(use_ab);
         } else {
             /* Only changing brightness */
@@ -390,55 +377,21 @@
 }
 
 
-static int wait_for_file(const char *fname)
-{
-    int fd, ret;
-    char buf;
-
-    fd = open(fname, O_RDONLY);
-    if (fd == -1)
-        return -errno;
-
-    do {
-        ret = read(fd, &buf, 1);
-    } while (ret == -1 && errno == EINTR);
-
-    close(fd);
-    return ret == -1 ? -errno : 0;
-}
-
-static gboolean fb_state_changed(gpointer data)
-{
-    fb_state = (enum fb_state)data;
-    powerd_debug("fb state %s", fb_state == FB_SLEEP ? "sleep" : "awake");
-    if (fb_state == FB_AWAKE && powerd_display_enabled())
+static gboolean exit_suspend_worker(gpointer unused)
+{
+    /* Make sure suspend is still disabled */
+    if (!powerd_suspend_active() && powerd_display_enabled())
         turn_on_display(ab_enabled(internal_state.flags));
     return FALSE;
 }
 
-static gpointer wait_for_fb_thread(gpointer unused)
+void powerd_display_exit_suspend(void)
 {
-    unsigned long state = fb_state;
-
-    if (state >= NUM_FB_STATES) {
-        powerd_warn("Invalid FB state");
-        return NULL;
-    }
-
-    while (1) {
-        state = (state == FB_SLEEP) ? FB_AWAKE : FB_SLEEP;
-        if (wait_for_file(fb_file_names[state])) {
-            powerd_warn("Error reading %s", fb_file_names[state]);
-            return NULL;
-        }
-        g_timeout_add(0, fb_state_changed, (gpointer)state);
-    }
-    return NULL;
+    g_timeout_add(0, exit_suspend_worker, NULL);
 }
 
 int powerd_display_init(void)
 {
-    GError *error;
     GValue v = G_VALUE_INIT;
     int brightness, max_brightness;
     struct stat stats;
@@ -487,9 +440,5 @@
     if (stat(MIR_HINT_PATH, &stats) == 0)
         running_mir = TRUE;
 
-    /* If kernel supports earlysuspend, start thread to monitor fb state */
-    if (!access(fb_file_names[FB_SLEEP], F_OK))
-        g_thread_new("powerd_fb_wake_monitor", wait_for_fb_thread, &error);
-
     return 0;
 }

=== modified file 'src/power-request.c'
--- src/power-request.c	2013-08-09 19:53:58 +0000
+++ src/power-request.c	2014-01-06 16:20:16 +0000
@@ -86,6 +86,8 @@
 static gint update_pending;
 static gboolean ack_pending;
 
+static gboolean suspend_active = FALSE;
+
 static const char power_request_wakelock_name[] = "powerd_power_request";
 gint suspend_block_count;
 
@@ -357,6 +359,29 @@
     return TRUE;
 }
 
+static int enter_suspend(void)
+{
+    int ret = libsuspend_enter_suspend();
+    if (!ret)
+        suspend_active = TRUE;
+    return ret;
+}
+
+static int exit_suspend(void)
+{
+    int ret = libsuspend_exit_suspend();
+    if (!ret) {
+        suspend_active = FALSE;
+        powerd_display_exit_suspend();
+    }
+    return ret;
+}
+
+gboolean powerd_suspend_active(void)
+{
+    return suspend_active;
+}
+
 /* Must only be called from main loop */
 static enum SysPowerStates max_requested_state(void)
 {
@@ -394,7 +419,7 @@
 
     if (current_system_state == POWERD_SYS_STATE_SUSPEND) {
         powerd_debug("exiting suspend");
-        ret = libsuspend_exit_suspend();
+        ret = exit_suspend();
         if (ret)
             powerd_warn("Failed to exit suspend: %d", ret);
     }
@@ -448,7 +473,7 @@
 
     if (pending_system_state == POWERD_SYS_STATE_SUSPEND && allow_suspend) {
         powerd_debug("entering suspend");
-        ret = libsuspend_enter_suspend();
+        ret = enter_suspend();
         if (ret)
             powerd_warn("Failed to enter suspend: %d", ret);
     }

=== modified file 'src/powerd-internal.h'
--- src/powerd-internal.h	2013-10-10 15:35:12 +0000
+++ src/powerd-internal.h	2014-01-06 16:20:16 +0000
@@ -102,6 +102,7 @@
 gboolean handle_list_display_requests(PowerdSource *obj,
                                       GDBusMethodInvocation *invocation);
 void clear_disp_state_by_owner(const char *owner);
+void powerd_display_exit_suspend(void);
 
 /* System power state requests */
 gboolean handle_list_sys_requests(PowerdSource *obj,
@@ -124,6 +125,7 @@
 void powerd_sys_state_signal_emit(enum SysPowerStates state);
 void powerd_display_state_signal_emit(struct powerd_display_request *req);
 void clear_sys_state_by_owner(const char *owner);
+gboolean powerd_suspend_active(void);
 
 /* dbus name watch functions */
 void dbus_name_watch_init(void);