Merge lp:~jhodapp/powerd/fix-music-app-next into lp:powerd

[Jim Hodapp]

To prevent the device from suspending right away after the last lock is released, add a timeout and wait 6 seconds to release the final lock. This fixes bug: https://bugs.launchpad.net/powerd/+bug/1342351

To prevent the device from suspending right away after the last lock is released, add a timeout and wait 6 seconds to release the final lock. This fixes bug: https://bugs.launchpad.net/powerd/+bug/1342351

[Seth Forshee (sforshee)]

First, I'd argue that powerd shouldn't have this sort of behavior at all, and that the media hub or whoever shouldn't release it's active state request until it's really okay for the device to suspend.

Assuming that powerd should have this behavior, this isn't the right way to do it. The point of using a wake lock here is to avoid a race with the in-kernel autosuspend and handling an incoming request. I.e., block suspend in the kernel until the reqeust can get handed over to the main thread and that thread disables suspend. If powerd were ever used with a kernel lacking autosuspend (e.g. the stock distro kernels) this will fail, because there's no notion of wake locks in these kernels.

It would be better to move the call to enter_suspend() to happen in a timeout 6 seconds in the future, but you've got to make sure that the timeout is cancelled if leaving the suspend state in the interim.

review: Disapprove

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:137
http://jenkins.qa.ubuntu.com/job/powerd-ci/158/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/powerd-utopic-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/powerd-utopic-armhf-ci/10/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Jim Hodapp (jhodapp)]

Seth, thanks for the feedback. I wasn't quite sure where it should be done and thought about doing that in enter_suspend() as well. It wasn't quite obvious to me what the difference is between the two. Maybe adding a code comment for both functions would be appropriate to make it absolutely clear what the difference is?

I could go either way for a fix, either in powerd or media-hub. I'm discussing your objections to putting this in powerd with rsalveti. I do know of a way of accomplishing this in media-hub and may decide to move it there.

[Ricardo Salveti (rsalveti)]

Yeah, let's land this in media-hub for now, better to avoid races and regressions.

Unmerged revisions

137. By Jim Hodapp on 2014-08-06

To prevent the device from suspending right away after the last lock is released, add a timeout and wait 6 seconds to release the final lock. This fixes bug: https://bugs.launchpad.net/music-app/+bug/1342351

=== modified file 'src/power-request.c'
--- src/power-request.c	2014-05-13 22:05:20 +0000
+++ src/power-request.c	2014-08-06 16:08:38 +0000
@@ -127,10 +127,22 @@
     }
 }
 
+static gboolean release_wake_lock_cb(gpointer user_data)
+{
+    int ret;
+
+    powerd_debug("Unblocking suspend");
+    ret = libsuspend_release_wake_lock(power_request_wakelock_name);
+    if (ret)
+        powerd_warn("Could not release wake lock");
+
+    // Only call timeout callback once
+    return FALSE;
+}
+
 static void unblock_suspend(void)
 {
     gint old_count;
-    int ret;
     gboolean retry;
 
     do {
@@ -142,10 +154,12 @@
         retry = !g_atomic_int_compare_and_exchange(&suspend_block_count,
                                                    old_count, old_count - 1);
         if (!retry && old_count == 1) {
-            powerd_debug("Unblocking suspend");
-            ret = libsuspend_release_wake_lock(power_request_wakelock_name);
-            if (ret)
-                powerd_warn("Could not release wake lock");
+            // To prevent the device from suspending right away after the last
+            // lock is released, add a timeout and wait 6 seconds to release the
+            // final lock.
+            // This fixes bug: https://bugs.launchpad.net/music-app/+bug/1342351
+            g_timeout_add_seconds(6, release_wake_lock_cb, NULL);
+            powerd_debug("Delaying suspend 6 seconds");
         }
     } while (retry);
 }