Ping client Binders without autosuspend lock heldandroid-13.0.0_r49android-13.0.0_r45android-13.0.0_r44android-13.0.0_r43android-13.0.0_r42android-13.0.0_r41android-13.0.0_r40android-13.0.0_r39android-13.0.0_r38android-13.0.0_r37android-13.0.0_r36android-13.0.0_r35android-13.0.0_r34android-13.0.0_r33android-13.0.0_r32android13-qpr2-s9-releaseandroid13-qpr2-s8-releaseandroid13-qpr2-s7-releaseandroid13-qpr2-s6-releaseandroid13-qpr2-s5-releaseandroid13-qpr2-s3-releaseandroid13-qpr2-s2-releaseandroid13-qpr2-s12-releaseandroid13-qpr2-s11-releaseandroid13-qpr2-s10-releaseandroid13-qpr2-s1-releaseandroid13-qpr2-releaseandroid13-qpr2-b-s1-release

This fixes a deadlock situation:
- system_server is calling acquire/releaseWakeLock on the system suspend
  HAL, but is blocked because mAutosuspendLock is currently held
- The system suspend HAL's autosuspend thread is holding the
  mAutosuspendLock, but blocked in pingBinder(). If system_server's
  Binder thread pool is exhausted and all Binder threads are waiting on
  the acquire/releaseWakeLock call to return.

This change introduces a new lock to protect the list of client tokens.
When checking client liveness by pinging the Binder tokens, the
mAutosuspendLock must not be held, to avoid the circular dependency
described above.

The lock ordering is mAutosuspendClientTokensLock > mAutosuspendLock,
ie. mAutosuspendClientTokensLock must be locked before mAutosuspendLock.

Bug: 241476033
Bug: 237496877
Bug: 241283953
Test: atest SystemSuspendV1_0UnitTest SystemSuspendV1_0AidlTest
Merged-In: Ia480d1f77d632d7c839a1012f5cd7e7555130406
Change-Id: Ia480d1f77d632d7c839a1012f5cd7e7555130406

4 files changed, 166 insertions, 76 deletions

diff --git a/suspend/1.0/default/Android.bp b/suspend/1.0/default/Android.bp
index 9bcbc3c..eca48be 100644
--- a/suspend/1.0/default/Android.bp
+++ b/suspend/1.0/default/Android.bp
@@ -30,6 +30,7 @@ cc_defaults {
     cflags: [
         "-Wall",
         "-Werror",
+        "-Wthread-safety",
     ],
     cpp_std: "c++17",
 }
diff --git a/suspend/1.0/default/SystemSuspend.cpp b/suspend/1.0/default/SystemSuspend.cpp
index a0e08b4..5e5a745 100644
--- a/suspend/1.0/default/SystemSuspend.cpp
+++ b/suspend/1.0/default/SystemSuspend.cpp
@@ -158,13 +158,14 @@ SystemSuspend::SystemSuspend(unique_fd wakeupCountFd, unique_fd stateFd, unique_
 }
 
 bool SystemSuspend::enableAutosuspend(const sp<IBinder>& token) {
+    auto tokensLock = std::lock_guard(mAutosuspendClientTokensLock);
     auto autosuspendLock = std::lock_guard(mAutosuspendLock);
 
-    bool hasToken = std::find(mDisableAutosuspendTokens.begin(), mDisableAutosuspendTokens.end(),
-                              token) != mDisableAutosuspendTokens.end();
+    bool hasToken = std::find(mAutosuspendClientTokens.begin(), mAutosuspendClientTokens.end(),
+                              token) != mAutosuspendClientTokens.end();
 
     if (!hasToken) {
-        mDisableAutosuspendTokens.push_back(token);
+        mAutosuspendClientTokens.push_back(token);
     }
 
     if (mAutosuspendEnabled) {
@@ -178,7 +179,7 @@ bool SystemSuspend::enableAutosuspend(const sp<IBinder>& token) {
 }
 
 void SystemSuspend::disableAutosuspendLocked() {
-    mDisableAutosuspendTokens.clear();
+    mAutosuspendClientTokens.clear();
     if (mAutosuspendEnabled) {
         mAutosuspendEnabled = false;
         mAutosuspendCondVar.notify_all();
@@ -187,28 +188,37 @@ void SystemSuspend::disableAutosuspendLocked() {
 }
 
 void SystemSuspend::disableAutosuspend() {
+    auto tokensLock = std::lock_guard(mAutosuspendClientTokensLock);
     auto autosuspendLock = std::lock_guard(mAutosuspendLock);
     disableAutosuspendLocked();
 }
 
-bool SystemSuspend::hasAliveAutosuspendTokenLocked() {
-    mDisableAutosuspendTokens.erase(
-        std::remove_if(mDisableAutosuspendTokens.begin(), mDisableAutosuspendTokens.end(),
+void SystemSuspend::checkAutosuspendClientsLivenessLocked() {
+    // Ping autosuspend client tokens, remove any dead tokens from the list.
+    // mAutosuspendLock must not be held when calling this, as that could lead to a deadlock
+    // if pingBinder() can't be processed by system_server because it's Binder thread pool is
+    // exhausted and blocked on acquire/release wakelock calls.
+    mAutosuspendClientTokens.erase(
+        std::remove_if(mAutosuspendClientTokens.begin(), mAutosuspendClientTokens.end(),
                        [](const sp<IBinder>& token) { return token->pingBinder() != OK; }),
-        mDisableAutosuspendTokens.end());
+        mAutosuspendClientTokens.end());
+}
 
-    return !mDisableAutosuspendTokens.empty();
+bool SystemSuspend::hasAliveAutosuspendTokenLocked() {
+    return !mAutosuspendClientTokens.empty();
 }
 
 SystemSuspend::~SystemSuspend(void) {
+    auto tokensLock = std::lock_guard(mAutosuspendClientTokensLock);
     auto autosuspendLock = std::unique_lock(mAutosuspendLock);
 
     // signal autosuspend thread to shut down
     disableAutosuspendLocked();
 
     // wait for autosuspend thread to exit
-    mAutosuspendCondVar.wait_for(autosuspendLock, 100ms,
-                                 [this] { return !mAutosuspendThreadCreated; });
+    mAutosuspendCondVar.wait_for(autosuspendLock, 100ms, [this]() REQUIRES(mAutosuspendLock) {
+        return !mAutosuspendThreadCreated;
+    });
 }
 
 bool SystemSuspend::forceSuspend() {
@@ -273,80 +283,99 @@ void SystemSuspend::initAutosuspendLocked() {
         bool shouldSleep = true;
 
         while (true) {
-            if (!mAutosuspendEnabled) {
-                mAutosuspendThreadCreated = false;
-                return;
-            }
-            // If we got here by a failed write to /sys/power/wakeup_count; don't sleep
-            // since we didn't attempt to suspend on the last cycle of this loop.
-            if (shouldSleep) {
-                mAutosuspendCondVar.wait_for(autosuspendLock, mSleepTime,
-                                             [this] { return !mAutosuspendEnabled; });
-            }
-
-            if (!mAutosuspendEnabled) continue;
-
-            string wakeupCount;
             {
-                autosuspendLock.unlock();
+                base::ScopedLockAssertion autosuspendLocked(mAutosuspendLock);
 
-                lseek(mWakeupCountFd, 0, SEEK_SET);
-                wakeupCount = readFd(mWakeupCountFd);
+                if (!mAutosuspendEnabled) {
+                    mAutosuspendThreadCreated = false;
+                    return;
+                }
+                // If we got here by a failed write to /sys/power/wakeup_count; don't sleep
+                // since we didn't attempt to suspend on the last cycle of this loop.
+                if (shouldSleep) {
+                    mAutosuspendCondVar.wait_for(
+                        autosuspendLock, mSleepTime,
+                        [this]() REQUIRES(mAutosuspendLock) { return !mAutosuspendEnabled; });
+                }
 
-                autosuspendLock.lock();
+                if (!mAutosuspendEnabled) continue;
+                autosuspendLock.unlock();
             }
 
-            if (wakeupCount.empty()) {
-                PLOG(ERROR) << "error reading from /sys/power/wakeup_count";
-                continue;
-            }
+            lseek(mWakeupCountFd, 0, SEEK_SET);
+            string wakeupCount = readFd(mWakeupCountFd);
 
-            shouldSleep = false;
+            {
+                autosuspendLock.lock();
+                base::ScopedLockAssertion autosuspendLocked(mAutosuspendLock);
 
-            mAutosuspendCondVar.wait(
-                autosuspendLock, [this] { return mSuspendCounter == 0 || !mAutosuspendEnabled; });
-            // The mutex is locked and *MUST* remain locked until we write to /sys/power/state.
-            // Otherwise, a WakeLock might be acquired after we check mSuspendCounter and before we
-            // write to /sys/power/state.
+                if (wakeupCount.empty()) {
+                    PLOG(ERROR) << "error reading from /sys/power/wakeup_count";
+                    continue;
+                }
 
-            if (!mAutosuspendEnabled) continue;
+                shouldSleep = false;
 
-            if (!hasAliveAutosuspendTokenLocked()) {
-                disableAutosuspendLocked();
-                continue;
-            }
+                mAutosuspendCondVar.wait(autosuspendLock, [this]() REQUIRES(mAutosuspendLock) {
+                    return mSuspendCounter == 0 || !mAutosuspendEnabled;
+                });
 
-            if (!WriteStringToFd(wakeupCount, mWakeupCountFd)) {
-                PLOG(VERBOSE) << "error writing from /sys/power/wakeup_count";
-                continue;
+                if (!mAutosuspendEnabled) continue;
+                autosuspendLock.unlock();
             }
-            bool success = WriteStringToFd(kSleepState, mStateFd);
-            shouldSleep = true;
 
+            bool success;
             {
-                autosuspendLock.unlock();
+                auto tokensLock = std::lock_guard(mAutosuspendClientTokensLock);
+                checkAutosuspendClientsLivenessLocked();
 
-                if (!success) {
-                    PLOG(VERBOSE) << "error writing to /sys/power/state";
+                autosuspendLock.lock();
+                base::ScopedLockAssertion autosuspendLocked(mAutosuspendLock);
+
+                if (!hasAliveAutosuspendTokenLocked()) {
+                    disableAutosuspendLocked();
+                    continue;
                 }
 
-                struct SuspendTime suspendTime = readSuspendTime(mSuspendTimeFd);
-                updateSleepTime(success, suspendTime);
+                // Check suspend counter hasn't increased while checking client liveness
+                if (mSuspendCounter > 0) {
+                    continue;
+                }
 
-                std::vector<std::string> wakeupReasons = readWakeupReasons(mWakeupReasonsFd);
-                if (wakeupReasons == std::vector<std::string>({kUnknownWakeup})) {
-                    LOG(INFO) << "Unknown/empty wakeup reason. Re-opening wakeup_reason file.";
+                // The mutex is locked and *MUST* remain locked until we write to /sys/power/state.
+                // Otherwise, a WakeLock might be acquired after we check mSuspendCounter and before
+                // we write to /sys/power/state.
 
-                    mWakeupReasonsFd =
-                        std::move(reopenFileUsingFd(mWakeupReasonsFd.get(), O_CLOEXEC | O_RDONLY));
+                if (!WriteStringToFd(wakeupCount, mWakeupCountFd)) {
+                    PLOG(VERBOSE) << "error writing to /sys/power/wakeup_count";
+                    continue;
                 }
-                mWakeupList.update(wakeupReasons);
+                success = WriteStringToFd(kSleepState, mStateFd);
+                shouldSleep = true;
+
+                autosuspendLock.unlock();
+            }
 
-                mControlService->notifyWakeup(success, wakeupReasons);
+            if (!success) {
+                PLOG(VERBOSE) << "error writing to /sys/power/state";
+            }
 
-                // Take the lock before returning to the start of the loop
-                autosuspendLock.lock();
+            struct SuspendTime suspendTime = readSuspendTime(mSuspendTimeFd);
+            updateSleepTime(success, suspendTime);
+
+            std::vector<std::string> wakeupReasons = readWakeupReasons(mWakeupReasonsFd);
+            if (wakeupReasons == std::vector<std::string>({kUnknownWakeup})) {
+                LOG(INFO) << "Unknown/empty wakeup reason. Re-opening wakeup_reason file.";
+
+                mWakeupReasonsFd =
+                    std::move(reopenFileUsingFd(mWakeupReasonsFd.get(), O_CLOEXEC | O_RDONLY));
             }
+            mWakeupList.update(wakeupReasons);
+
+            mControlService->notifyWakeup(success, wakeupReasons);
+
+            // Take the lock before returning to the start of the loop
+            autosuspendLock.lock();
         }
     });
     autosuspendThread.detach();
diff --git a/suspend/1.0/default/SystemSuspend.h b/suspend/1.0/default/SystemSuspend.h
index ddcee24..a3f9a00 100644
--- a/suspend/1.0/default/SystemSuspend.h
+++ b/suspend/1.0/default/SystemSuspend.h
@@ -18,6 +18,7 @@
 #define ANDROID_SYSTEM_SYSTEM_SUSPEND_V1_0_H
 
 #include <android-base/result.h>
+#include <android-base/thread_annotations.h>
 #include <android-base/unique_fd.h>
 #include <android/system/suspend/internal/SuspendInfo.h>
 #include <utils/RefBase.h>
@@ -99,27 +100,39 @@ class SystemSuspend : public RefBase {
 
    private:
     ~SystemSuspend(void) override;
-    void initAutosuspendLocked();
-    void disableAutosuspendLocked();
-    bool hasAliveAutosuspendTokenLocked();
 
-    std::mutex mAutosuspendLock;
-    std::condition_variable mAutosuspendCondVar;
-    uint32_t mSuspendCounter;
+    std::mutex mAutosuspendClientTokensLock;
+    std::mutex mAutosuspendLock ACQUIRED_AFTER(mAutosuspendClientTokensLock);
+    std::mutex mSuspendInfoLock;
+
+    void initAutosuspendLocked()
+        EXCLUSIVE_LOCKS_REQUIRED(mAutosuspendClientTokensLock, mAutosuspendLock);
+    void disableAutosuspendLocked()
+        EXCLUSIVE_LOCKS_REQUIRED(mAutosuspendClientTokensLock, mAutosuspendLock);
+    void checkAutosuspendClientsLivenessLocked()
+        EXCLUSIVE_LOCKS_REQUIRED(mAutosuspendClientTokensLock);
+    bool hasAliveAutosuspendTokenLocked() EXCLUSIVE_LOCKS_REQUIRED(mAutosuspendClientTokensLock);
+
+    std::condition_variable mAutosuspendCondVar GUARDED_BY(mAutosuspendLock);
+    uint32_t mSuspendCounter GUARDED_BY(mAutosuspendLock);
+
+    std::vector<sp<IBinder>> mAutosuspendClientTokens GUARDED_BY(mAutosuspendClientTokensLock);
+    std::atomic<bool> mAutosuspendEnabled GUARDED_BY(mAutosuspendLock){false};
+    std::atomic<bool> mAutosuspendThreadCreated GUARDED_BY(mAutosuspendLock){false};
+
     unique_fd mWakeupCountFd;
     unique_fd mStateFd;
 
     unique_fd mSuspendStatsFd;
     unique_fd mSuspendTimeFd;
 
-    std::mutex mSuspendInfoLock;
-    SuspendInfo mSuspendInfo;
+    SuspendInfo mSuspendInfo GUARDED_BY(mSuspendInfoLock);
 
     const SleepTimeConfig kSleepTimeConfig;
 
     // Amount of thread sleep time between consecutive iterations of the suspend loop
     std::chrono::milliseconds mSleepTime;
-    int32_t mNumConsecutiveBadSuspends;
+    int32_t mNumConsecutiveBadSuspends GUARDED_BY(mSuspendInfoLock);
 
     // Updates thread sleep time and suspend stats depending on the result of suspend attempt
     void updateSleepTime(bool success, const struct SuspendTime& suspendTime);
@@ -137,10 +150,6 @@ class SystemSuspend : public RefBase {
     unique_fd mWakeLockFd;
     unique_fd mWakeUnlockFd;
     unique_fd mWakeupReasonsFd;
-
-    std::atomic<bool> mAutosuspendEnabled{false};
-    std::atomic<bool> mAutosuspendThreadCreated{false};
-    std::vector<sp<IBinder>> mDisableAutosuspendTokens;
 };
 
 }  // namespace V1_0
diff --git a/suspend/1.0/default/SystemSuspendUnitTest.cpp b/suspend/1.0/default/SystemSuspendUnitTest.cpp
index c779623..f6e216b 100644
--- a/suspend/1.0/default/SystemSuspendUnitTest.cpp
+++ b/suspend/1.0/default/SystemSuspendUnitTest.cpp
@@ -335,6 +335,57 @@ TEST_F(SystemSuspendTest, BlockAutosuspendIfBinderIsDead) {
     ASSERT_TRUE(isSystemSuspendBlocked(150));
 }
 
+TEST_F(SystemSuspendTest, UnresponsiveClientDoesNotBlockAcquireRelease) {
+    static std::mutex _lock;
+    static std::condition_variable inPingBinderCondVar;
+    static bool inPingBinder = false;
+
+    class UnresponsiveBinder : public BBinder {
+        android::status_t pingBinder() override {
+            auto lock = std::unique_lock(_lock);
+            inPingBinder = true;
+            inPingBinderCondVar.notify_all();
+
+            // Block pingBinder until test finishes and releases its lock
+            inPingBinderCondVar.wait(lock);
+            return android::UNKNOWN_ERROR;
+        }
+    };
+
+    systemSuspend->disableAutosuspend();
+    unblockSystemSuspendFromWakeupCount();
+    ASSERT_TRUE(isSystemSuspendBlocked());
+
+    auto token = sp<UnresponsiveBinder>::make();
+    bool enabled = false;
+    controlServiceInternal->enableAutosuspend(token, &enabled);
+    unblockSystemSuspendFromWakeupCount();
+
+    auto lock = std::unique_lock(_lock);
+    // wait until pingBinder has been called.
+    if (!inPingBinder) {
+        inPingBinderCondVar.wait(lock);
+    }
+    // let pingBinder finish once we release the test lock
+    inPingBinderCondVar.notify_all();
+
+    std::condition_variable wakeLockAcquired;
+    std::thread(
+        [this](std::condition_variable& wakeLockAcquired) {
+            std::shared_ptr<IWakeLock> testLock = acquireWakeLock("testLock");
+            testLock->release();
+            wakeLockAcquired.notify_all();
+        },
+        std::ref(wakeLockAcquired))
+        .detach();
+
+    std::mutex _acquireReleaseLock;
+    auto acquireReleaseLock = std::unique_lock(_acquireReleaseLock);
+    bool timedOut = wakeLockAcquired.wait_for(acquireReleaseLock, 200ms) == std::cv_status::timeout;
+
+    ASSERT_FALSE(timedOut);
+}
+
 TEST_F(SystemSuspendTest, AutosuspendLoop) {
     checkLoop(5);
 }