Re: [PATCH] coreaudio: Lock only the buffer

[Philippe Mathieu-Daudé]

On 6/22/21 3:50 AM, Akihiko Odaki wrote:
> On macOS 11.3.1, Core Audio calls AudioDeviceIOProc after calling an
> internal function named HALB_Mutex::Lock(), which locks a mutex in
> HALB_IOThread::Entry(void*). HALB_Mutex::Lock() is also called in
> AudioObjectGetPropertyData, which is called by coreaudio driver.
> Therefore, a deadlock will occur if coreaudio driver calls
> AudioObjectGetPropertyData while holding a lock for a mutex and tries
> to lock the same mutex in AudioDeviceIOProc.
> 
> audioDeviceIOProc, which implements AudioDeviceIOProc in coreaudio
> driver, requires an exclusive access for the device configuration and
> the buffer. Fortunately, a mutex is necessary only for the buffer in
> audioDeviceIOProc because a change for the device configuration occurs
> only before setting up AudioDeviceIOProc or after stopping the playback
> with AudioDeviceStop.
> 
> With this change, the mutex owned by the driver will only be used for
> the buffer, and the device configuration change will be protected with
> the implicit iothread mutex.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> ---
>  audio/coreaudio.c | 59 +++++++++++------------------------------------
>  1 file changed, 13 insertions(+), 46 deletions(-)
> 
> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
> index 578ec9b8b2e..c8d9f01d275 100644
> --- a/audio/coreaudio.c
> +++ b/audio/coreaudio.c
> @@ -26,6 +26,7 @@
>  #include <CoreAudio/CoreAudio.h>
>  #include <pthread.h>            /* pthread_X */
>  
> +#include "qemu/main-loop.h"
>  #include "qemu/module.h"
>  #include "audio.h"
>  

Suggestions to complete your patch, document init_out_device()
and update_device_playback_state() are called with iothread
lock taken:

@@ -456,6 +456,7 @@ static OSStatus init_out_device(coreaudioVoiceOut *core)
     return 0;
 }

+/* Called with iothread lock taken. */
 static void fini_out_device(coreaudioVoiceOut *core)
 {
     OSStatus status;
@@ -486,6 +487,7 @@ static void fini_out_device(coreaudioVoiceOut *core)
     core->outputDeviceID = kAudioDeviceUnknown;
 }

+/* Called with iothread lock taken. */
 static void update_device_playback_state(coreaudioVoiceOut *core)
 {
     OSStatus status;

> @@ -551,9 +552,7 @@ static OSStatus handle_voice_change(
>      OSStatus status;
>      coreaudioVoiceOut *core = in_client_data;
>  
> -    if (coreaudio_lock(core, __func__)) {
> -        abort();
> -    }
> +    qemu_mutex_lock_iothread();
>  
>      if (core->outputDeviceID) {
>          fini_out_device(core);
> @@ -564,7 +563,7 @@ static OSStatus handle_voice_change(
>          update_device_playback_state(core);
>      }
>  
> -    coreaudio_unlock (core, __func__);
> +    qemu_mutex_unlock_iothread();
>      return status;
>  }
>  
> @@ -582,11 +581,7 @@ static int coreaudio_init_out(HWVoiceOut *hw, struct 
> audsettings *as,
>      err = pthread_mutex_init(&core->mutex, NULL);
>      if (err) {
>          dolog("Could not create mutex\nReason: %s\n", strerror (err));
> -        goto mutex_error;
> -    }
> -
> -    if (coreaudio_lock(core, __func__)) {
> -        goto lock_error;
> +        return -1;
>      }
>  
>      obt_as = *as;
> @@ -606,37 +601,21 @@ static int coreaudio_init_out(HWVoiceOut *hw, struct 
> audsettings *as,
>      if (status != kAudioHardwareNoError) {
>          coreaudio_playback_logerr (status,
>                                     "Could not listen to voice property 
> change\n");
> -        goto listener_error;
> +        return -1;
>      }
>  
>      if (init_out_device(core)) {
> -        goto device_error;
> +        status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
> +                                                   &voice_addr,
> +                                                   handle_voice_change,
> +                                                   core);
> +        if (status != kAudioHardwareNoError) {
> +            coreaudio_playback_logerr(status,
> +                                      "Could not remove voice property 
> change listener\n");
> +        }
>      }
>  
> -    coreaudio_unlock(core, __func__);
>      return 0;
> -
> -device_error:
> -    status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
> -                                               &voice_addr,
> -                                               handle_voice_change,
> -                                               core);
> -    if (status != kAudioHardwareNoError) {
> -        coreaudio_playback_logerr(status,
> -                                  "Could not remove voice property change 
> listener\n");
> -    }
> -
> -listener_error:
> -    coreaudio_unlock(core, __func__);
> -
> -lock_error:
> -    err = pthread_mutex_destroy(&core->mutex);
> -    if (err) {
> -        dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
> -    }
> -
> -mutex_error:
> -    return -1;
>  }
>  
>  static void coreaudio_fini_out (HWVoiceOut *hw)
> @@ -645,10 +624,6 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
>      int err;
>      coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
>  
> -    if (coreaudio_lock(core, __func__)) {
> -        abort();
> -    }
> -
>      status = AudioObjectRemovePropertyListener(kAudioObjectSystemObject,
>                                                 &voice_addr,
>                                                 handle_voice_change,
> @@ -659,8 +634,6 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
>  
>      fini_out_device(core);
>  
> -    coreaudio_unlock(core, __func__);
> -
>      /* destroy mutex */
>      err = pthread_mutex_destroy(&core->mutex);
>      if (err) {
> @@ -672,14 +645,8 @@ static void coreaudio_enable_out(HWVoiceOut *hw, bool 
> enable)
>  {
>      coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
>  
> -    if (coreaudio_lock(core, __func__)) {
> -        abort();
> -    }
> -
>      core->enabled = enable;
>      update_device_playback_state(core);
> -
> -    coreaudio_unlock(core, __func__);
>  }
>  
>  static void *coreaudio_audio_init(Audiodev *dev)
>