* [PATCH] coreaudio: Lock only the buffer
@ 2021-06-22 1:50 Akihiko Odaki
2021-06-22 6:57 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 2+ messages in thread
From: Akihiko Odaki @ 2021-06-22 1:50 UTC (permalink / raw)
Cc: qemu-devel, Akihiko Odaki, Gerd Hoffmann
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"
@@ -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)
--
2.30.1 (Apple Git-130)
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] coreaudio: Lock only the buffer
2021-06-22 1:50 [PATCH] coreaudio: Lock only the buffer Akihiko Odaki
@ 2021-06-22 6:57 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 2+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-22 6:57 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: qemu-devel, Gerd Hoffmann
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)
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-22 6:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 1:50 [PATCH] coreaudio: Lock only the buffer Akihiko Odaki
2021-06-22 6:57 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.