From b53742607af505c80e1be5506dceebd93b534681 Mon Sep 17 00:00:00 2001 From: Tri Vo Date: Thu, 30 Aug 2018 17:58:41 -0700 Subject: [PATCH] Add ISystemSuspendCallback. The callback will be used to synchronously report system suspend events. Current usage: BatteryStatsService needs to be notified about each system suspend attempt and whether it was successful. Bug: 78888165 Test: SystemSuspendV1_0UnitTest Change-Id: I234f74f6f3c35e5d6bb33ceda569e11520850ccf --- suspend/1.0/Android.bp | 1 + suspend/1.0/ISystemSuspend.hal | 11 ++++ suspend/1.0/ISystemSuspendCallback.hal | 27 ++++++++++ suspend/1.0/default/Android.bp | 5 +- suspend/1.0/default/SystemSuspend.cpp | 54 +++++++++++++------ suspend/1.0/default/SystemSuspend.h | 20 ++++++- suspend/1.0/default/SystemSuspendUnitTest.cpp | 78 ++++++++++++++++++++++++--- 7 files changed, 170 insertions(+), 26 deletions(-) create mode 100644 suspend/1.0/ISystemSuspendCallback.hal diff --git a/suspend/1.0/Android.bp b/suspend/1.0/Android.bp index 3f7bf93..cdf4435 100644 --- a/suspend/1.0/Android.bp +++ b/suspend/1.0/Android.bp @@ -6,6 +6,7 @@ hidl_interface { }, srcs: [ "ISystemSuspend.hal", + "ISystemSuspendCallback.hal", "IWakeLock.hal", ], interfaces: [ diff --git a/suspend/1.0/ISystemSuspend.hal b/suspend/1.0/ISystemSuspend.hal index 3452d14..344aa51 100644 --- a/suspend/1.0/ISystemSuspend.hal +++ b/suspend/1.0/ISystemSuspend.hal @@ -16,10 +16,21 @@ package android.system.suspend@1.0; +import ISystemSuspendCallback; import IWakeLock; interface ISystemSuspend { /** + * Registers a callback for system suspend events. ISystemSuspend must keep + * track of all registered callbacks unless the client process that + * registered the callback dies. + * + * @param callback the callback to register. + * @return status true on success, false otherwise. + */ + registerCallback(ISystemSuspendCallback callback) generates (bool success); + + /** * Starts automatic system suspension. * * @return status true on success, false otherwise. diff --git a/suspend/1.0/ISystemSuspendCallback.hal b/suspend/1.0/ISystemSuspendCallback.hal new file mode 100644 index 0000000..3bc88d7 --- /dev/null +++ b/suspend/1.0/ISystemSuspendCallback.hal @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.system.suspend@1.0; + +interface ISystemSuspendCallback { + /** + * An implementation of ISystemSuspend must call notifyWakeup after every + * system wakeup. + * + * @param success whether previous system suspend attempt was successful. + */ + notifyWakeup(bool success); +}; diff --git a/suspend/1.0/default/Android.bp b/suspend/1.0/default/Android.bp index 10842f5..25bdff8 100644 --- a/suspend/1.0/default/Android.bp +++ b/suspend/1.0/default/Android.bp @@ -84,7 +84,10 @@ cc_test { "system_suspend_defaults", "system_suspend_stats_defaults", ], - static_libs: ["android.system.suspend@1.0"], + static_libs: [ + "android.system.suspend@1.0", + "libgmock", + ], srcs: [ "SystemSuspend.cpp", "SystemSuspendUnitTest.cpp" diff --git a/suspend/1.0/default/SystemSuspend.cpp b/suspend/1.0/default/SystemSuspend.cpp index 14800a8..bf22f12 100644 --- a/suspend/1.0/default/SystemSuspend.cpp +++ b/suspend/1.0/default/SystemSuspend.cpp @@ -73,17 +73,12 @@ Return WakeLock::release() { void WakeLock::releaseOnce() { std::call_once(mReleased, [this]() { mSystemSuspend->decSuspendCounter(); - mSystemSuspend->deleteWakeLockStatsEntry(reinterpret_cast(this)); + mSystemSuspend->deleteWakeLockStatsEntry(reinterpret_cast(this)); }); } SystemSuspend::SystemSuspend(unique_fd wakeupCountFd, unique_fd stateFd) - : mCounterLock(), - mCounterCondVar(), - mSuspendCounter(0), - mWakeupCountFd(std::move(wakeupCountFd)), - mStateFd(std::move(stateFd)), - mStats() {} + : mSuspendCounter(0), mWakeupCountFd(std::move(wakeupCountFd)), mStateFd(std::move(stateFd)) {} Return SystemSuspend::enableAutosuspend() { static bool initialized = false; @@ -105,11 +100,33 @@ Return> SystemSuspend::acquireWakeLock(const hidl_string& name) { wlStats.set_name(name); wlStats.set_pid(getCallingPid()); // Use WakeLock's address as a unique identifier. - (*mStats.mutable_wake_lock_stats())[reinterpret_cast(wl)] = wlStats; + (*mStats.mutable_wake_lock_stats())[reinterpret_cast(wl)] = wlStats; } return wl; } +Return SystemSuspend::registerCallback(const sp& cb) { + if (!cb) { + return false; + } + auto l = std::lock_guard(mCallbackLock); + if (findCb(cb) == mCallbacks.end()) { + auto linkRet = cb->linkToDeath(this, 0 /* cookie */); + if (!linkRet.withDefault(false)) { + LOG(ERROR) << __func__ << "Cannot link to death: " + << (linkRet.isOk() ? "linkToDeath returns false" : linkRet.description()); + return false; + } + mCallbacks.push_back(cb); + } + return true; +} + +void SystemSuspend::serviceDied(uint64_t, const wp& service) { + auto l = std::lock_guard(mCallbackLock); + mCallbacks.erase(findCb(service.promote())); +} + Return SystemSuspend::debug(const hidl_handle& handle, const hidl_vec& /* options */) { if (handle == nullptr || handle->numFds < 1 || handle->data[0] < 0) { @@ -139,7 +156,7 @@ void SystemSuspend::decSuspendCounter() { } } -void SystemSuspend::deleteWakeLockStatsEntry(uint64_t id) { +void SystemSuspend::deleteWakeLockStatsEntry(WakeLockIdType id) { auto l = std::lock_guard(mStatsLock); mStats.mutable_wake_lock_stats()->erase(id); } @@ -154,19 +171,26 @@ void SystemSuspend::initAutosuspend() { continue; } - auto l = std::unique_lock(mCounterLock); - mCounterCondVar.wait(l, [this] { return mSuspendCounter == 0; }); - // The mutex is locked and *MUST* remain locked until the end of the scope. Otherwise, - // a WakeLock might be acquired after we check mSuspendCounter and before we write to - // /sys/power/state. + auto counterLock = std::unique_lock(mCounterLock); + mCounterCondVar.wait(counterLock, [this] { return mSuspendCounter == 0; }); + // 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 (!WriteStringToFd(wakeupCount, mWakeupCountFd)) { PLOG(VERBOSE) << "error writing from /sys/power/wakeup_count"; continue; } - if (!WriteStringToFd(kSleepState, mStateFd)) { + bool success = WriteStringToFd(kSleepState, mStateFd); + counterLock.unlock(); + + if (!success) { PLOG(VERBOSE) << "error writing to /sys/power/state"; } + auto callbackLock = std::lock_guard(mCallbackLock); + for (const auto& callback : mCallbacks) { + callback->notifyWakeup(success).isOk(); // ignore errors + } } }); autosuspendThread.detach(); diff --git a/suspend/1.0/default/SystemSuspend.h b/suspend/1.0/default/SystemSuspend.h index d5f2ea1..fc397fc 100644 --- a/suspend/1.0/default/SystemSuspend.h +++ b/suspend/1.0/default/SystemSuspend.h @@ -19,6 +19,8 @@ #include #include +#include +#include #include #include @@ -31,11 +33,15 @@ namespace suspend { namespace V1_0 { using ::android::base::unique_fd; +using ::android::hardware::hidl_death_recipient; using ::android::hardware::hidl_handle; using ::android::hardware::hidl_string; using ::android::hardware::hidl_vec; +using ::android::hardware::interfacesEqual; using ::android::hardware::Return; +using WakeLockIdType = uint64_t; + class SystemSuspend; std::string readFd(int fd); @@ -54,15 +60,17 @@ class WakeLock : public IWakeLock { SystemSuspend* mSystemSuspend; }; -class SystemSuspend : public ISystemSuspend { +class SystemSuspend : public ISystemSuspend, public hidl_death_recipient { public: SystemSuspend(unique_fd wakeupCountFd, unique_fd stateFd); Return enableAutosuspend() override; Return> acquireWakeLock(const hidl_string& name) override; + Return registerCallback(const sp& cb) override; Return debug(const hidl_handle& handle, const hidl_vec& options) override; + void serviceDied(uint64_t /* cookie */, const wp& service); void incSuspendCounter(); void decSuspendCounter(); - void deleteWakeLockStatsEntry(uint64_t id); + void deleteWakeLockStatsEntry(WakeLockIdType id); private: void initAutosuspend(); @@ -78,6 +86,14 @@ class SystemSuspend : public ISystemSuspend { // Never hold both locks at the same time to avoid deadlock. std::mutex mStatsLock; SystemSuspendStats mStats; + + using CbType = sp; + std::mutex mCallbackLock; + std::vector mCallbacks; + std::vector::iterator findCb(const sp& cb) { + return std::find_if(mCallbacks.begin(), mCallbacks.end(), + [&cb](const CbType& i) { return interfacesEqual(i, cb); }); + } }; } // namespace V1_0 diff --git a/suspend/1.0/default/SystemSuspendUnitTest.cpp b/suspend/1.0/default/SystemSuspendUnitTest.cpp index 4c85968..d7055d1 100644 --- a/suspend/1.0/default/SystemSuspendUnitTest.cpp +++ b/suspend/1.0/default/SystemSuspendUnitTest.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -44,6 +45,7 @@ using android::hardware::joinRpcThreadpool; using android::hardware::Return; using android::hardware::Void; using android::system::suspend::V1_0::ISystemSuspend; +using android::system::suspend::V1_0::ISystemSuspendCallback; using android::system::suspend::V1_0::IWakeLock; using android::system::suspend::V1_0::readFd; using android::system::suspend::V1_0::SystemSuspend; @@ -151,6 +153,18 @@ class SystemSuspendTest : public ::testing::Test { size_t getWakeLockCount() { return getDebugDump().wake_lock_stats().size(); } + void checkLoop(int numIter) { + for (int i = 0; i < numIter; i++) { + // Mock value for /sys/power/wakeup_count. + std::string wakeupCount = std::to_string(rand()); + ASSERT_TRUE(WriteStringToFd(wakeupCount, wakeupCountFd)); + ASSERT_EQ(readFd(wakeupCountFd), wakeupCount) + << "wakeup count value written by SystemSuspend is not equal to value given to it"; + ASSERT_EQ(readFd(stateFd), "mem") + << "SystemSuspend failed to write correct sleep state."; + } + } + sp suspendService; int stateFd; int wakeupCountFd; @@ -162,14 +176,7 @@ TEST_F(SystemSuspendTest, OnlyOneEnableAutosuspend) { } TEST_F(SystemSuspendTest, AutosuspendLoop) { - for (int i = 0; i < 2; i++) { - // Mock value for /sys/power/wakeup_count. - std::string wakeupCount = std::to_string(rand()); - ASSERT_TRUE(WriteStringToFd(wakeupCount, wakeupCountFd)); - ASSERT_EQ(readFd(wakeupCountFd), wakeupCount) - << "wakeup count value written by SystemSuspend is not equal to value given to it"; - ASSERT_EQ(readFd(stateFd), "mem") << "SystemSuspend failed to write correct sleep state."; - } + checkLoop(5); } // Tests that upon WakeLock destruction SystemSuspend HAL is unblocked. @@ -270,11 +277,66 @@ TEST_F(SystemSuspendTest, WakeLockStressTest) { ASSERT_EQ(getWakeLockCount(), 0); } +// Callbacks are passed around as sp<>. However, mock expectations are verified when mock objects +// are destroyed, i.e. the test needs to control lifetime of the mock object. +// MockCallbackImpl can be destroyed independently of its wrapper MockCallback which is passed to +// SystemSuspend. +struct MockCallbackImpl { + MOCK_METHOD1(notifyWakeup, Return(bool)); +}; + +class MockCallback : public ISystemSuspendCallback { + public: + MockCallback(MockCallbackImpl* impl) : mImpl(impl), mDisabled(false) {} + Return notifyWakeup(bool x) { return mDisabled ? Void() : mImpl->notifyWakeup(x); } + // In case we pull the rug from under MockCallback, but SystemSuspend still has an sp<> to the + // object. + void disable() { mDisabled = true; } + + private: + MockCallbackImpl* mImpl; + bool mDisabled; +}; + +// Tests that nullptr can't be registered as callbacks. +TEST_F(SystemSuspendTest, RegisterInvalidCallback) { + ASSERT_FALSE(suspendService->registerCallback(nullptr)); +} + +// Tests that SystemSuspend HAL correctly notifies wakeup events. +TEST_F(SystemSuspendTest, CallbackNotifyWakeup) { + constexpr int numWakeups = 5; + MockCallbackImpl impl; + // SystemSuspend should suspend numWakeup + 1 times. However, it might + // only be able to notify numWakeup times. The test case might have + // finished by the time last notification completes. + EXPECT_CALL(impl, notifyWakeup).Times(testing::AtLeast(numWakeups)); + sp cb = new MockCallback(&impl); + ASSERT_TRUE(suspendService->registerCallback(cb)); + checkLoop(numWakeups + 1); + cb->disable(); +} + +// Tests that SystemSuspend HAL correctly deals with a dead callback. +TEST_F(SystemSuspendTest, DeadCallback) { + ASSERT_EXIT( + { + sp cb = new MockCallback(nullptr); + ASSERT_TRUE(suspendService->registerCallback(cb)); + std::exit(0); + }, + ::testing::ExitedWithCode(0), ""); + + // Dead process callback must still be dealt with either by unregistering it + // or checking isOk() on every call. + checkLoop(3); +} } // namespace android int main(int argc, char** argv) { setenv("TREBLE_TESTING_OVERRIDE", "true", true); ::testing::AddGlobalTestEnvironment(android::SystemSuspendTestEnvironment::Instance()); + ::testing::InitGoogleMock(&argc, argv); ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); } -- 2.11.0