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

Proposed by Seth Forshee on 2014-01-03
Status: Merged
Approved by: Ricardo Salveti on 2014-01-08
Approved revision: 107
Merged at revision: 105
Proposed branch: lp:~sforshee/powerd/fix-display-timing
Merge into: lp:powerd
Diff against target: 350 lines (+139/-65)
5 files modified
CMakeLists.txt (+3/-0)
libsuspend/earlysuspend.c (+98/-3)
src/display.c (+9/-60)
src/power-request.c (+27/-2)
src/powerd-internal.h (+2/-0)
To merge this branch: bzr merge lp:~sforshee/powerd/fix-display-timing
Reviewer Review Type Date Requested Status
Ricardo Salveti 2014-01-03 Approve on 2014-01-08
PS Jenkins bot continuous-integration Approve on 2014-01-06
Kevin DuBois (community) testing 2014-01-03 Approve on 2014-01-03
Review via email: mp+200431@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Kevin DuBois (kdub) wrote :

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

review: Approve (testing)
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) wrote :
review: Approve (continuous-integration)
Ricardo Salveti (rsalveti) wrote :

Code looks good and works as expected.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2013-12-06 22:11:52 +0000
3+++ CMakeLists.txt 2014-01-06 16:20:16 +0000
4@@ -17,6 +17,8 @@
5
6 set(GDBUS_NAME powerd-dbus)
7
8+find_package(Threads REQUIRED)
9+
10 find_package(PkgConfig)
11 pkg_check_modules(ANDROID_HEADERS android-headers)
12 pkg_check_modules(HYBRIS_IS libis)
13@@ -107,6 +109,7 @@
14 powerd
15 suspend
16
17+ ${CMAKE_THREAD_LIBS_INIT}
18 ${HYBRIS_IS_LIBRARIES}
19 ${HYBRIS_SF_LIBRARIES}
20 ${GLIB_LIBRARIES}
21
22=== modified file 'libsuspend/earlysuspend.c'
23--- libsuspend/earlysuspend.c 2013-05-23 14:45:14 +0000
24+++ libsuspend/earlysuspend.c 2014-01-06 16:20:16 +0000
25@@ -18,9 +18,34 @@
26
27 #include <stdio.h>
28 #include <string.h>
29+#include <unistd.h>
30+#include <fcntl.h>
31+#include <errno.h>
32+#include <pthread.h>
33 #include "common.h"
34 #include "sysfs.h"
35
36+/* Use to suppress unused parameter warnings */
37+#define __unused __attribute__ ((unused))
38+
39+enum fb_state {
40+ FB_SLEEP,
41+ FB_AWAKE,
42+
43+ NUM_FB_STATES
44+};
45+static enum fb_state fb_state = FB_AWAKE;
46+
47+static const char *fb_file_names[] = {
48+ [FB_SLEEP] = "/sys/power/wait_for_fb_sleep",
49+ [FB_AWAKE] = "/sys/power/wait_for_fb_wake"
50+};
51+
52+static pthread_t fb_monitor_thread;
53+static pthread_mutex_t fb_state_mutex = PTHREAD_MUTEX_INITIALIZER;
54+static pthread_cond_t fb_state_cond = PTHREAD_COND_INITIALIZER;
55+static int wait_for_fb = 0;
56+
57 static const char state_path[] = "/sys/power/state";
58 static const char wakelock_path[] = "/sys/power/wake_lock";
59 static const char wakeunlock_path[] = "/sys/power/wake_unlock";
60@@ -29,15 +54,83 @@
61 static const char mem_str[] = "mem";
62 static const char on_str[] = "on";
63
64+static int wait_for_file(const char *fname)
65+{
66+ int fd, ret;
67+ char buf;
68+
69+ fd = open(fname, O_RDONLY);
70+ if (fd == -1)
71+ return -errno;
72+
73+ do {
74+ ret = read(fd, &buf, 1);
75+ } while (ret == -1 && errno == EINTR);
76+
77+ close(fd);
78+ return ret == -1 ? -errno : 0;
79+}
80+
81+static void *fb_monitor_thread_func(__unused void *unused)
82+{
83+ enum fb_state next_state;
84+ int ret;
85+
86+ while (1) {
87+ next_state = fb_state == FB_SLEEP ? FB_AWAKE : FB_SLEEP;
88+ ret = wait_for_file(fb_file_names[next_state]);
89+ if (ret)
90+ continue;
91+
92+ pthread_mutex_lock(&fb_state_mutex);
93+ fb_state = next_state;
94+ pthread_cond_signal(&fb_state_cond);
95+ pthread_mutex_unlock(&fb_state_mutex);
96+ }
97+
98+ wait_for_fb = 0;
99+ return NULL;
100+}
101+
102+/* Returns 1 if fb monitor thread started, 0 otherwise */
103+static int start_fb_monitor_thread(void)
104+{
105+ if (access(fb_file_names[FB_SLEEP], F_OK))
106+ return 0;
107+ if (access(fb_file_names[FB_AWAKE], F_OK))
108+ return 0;
109+
110+ return !pthread_create(&fb_monitor_thread, NULL,
111+ fb_monitor_thread_func, NULL);
112+}
113+
114 static int earlysuspend_enter(void)
115 {
116- int ret = sysfs_write(state_path, mem_str, ARRAY_SIZE(mem_str) - 1);
117+ int ret;
118+ int len = ARRAY_SIZE(mem_str) - 1;
119+
120+ ret = sysfs_write(state_path, mem_str, len);
121+ if (ret == len && wait_for_fb) {
122+ pthread_mutex_lock(&fb_state_mutex);
123+ while (fb_state != FB_SLEEP)
124+ pthread_cond_wait(&fb_state_cond, &fb_state_mutex);
125+ pthread_mutex_unlock(&fb_state_mutex);
126+ }
127 return ret < 0 ? ret : 0;
128 }
129
130 static int earlysuspend_exit(void)
131 {
132- int ret = sysfs_write(state_path, on_str, ARRAY_SIZE(on_str) - 1);
133+ int ret;
134+ int len = ARRAY_SIZE(on_str) - 1;
135+
136+ ret = sysfs_write(state_path, on_str, len);
137+ if (ret == len && wait_for_fb) {
138+ pthread_mutex_lock(&fb_state_mutex);
139+ while (fb_state != FB_AWAKE)
140+ pthread_cond_wait(&fb_state_cond, &fb_state_mutex);
141+ pthread_mutex_unlock(&fb_state_mutex);
142+ }
143 return ret < 0 ? ret : 0;
144 }
145
146@@ -64,8 +157,10 @@
147 {
148 if (!sysfs_file_exists(autosleep_path) &&
149 sysfs_file_exists(wakelock_path) &&
150- sysfs_file_exists(state_path))
151+ sysfs_file_exists(state_path)) {
152+ wait_for_fb = start_fb_monitor_thread();
153 return &earlysuspend_handler;
154+ }
155
156 return NULL;
157 }
158
159=== modified file 'src/display.c'
160--- src/display.c 2013-10-10 15:35:12 +0000
161+++ src/display.c 2014-01-06 16:20:16 +0000
162@@ -77,19 +77,6 @@
163 */
164 static unsigned screen_off_overrides;
165
166-enum fb_state {
167- FB_SLEEP,
168- FB_AWAKE,
169-
170- NUM_FB_STATES
171-};
172-static enum fb_state fb_state = FB_AWAKE;
173-
174-static const char *fb_file_names[] = {
175- [FB_SLEEP] = "/sys/power/wait_for_fb_sleep",
176- [FB_AWAKE] = "/sys/power/wait_for_fb_wake"
177-};
178-
179 static const char *display_state_strs[POWERD_NUM_DISPLAY_STATES] = {
180 [POWERD_DISPLAY_STATE_DONT_CARE] = "off",
181 [POWERD_DISPLAY_STATE_ON] = "on",
182@@ -254,12 +241,12 @@
183 powerd_warn("Request for active state failed");
184
185 /*
186- * If the current state is suspend we need to wait for
187- * notification of leaving the active state, otherwise the
188+ * If currently in the suspend state we need to wait for
189+ * notification of entering the active state, otherwise the
190 * screen may fail to turn on for devices using earlysuspend.
191 * Otherwise we can turn on the screen right away.
192 */
193- if (fb_state == FB_AWAKE)
194+ if (!powerd_suspend_active())
195 turn_on_display(use_ab);
196 } else {
197 /* Only changing brightness */
198@@ -390,55 +377,21 @@
199 }
200
201
202-static int wait_for_file(const char *fname)
203-{
204- int fd, ret;
205- char buf;
206-
207- fd = open(fname, O_RDONLY);
208- if (fd == -1)
209- return -errno;
210-
211- do {
212- ret = read(fd, &buf, 1);
213- } while (ret == -1 && errno == EINTR);
214-
215- close(fd);
216- return ret == -1 ? -errno : 0;
217-}
218-
219-static gboolean fb_state_changed(gpointer data)
220-{
221- fb_state = (enum fb_state)data;
222- powerd_debug("fb state %s", fb_state == FB_SLEEP ? "sleep" : "awake");
223- if (fb_state == FB_AWAKE && powerd_display_enabled())
224+static gboolean exit_suspend_worker(gpointer unused)
225+{
226+ /* Make sure suspend is still disabled */
227+ if (!powerd_suspend_active() && powerd_display_enabled())
228 turn_on_display(ab_enabled(internal_state.flags));
229 return FALSE;
230 }
231
232-static gpointer wait_for_fb_thread(gpointer unused)
233+void powerd_display_exit_suspend(void)
234 {
235- unsigned long state = fb_state;
236-
237- if (state >= NUM_FB_STATES) {
238- powerd_warn("Invalid FB state");
239- return NULL;
240- }
241-
242- while (1) {
243- state = (state == FB_SLEEP) ? FB_AWAKE : FB_SLEEP;
244- if (wait_for_file(fb_file_names[state])) {
245- powerd_warn("Error reading %s", fb_file_names[state]);
246- return NULL;
247- }
248- g_timeout_add(0, fb_state_changed, (gpointer)state);
249- }
250- return NULL;
251+ g_timeout_add(0, exit_suspend_worker, NULL);
252 }
253
254 int powerd_display_init(void)
255 {
256- GError *error;
257 GValue v = G_VALUE_INIT;
258 int brightness, max_brightness;
259 struct stat stats;
260@@ -487,9 +440,5 @@
261 if (stat(MIR_HINT_PATH, &stats) == 0)
262 running_mir = TRUE;
263
264- /* If kernel supports earlysuspend, start thread to monitor fb state */
265- if (!access(fb_file_names[FB_SLEEP], F_OK))
266- g_thread_new("powerd_fb_wake_monitor", wait_for_fb_thread, &error);
267-
268 return 0;
269 }
270
271=== modified file 'src/power-request.c'
272--- src/power-request.c 2013-08-09 19:53:58 +0000
273+++ src/power-request.c 2014-01-06 16:20:16 +0000
274@@ -86,6 +86,8 @@
275 static gint update_pending;
276 static gboolean ack_pending;
277
278+static gboolean suspend_active = FALSE;
279+
280 static const char power_request_wakelock_name[] = "powerd_power_request";
281 gint suspend_block_count;
282
283@@ -357,6 +359,29 @@
284 return TRUE;
285 }
286
287+static int enter_suspend(void)
288+{
289+ int ret = libsuspend_enter_suspend();
290+ if (!ret)
291+ suspend_active = TRUE;
292+ return ret;
293+}
294+
295+static int exit_suspend(void)
296+{
297+ int ret = libsuspend_exit_suspend();
298+ if (!ret) {
299+ suspend_active = FALSE;
300+ powerd_display_exit_suspend();
301+ }
302+ return ret;
303+}
304+
305+gboolean powerd_suspend_active(void)
306+{
307+ return suspend_active;
308+}
309+
310 /* Must only be called from main loop */
311 static enum SysPowerStates max_requested_state(void)
312 {
313@@ -394,7 +419,7 @@
314
315 if (current_system_state == POWERD_SYS_STATE_SUSPEND) {
316 powerd_debug("exiting suspend");
317- ret = libsuspend_exit_suspend();
318+ ret = exit_suspend();
319 if (ret)
320 powerd_warn("Failed to exit suspend: %d", ret);
321 }
322@@ -448,7 +473,7 @@
323
324 if (pending_system_state == POWERD_SYS_STATE_SUSPEND && allow_suspend) {
325 powerd_debug("entering suspend");
326- ret = libsuspend_enter_suspend();
327+ ret = enter_suspend();
328 if (ret)
329 powerd_warn("Failed to enter suspend: %d", ret);
330 }
331
332=== modified file 'src/powerd-internal.h'
333--- src/powerd-internal.h 2013-10-10 15:35:12 +0000
334+++ src/powerd-internal.h 2014-01-06 16:20:16 +0000
335@@ -102,6 +102,7 @@
336 gboolean handle_list_display_requests(PowerdSource *obj,
337 GDBusMethodInvocation *invocation);
338 void clear_disp_state_by_owner(const char *owner);
339+void powerd_display_exit_suspend(void);
340
341 /* System power state requests */
342 gboolean handle_list_sys_requests(PowerdSource *obj,
343@@ -124,6 +125,7 @@
344 void powerd_sys_state_signal_emit(enum SysPowerStates state);
345 void powerd_display_state_signal_emit(struct powerd_display_request *req);
346 void clear_sys_state_by_owner(const char *owner);
347+gboolean powerd_suspend_active(void);
348
349 /* dbus name watch functions */
350 void dbus_name_watch_init(void);

Subscribers

People subscribed via source and target branches