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.od...@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)
>
