Graceful exit for StHalLpmaHandler
In previous implementation, we rely on mCurrentLpmaEnbled==mTargetLpmaEnabled to stuck in wait and detach the thread. This could cause issue when StHalLpmaHandler has been destructed but the working thread is not in wait condition, causing the detached thread accessing a destructed variable. Current implementation added terminate condition in the worker thread and wait for it to join before destructing the main object.
Test: Tested using chre_power_test_client to enable/disable audio data on flame couple of times and log looks normal. Also did pkill chre and no error message occurs.
Fixes: 241015054
Change-Id: Ibb737b58983f8f4b817222b4a78841282dafaf12
diff --git a/Android.bp b/Android.bp
index 59b8a5a..cfc855f 100644
--- a/Android.bp
+++ b/Android.bp
@@ -695,6 +695,7 @@
"pw_varint",
],
shared_libs: [
+ "libhidlbase",
"libjsoncpp",
"libpower",
"android.hardware.soundtrigger@2.0",
diff --git a/host/common/include/chre_host/st_hal_lpma_handler.h b/host/common/include/chre_host/st_hal_lpma_handler.h
index c2bbdee..289542f 100644
--- a/host/common/include/chre_host/st_hal_lpma_handler.h
+++ b/host/common/include/chre_host/st_hal_lpma_handler.h
@@ -50,12 +50,7 @@
StHalLpmaHandler() = delete;
explicit StHalLpmaHandler(bool allowed);
- ~StHalLpmaHandler() {
- if (mThread.has_value()) {
- // TODO(b/241015054): Change this to join after adding proper handler
- mThread->detach();
- }
- }
+ ~StHalLpmaHandler();
/**
* If LPMA is enabled, starts a worker thread to load/unload models.
@@ -96,6 +91,8 @@
bool mCurrentLpmaEnabled;
bool mTargetLpmaEnabled;
bool mCondVarPredicate;
+ bool mStThreadShouldExit = false;
+
SoundModelHandle mLpmaHandle = 0;
int mRetryCount;
@@ -165,22 +162,13 @@
void onStHalServiceDeath();
/**
- * This function blocks on a condition variable and when notified, based
- * on its current state and as notified by enable(), performs a load or
- * unload. The function also resets the delay and retry counts if the current
- * and next states match
+ * Invoke Hal to load or unload LPMA depending on the status of
+ * mTargetLpmaEnabled and mCurrentLpmaEnabled
*
- * @return true if the state update succeeded, and we don't need to retry with
- * a delay
+ * @param locked lock that holds the mutex that guards mTargetLpmaEnabled
*/
- bool waitOnStHalRequestAndProcess();
-
- /**
- * Delay retrying a load if a state update failed
- */
- void delay();
+ void stHalRequestAndProcessLocked(std::unique_lock<std::mutex> const &locked);
};
-
} // namespace chre
} // namespace android
diff --git a/host/common/st_hal_lpma_handler.cc b/host/common/st_hal_lpma_handler.cc
index 4f072b7..86452bb 100644
--- a/host/common/st_hal_lpma_handler.cc
+++ b/host/common/st_hal_lpma_handler.cc
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+#include <cassert>
#include <cinttypes>
#include "chre_host/st_hal_lpma_handler.h"
@@ -55,6 +56,18 @@
mDeathRecipient = new StHalDeathRecipient(cb);
}
+StHalLpmaHandler::~StHalLpmaHandler() {
+ if (mTargetLpmaEnabled) {
+ stopAndUnload();
+ }
+ if (mThread.has_value()) {
+ mStThreadShouldExit = true;
+ mCondVar.notify_all();
+ mThread->join();
+ }
+ releaseWakeLock();
+}
+
void StHalLpmaHandler::init() {
if (mIsLpmaAllowed) {
mThread = std::thread(&StHalLpmaHandler::stHalLpmaHandlerThreadEntry, this);
@@ -182,17 +195,16 @@
}
}
-bool StHalLpmaHandler::waitOnStHalRequestAndProcess() {
- bool noDelayNeeded = true;
- std::unique_lock<std::mutex> lock(mMutex);
-
+void StHalLpmaHandler::stHalRequestAndProcessLocked(
+ std::unique_lock<std::mutex> const &locked) {
+ // Cannot use assert(locked.owns_lock()) since locked are not use in other
+ // places and non-debug version will not use assert. Compiler will not compile
+ // if there is unused parameter.
+ if (!locked.owns_lock()) {
+ assert(false);
+ }
if (mCurrentLpmaEnabled == mTargetLpmaEnabled) {
- mRetryDelay = 0;
- mRetryCount = 0;
- releaseWakeLock(); // Allow the system to suspend while waiting.
- mCondVar.wait(lock, [this] { return mCondVarPredicate; });
- mCondVarPredicate = false;
- acquireWakeLock(); // Ensure the system stays up while retrying.
+ return;
} else if (mTargetLpmaEnabled && loadAndStart()) {
mCurrentLpmaEnabled = mTargetLpmaEnabled;
} else if (!mTargetLpmaEnabled) {
@@ -202,38 +214,38 @@
// supplied handle is invalid and should not be unloaded again.
stopAndUnload();
mCurrentLpmaEnabled = mTargetLpmaEnabled;
- } else {
- noDelayNeeded = false;
- }
-
- return noDelayNeeded;
-}
-
-void StHalLpmaHandler::delay() {
- constexpr useconds_t kInitialRetryDelayUs = 500000;
- constexpr int kRetryGrowthFactor = 2;
- constexpr int kRetryGrowthLimit = 5; // Terminates at 8s retry interval.
- constexpr int kRetryWakeLockLimit = 10; // Retry with a wakelock 10 times.
-
- if (mRetryDelay == 0) {
- mRetryDelay = kInitialRetryDelayUs;
- } else if (mRetryCount < kRetryGrowthLimit) {
- mRetryDelay *= kRetryGrowthFactor;
- }
- usleep(mRetryDelay);
- mRetryCount++;
- if (mRetryCount > kRetryWakeLockLimit) {
- releaseWakeLock();
}
}
void StHalLpmaHandler::stHalLpmaHandlerThreadEntry() {
LOGD("Starting LPMA thread");
+ constexpr useconds_t kInitialRetryDelayUs = 500000;
+ constexpr int kRetryGrowthFactor = 2;
+ constexpr int kRetryGrowthLimit = 5; // Terminates at 8s retry interval.
+ constexpr int kRetryWakeLockLimit = 10; // Retry with a wakelock 10 times.
+ std::unique_lock<std::mutex> lock(mMutex);
+ while (!mStThreadShouldExit) {
+ stHalRequestAndProcessLocked(lock);
+ bool retryNeeded = (mCurrentLpmaEnabled != mTargetLpmaEnabled);
+ releaseWakeLock();
- while (true) {
- if (!waitOnStHalRequestAndProcess()) {
- // processing an LPMA state update failed, retry after a little while
- delay();
+ if (retryNeeded) {
+ if (mRetryCount < kRetryGrowthLimit) {
+ mRetryCount += 1;
+ mRetryDelay = mRetryDelay * kRetryGrowthFactor;
+ }
+ mCondVar.wait_for(lock, std::chrono::microseconds(mRetryDelay), [this] {
+ return mCondVarPredicate || mStThreadShouldExit;
+ });
+ } else {
+ mRetryCount = 0;
+ mRetryDelay = kInitialRetryDelayUs;
+ mCondVar.wait(
+ lock, [this] { return mCondVarPredicate || mStThreadShouldExit; });
+ }
+ mCondVarPredicate = false;
+ if (mRetryCount <= kRetryWakeLockLimit) {
+ acquireWakeLock();
}
}
}
