commit fce3b6c94d9f37795a8f82b7cf0af7461a018fee
author François Gaffie <francois.gaffie@renault.com> Thu May 23 13:52:23 2024 +0200
committer Shunkai Yao <yaoshunkai@google.com> Fri Jul 12 18:28:39 2024 +0000
tree 41a976771ed7b8811cdcfd6be9187a04f4b9bfe9
parent 6e7873a71781eb11289ddcb8e4dddab0012a1ca8

AudioFlinger: fix regression on device effect introduced locking order

The lock order has been enforced within AudioFlinger.
Order has been defined. However, the Device Effect does not follow
the same pattern as session effects.

Session:
Client -> EffectHandle -> ThreadBase_Mutex (or AudioFlinger Mutex)
Device:
(Default effect)
AudioFlinger -> DeviceEffectManager -> DeviceEffectProxy -> DeviceEffectHandle
(added by Fx App)
Client -> EffectHandle -> DeviceEffectProxy -> DeviceEffectHandle

-Can be addded automatically by AudioFlinger upon patch creation
-May be Added to Audio IDevice
-Can be enabled / disabled by AudioFx app via DeviceEffectProxy.
-Use internal device effect handle that are now controlled by application
diretly, preventing from risk of concurrent access (e.g. disconnect /
enabled status)

This CL intends to fix the locking order assertions by
-introducing a new lock for internal device effect Handle
-unlocking temporarily to prevent breaking the lock order.

Flag: EXEMPT bugfix
Bug: 329395147
Bug: 348986455
Test: atest CtsMediaAudioTestCases

Change-Id: I6667c3ef1b77708ccb4d644fd1b53a2fe3896b72
Signed-off-by: François Gaffie <francois.gaffie@renault.com>

services/audioflinger/Effects.cpp

diff --git a/services/audioflinger/Effects.cpp b/services/audioflinger/Effects.cpp
index 973b2ab..0405035 100644
--- a/services/audioflinger/Effects.cpp
+++ b/services/audioflinger/Effects.cpp
@@ -1760,6 +1760,9 @@
         const sp<media::IEffectClient>& effectClient,
         int32_t priority, bool notifyFramesProcessed)
 {
+    if (client == nullptr && effectClient == nullptr) {
+        return sp<InternalEffectHandle>::make(effect, notifyFramesProcessed);
+    }
     return sp<EffectHandle>::make(
             effect, client, effectClient, priority, notifyFramesProcessed);
 }
@@ -1767,12 +1770,14 @@
 EffectHandle::EffectHandle(const sp<IAfEffectBase>& effect,
                                          const sp<Client>& client,
                                          const sp<media::IEffectClient>& effectClient,
-                                         int32_t priority, bool notifyFramesProcessed)
-    : BnEffect(),
+                                         int32_t priority, bool notifyFramesProcessed,
+                                         bool isInternal,
+                                         audio_utils::MutexOrder mutexOrder)
+    : BnEffect(), mMutex(mutexOrder),
     mEffect(effect), mEffectClient(media::EffectClientAsyncProxy::makeIfNeeded(effectClient)),
     mClient(client), mCblk(nullptr),
     mPriority(priority), mHasControl(false), mEnabled(false), mDisconnected(false),
-    mNotifyFramesProcessed(notifyFramesProcessed)
+    mNotifyFramesProcessed(notifyFramesProcessed), mIsInternal(isInternal)
 {
     ALOGV("constructor %p client %p", this, client.get());
     setMinSchedulerPolicy(SCHED_NORMAL, ANDROID_PRIORITY_AUDIO);
@@ -1939,7 +1944,7 @@
 
 void EffectHandle::disconnect(bool unpinIfLast)
 {
-    audio_utils::lock_guard _l(mutex());
+    audio_utils::unique_lock _l(mutex());
     ALOGV("disconnect(%s) %p", unpinIfLast ? "true" : "false", this);
     if (mDisconnected) {
         if (unpinIfLast) {
@@ -1951,11 +1956,19 @@
     {
         sp<IAfEffectBase> effect = mEffect.promote();
         if (effect != 0) {
+            // Unlock e.g. for device effect: may need to acquire AudioFlinger lock
+            // Also Internal Effect Handle would require Proxy lock (and vice versa).
+            if (isInternal()) {
+                _l.unlock();
+            }
             if (effect->disconnectHandle(this, unpinIfLast) > 0) {
                 ALOGW("%s Effect handle %p disconnected after thread destruction",
                     __func__, this);
             }
             effect->updatePolicyState();
+            if (isInternal()) {
+                _l.lock();
+            }
         }
     }
 
@@ -3544,8 +3557,7 @@
             }
             mHalEffect->configure_l();
         }
-        *handle = new EffectHandle(mHalEffect, nullptr, nullptr, 0 /*priority*/,
-                                   mNotifyFramesProcessed);
+        *handle = sp<InternalEffectHandle>::make(mHalEffect, mNotifyFramesProcessed);
         status = (*handle)->initCheck();
         if (status == OK) {
             status = mHalEffect->addHandle((*handle).get());
@@ -3592,15 +3604,16 @@
     return status;
 }
 
-void DeviceEffectProxy::onReleasePatch(audio_patch_handle_t patchHandle) {
-    sp<IAfEffectHandle> effect;
+sp<IAfEffectHandle> DeviceEffectProxy::onReleasePatch(audio_patch_handle_t patchHandle) {
+    sp<IAfEffectHandle> disconnectedHandle;
     {
         audio_utils::lock_guard _l(proxyMutex());
         if (mEffectHandles.find(patchHandle) != mEffectHandles.end()) {
-            effect = mEffectHandles.at(patchHandle);
+            disconnectedHandle = std::move(mEffectHandles.at(patchHandle));
             mEffectHandles.erase(patchHandle);
         }
     }
+    return disconnectedHandle;
 }