From 5d77c7290c9eec8e4ec6300ce9fb783ee8d36a40 Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Wed, 29 Nov 2017 12:28:00 -0800 Subject: [PATCH] PowerManagerService: Do not hold lock for the duration of PowerHal call Make getPowerHal return a local copy of sp which can be used to do PowerHAL calls without holding gPowerHal mutex. Bug: 69475681 Test: Do powerhint with loop of adb shell "setprop ctl.restart POWERHAL_SERVICE" Change-Id: I8d538de11a5b2347b554b64cc26aad2f3ce5d801 --- ..._android_server_am_BatteryStatsService.cpp | 70 ++++------- ...droid_server_power_PowerManagerService.cpp | 116 ++++++++++-------- 2 files changed, 94 insertions(+), 92 deletions(-) diff --git a/services/core/jni/com_android_server_am_BatteryStatsService.cpp b/services/core/jni/com_android_server_am_BatteryStatsService.cpp index ae7d6daa649b0..39cc95368d716 100644 --- a/services/core/jni/com_android_server_am_BatteryStatsService.cpp +++ b/services/core/jni/com_android_server_am_BatteryStatsService.cpp @@ -43,13 +43,14 @@ using android::hardware::Return; using android::hardware::Void; -using android::hardware::power::V1_0::IPower; using android::hardware::power::V1_0::PowerStatePlatformSleepState; using android::hardware::power::V1_0::PowerStateVoter; using android::hardware::power::V1_0::Status; using android::hardware::power::V1_1::PowerStateSubsystem; using android::hardware::power::V1_1::PowerStateSubsystemSleepState; using android::hardware::hidl_vec; +using IPowerV1_1 = android::hardware::power::V1_1::IPower; +using IPowerV1_0 = android::hardware::power::V1_0::IPower; namespace android { @@ -59,9 +60,9 @@ namespace android static bool wakeup_init = false; static sem_t wakeup_sem; -extern sp gPowerHalV1_0; -extern std::mutex gPowerHalMutex; -extern bool getPowerHal(); +extern sp getPowerHalV1_0(); +extern sp getPowerHalV1_1(); +extern bool processPowerHalReturn(const Return &ret, const char* functionName); // Java methods used in getLowPowerStats static jmethodID jgetAndUpdatePlatformState = NULL; @@ -203,13 +204,13 @@ static void getLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject jrpmStats return; } - std::lock_guard lock(gPowerHalMutex); - if (!getPowerHal()) { + sp powerHalV1_0 = getPowerHalV1_0(); + if (powerHalV1_0 == nullptr) { ALOGE("Power Hal not loaded"); return; } - Return ret = gPowerHalV1_0->getPlatformLowPowerStats( + Return ret = powerHalV1_0->getPlatformLowPowerStats( [&env, &jrpmStats](hidl_vec states, Status status) { if (status != Status::SUCCESS) return; @@ -236,20 +237,17 @@ static void getLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject jrpmStats } } }); - if (!ret.isOk()) { - ALOGE("getLowPowerStats() failed: power HAL service not available"); - gPowerHalV1_0 = nullptr; + if (!processPowerHalReturn(ret, "getPlatformLowPowerStats")) { return; } - //Trying to cast to IPower 1.1, this will succeed only for devices supporting 1.1 - sp gPowerHal_1_1 - = android::hardware::power::V1_1::IPower::castFrom(gPowerHalV1_0); - if (gPowerHal_1_1 == nullptr) { - //This device does not support IPower@1.1, exiting gracefully + // Trying to get IPower 1.1, this will succeed only for devices supporting 1.1 + sp powerHal_1_1 = getPowerHalV1_1(); + if (powerHal_1_1 == nullptr) { + // This device does not support IPower@1.1, exiting gracefully return; } - ret = gPowerHal_1_1->getSubsystemLowPowerStats( + ret = powerHal_1_1->getSubsystemLowPowerStats( [&env, &jrpmStats](hidl_vec subsystems, Status status) { if (status != Status::SUCCESS) return; @@ -275,11 +273,7 @@ static void getLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject jrpmStats } } }); - if (!ret.isOk()) { - ALOGE("getSubsystemLowPowerStats() failed: power HAL service not available"); - gPowerHalV1_0 = nullptr; - } - // gPowerHalMutex released here + processPowerHalReturn(ret, "getSubsystemLowPowerStats"); } static jint getPlatformLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject outBuf) { @@ -294,13 +288,13 @@ static jint getPlatformLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject o } { - std::lock_guard lock(gPowerHalMutex); - if (!getPowerHal()) { + sp powerHalV1_0 = getPowerHalV1_0(); + if (powerHalV1_0 == nullptr) { ALOGE("Power Hal not loaded"); return -1; } - Return ret = gPowerHalV1_0->getPlatformLowPowerStats( + Return ret = powerHalV1_0->getPlatformLowPowerStats( [&offset, &remaining, &total_added](hidl_vec states, Status status) { if (status != Status::SUCCESS) @@ -352,9 +346,7 @@ static jint getPlatformLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject o } ); - if (!ret.isOk()) { - ALOGE("getPlatformLowPowerStats() failed: power HAL service not available"); - gPowerHalV1_0 = nullptr; + if (!processPowerHalReturn(ret, "getPlatformLowPowerStats")) { return -1; } } @@ -369,8 +361,8 @@ static jint getSubsystemLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject int remaining = (int)env->GetDirectBufferCapacity(outBuf); int total_added = -1; - //This is a IPower 1.1 API - sp gPowerHal_1_1 = nullptr; + // This is a IPower 1.1 API + sp powerHal_1_1 = nullptr; if (outBuf == NULL) { jniThrowException(env, "java/lang/NullPointerException", "null argument"); @@ -378,20 +370,14 @@ static jint getSubsystemLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject } { - std::lock_guard lock(gPowerHalMutex); - if (!getPowerHal()) { - ALOGE("Power Hal not loaded"); - return -1; - } - - //Trying to cast to 1.1, this will succeed only for devices supporting 1.1 - gPowerHal_1_1 = android::hardware::power::V1_1::IPower::castFrom(gPowerHalV1_0); - if (gPowerHal_1_1 == nullptr) { + // Trying to get 1.1, this will succeed only for devices supporting 1.1 + powerHal_1_1 = getPowerHalV1_1(); + if (powerHal_1_1 == nullptr) { //This device does not support IPower@1.1, exiting gracefully return 0; - } + } - Return ret = gPowerHal_1_1->getSubsystemLowPowerStats( + Return ret = powerHal_1_1->getSubsystemLowPowerStats( [&offset, &remaining, &total_added](hidl_vec subsystems, Status status) { @@ -452,9 +438,7 @@ static jint getSubsystemLowPowerStats(JNIEnv* env, jobject /* clazz */, jobject } ); - if (!ret.isOk()) { - ALOGE("getSubsystemLowPowerStats() failed: power HAL service not available"); - gPowerHalV1_0 = nullptr; + if (!processPowerHalReturn(ret, "getSubsystemLowPowerStats")) { return -1; } } diff --git a/services/core/jni/com_android_server_power_PowerManagerService.cpp b/services/core/jni/com_android_server_power_PowerManagerService.cpp index b6c3df707c310..b2d35d4153a05 100644 --- a/services/core/jni/com_android_server_power_PowerManagerService.cpp +++ b/services/core/jni/com_android_server_power_PowerManagerService.cpp @@ -41,10 +41,11 @@ using android::hardware::Return; using android::hardware::Void; -using android::hardware::power::V1_1::IPower; using android::hardware::power::V1_0::PowerHint; using android::hardware::power::V1_0::Feature; using android::String8; +using IPowerV1_1 = android::hardware::power::V1_1::IPower; +using IPowerV1_0 = android::hardware::power::V1_0::IPower; namespace android { @@ -57,10 +58,11 @@ static struct { // ---------------------------------------------------------------------------- static jobject gPowerManagerServiceObj; -sp gPowerHalV1_0 = nullptr; -sp gPowerHalV1_1 = nullptr; -bool gPowerHalExists = true; -std::mutex gPowerHalMutex; +// Use getPowerHal* to retrieve a copy +static sp gPowerHalV1_0_ = nullptr; +static sp gPowerHalV1_1_ = nullptr; +static bool gPowerHalExists = true; +static std::mutex gPowerHalMutex; static nsecs_t gLastEventTime[USER_ACTIVITY_EVENT_LAST + 1]; // Throttling interval for user activity calls. @@ -80,26 +82,63 @@ static bool checkAndClearExceptionFromCallback(JNIEnv* env, const char* methodNa // Check validity of current handle to the power HAL service, and call getService() if necessary. // The caller must be holding gPowerHalMutex. -bool getPowerHal() { - if (gPowerHalExists && gPowerHalV1_0 == nullptr) { - gPowerHalV1_0 = android::hardware::power::V1_0::IPower::getService(); - if (gPowerHalV1_0 != nullptr) { - gPowerHalV1_1 = android::hardware::power::V1_1::IPower::castFrom(gPowerHalV1_0); - ALOGI("Loaded power HAL service"); +static void connectPowerHalLocked() { + if (gPowerHalExists && gPowerHalV1_0_ == nullptr) { + gPowerHalV1_0_ = IPowerV1_0::getService(); + if (gPowerHalV1_0_ != nullptr) { + ALOGI("Loaded power HAL 1.0 service"); + // Try cast to powerHAL V1_1 + gPowerHalV1_1_ = IPowerV1_1::castFrom(gPowerHalV1_0_); + if (gPowerHalV1_1_ == nullptr) { + } else { + ALOGI("Loaded power HAL 1.1 service"); + } } else { ALOGI("Couldn't load power HAL service"); gPowerHalExists = false; } } - return gPowerHalV1_0 != nullptr; +} + +// Retrieve a copy of PowerHAL V1_0 +sp getPowerHalV1_0() { + std::lock_guard lock(gPowerHalMutex); + connectPowerHalLocked(); + return gPowerHalV1_0_; +} + +// Retrieve a copy of PowerHAL V1_1 +sp getPowerHalV1_1() { + std::lock_guard lock(gPowerHalMutex); + connectPowerHalLocked(); + return gPowerHalV1_1_; } // Check if a call to a power HAL function failed; if so, log the failure and invalidate the -// current handle to the power HAL service. The caller must be holding gPowerHalMutex. -static void processReturn(const Return &ret, const char* functionName) { +// current handle to the power HAL service. +bool processPowerHalReturn(const Return &ret, const char* functionName) { if (!ret.isOk()) { ALOGE("%s() failed: power HAL service not available.", functionName); - gPowerHalV1_0 = nullptr; + gPowerHalMutex.lock(); + gPowerHalV1_0_ = nullptr; + gPowerHalV1_1_ = nullptr; + gPowerHalMutex.unlock(); + } + return ret.isOk(); +} + +static void sendPowerHint(PowerHint hintId, uint32_t data) { + sp powerHalV1_1 = getPowerHalV1_1(); + Return ret; + if (powerHalV1_1 != nullptr) { + ret = powerHalV1_1->powerHintAsync(hintId, data); + processPowerHalReturn(ret, "powerHintAsync"); + } else { + sp powerHalV1_0 = getPowerHalV1_0(); + if (powerHalV1_0 != nullptr) { + ret = powerHalV1_0->powerHint(hintId, data); + processPowerHalReturn(ret, "powerHint"); + } } } @@ -119,20 +158,8 @@ void android_server_PowerManagerService_userActivity(nsecs_t eventTime, int32_t } gLastEventTime[eventType] = eventTime; - // Tell the power HAL when user activity occurs. - gPowerHalMutex.lock(); - if (getPowerHal()) { - Return ret; - if (gPowerHalV1_1 != nullptr) { - ret = gPowerHalV1_1->powerHintAsync(PowerHint::INTERACTION, 0); - } else { - ret = gPowerHalV1_0->powerHint(PowerHint::INTERACTION, 0); - } - processReturn(ret, "powerHint"); - } - gPowerHalMutex.unlock(); - + sendPowerHint(PowerHint::INTERACTION, 0); } JNIEnv* env = AndroidRuntime::getJNIEnv(); @@ -150,7 +177,7 @@ static void nativeInit(JNIEnv* env, jobject obj) { gPowerManagerServiceObj = env->NewGlobalRef(obj); gPowerHalMutex.lock(); - getPowerHal(); + connectPowerHalLocked(); gPowerHalMutex.unlock(); } @@ -165,11 +192,11 @@ static void nativeReleaseSuspendBlocker(JNIEnv *env, jclass /* clazz */, jstring } static void nativeSetInteractive(JNIEnv* /* env */, jclass /* clazz */, jboolean enable) { - std::lock_guard lock(gPowerHalMutex); - if (getPowerHal()) { + sp powerHalV1_0 = getPowerHalV1_0(); + if (powerHalV1_0 != nullptr) { android::base::Timer t; - Return ret = gPowerHalV1_0->setInteractive(enable); - processReturn(ret, "setInteractive"); + Return ret = powerHalV1_0->setInteractive(enable); + processPowerHalReturn(ret, "setInteractive"); if (t.duration() > 20ms) { ALOGD("Excessive delay in setInteractive(%s) while turning screen %s", enable ? "true" : "false", enable ? "on" : "off"); @@ -193,24 +220,15 @@ static void nativeSetAutoSuspend(JNIEnv* /* env */, jclass /* clazz */, jboolean } } -static void nativeSendPowerHint(JNIEnv *env, jclass clazz, jint hintId, jint data) { - std::lock_guard lock(gPowerHalMutex); - if (getPowerHal()) { - Return ret; - if (gPowerHalV1_1 != nullptr) { - ret = gPowerHalV1_1->powerHintAsync((PowerHint)hintId, data); - } else { - ret = gPowerHalV1_0->powerHint((PowerHint)hintId, data); - } - processReturn(ret, "powerHint"); - } +static void nativeSendPowerHint(JNIEnv* /* env */, jclass /* clazz */, jint hintId, jint data) { + sendPowerHint(static_cast(hintId), data); } -static void nativeSetFeature(JNIEnv *env, jclass clazz, jint featureId, jint data) { - std::lock_guard lock(gPowerHalMutex); - if (getPowerHal()) { - Return ret = gPowerHalV1_0->setFeature((Feature)featureId, static_cast(data)); - processReturn(ret, "setFeature"); +static void nativeSetFeature(JNIEnv* /* env */, jclass /* clazz */, jint featureId, jint data) { + sp powerHalV1_0 = getPowerHalV1_0(); + if (powerHalV1_0 != nullptr) { + Return ret = powerHalV1_0->setFeature((Feature)featureId, static_cast(data)); + processPowerHalReturn(ret, "setFeature"); } }