diff --git a/src/ba-transport.c b/src/ba-transport.c index 7aba1af2b..97eccbc9c 100644 --- a/src/ba-transport.c +++ b/src/ba-transport.c @@ -94,6 +94,7 @@ static int transport_pcm_init( ba_transport_pcm_volume_set(&pcm->volume[1], NULL, NULL, NULL); pthread_mutex_init(&pcm->mutex, NULL); + pthread_mutex_init(&pcm->client_mtx, NULL); pthread_cond_init(&pcm->cond, NULL); pcm->ba_dbus_path = g_strdup_printf("%s/%s/%s", @@ -111,6 +112,7 @@ static void transport_pcm_free( pthread_mutex_unlock(&pcm->mutex); pthread_mutex_destroy(&pcm->mutex); + pthread_mutex_destroy(&pcm->client_mtx); pthread_cond_destroy(&pcm->cond); if (pcm->ba_dbus_path != NULL) @@ -150,6 +152,48 @@ void ba_transport_pcm_volume_set( } +static int ba_transport_pcms_full_lock(struct ba_transport *t) { + if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) { + /* lock client mutexes first to avoid deadlock */ + pthread_mutex_lock(&t->a2dp.pcm.client_mtx); + pthread_mutex_lock(&t->a2dp.pcm_bc.client_mtx); + /* lock PCM data mutexes */ + pthread_mutex_lock(&t->a2dp.pcm.mutex); + pthread_mutex_lock(&t->a2dp.pcm_bc.mutex); + return 0; + } + if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) { + /* lock client mutexes first to avoid deadlock */ + pthread_mutex_lock(&t->sco.spk_pcm.client_mtx); + pthread_mutex_lock(&t->sco.mic_pcm.client_mtx); + /* lock PCM data mutexes */ + pthread_mutex_lock(&t->sco.spk_pcm.mutex); + pthread_mutex_lock(&t->sco.mic_pcm.mutex); + return 0; + } + errno = EINVAL; + return -1; +} + +static int ba_transport_pcms_full_unlock(struct ba_transport *t) { + if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) { + pthread_mutex_unlock(&t->a2dp.pcm.mutex); + pthread_mutex_unlock(&t->a2dp.pcm_bc.mutex); + pthread_mutex_unlock(&t->a2dp.pcm.client_mtx); + pthread_mutex_unlock(&t->a2dp.pcm_bc.client_mtx); + return 0; + } + if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) { + pthread_mutex_unlock(&t->sco.spk_pcm.mutex); + pthread_mutex_unlock(&t->sco.mic_pcm.mutex); + pthread_mutex_unlock(&t->sco.spk_pcm.client_mtx); + pthread_mutex_unlock(&t->sco.mic_pcm.client_mtx); + return 0; + } + errno = EINVAL; + return -1; +} + static int transport_thread_init( struct ba_transport_thread *th, struct ba_transport *t) { @@ -392,9 +436,11 @@ static void transport_threads_cancel(struct ba_transport *t) { static void transport_threads_cancel_if_no_clients(struct ba_transport *t) { - /* Hold PCM locks, so no client will open a PCM in - * the middle of our PCM inactivity check. */ - ba_transport_pcms_lock(t); + /* Hold PCM client and data locks. The data lock is required because we + * are going to check the PCM FIFO file descriptor. The client lock is + * required to prevent PCM clients from opening PCM in the middle of our + * inactivity check. */ + ba_transport_pcms_full_lock(t); /* Hold BT lock, because we are going to modify * the IO transports stopping flag. */ @@ -428,7 +474,7 @@ static void transport_threads_cancel_if_no_clients(struct ba_transport *t) { final: pthread_mutex_unlock(&t->bt_fd_mtx); - ba_transport_pcms_unlock(t); + ba_transport_pcms_full_unlock(t); if (stop) { transport_threads_cancel(t); @@ -900,7 +946,7 @@ void ba_transport_destroy(struct ba_transport *t) { /* stop transport IO threads */ ba_transport_stop(t); - ba_transport_pcms_lock(t); + ba_transport_pcms_full_lock(t); /* terminate on-going PCM connections - exit PCM controllers */ if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) { @@ -915,7 +961,7 @@ void ba_transport_destroy(struct ba_transport *t) { /* make sure that transport is released */ ba_transport_release(t); - ba_transport_pcms_unlock(t); + ba_transport_pcms_full_unlock(t); ba_transport_unref(t); } @@ -992,36 +1038,6 @@ void ba_transport_pcm_unref(struct ba_transport_pcm *pcm) { ba_transport_unref(pcm->t); } -int ba_transport_pcms_lock(struct ba_transport *t) { - if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) { - pthread_mutex_lock(&t->a2dp.pcm.mutex); - pthread_mutex_lock(&t->a2dp.pcm_bc.mutex); - return 0; - } - if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) { - pthread_mutex_lock(&t->sco.spk_pcm.mutex); - pthread_mutex_lock(&t->sco.mic_pcm.mutex); - return 0; - } - errno = EINVAL; - return -1; -} - -int ba_transport_pcms_unlock(struct ba_transport *t) { - if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) { - pthread_mutex_unlock(&t->a2dp.pcm.mutex); - pthread_mutex_unlock(&t->a2dp.pcm_bc.mutex); - return 0; - } - if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) { - pthread_mutex_unlock(&t->sco.spk_pcm.mutex); - pthread_mutex_unlock(&t->sco.mic_pcm.mutex); - return 0; - } - errno = EINVAL; - return -1; -} - int ba_transport_select_codec_a2dp( struct ba_transport *t, const struct a2dp_sep *sep) { @@ -1096,11 +1112,11 @@ int ba_transport_select_codec_sco( /* stop transport IO threads */ ba_transport_stop(t); - ba_transport_pcms_lock(t); + ba_transport_pcms_full_lock(t); /* release ongoing PCM connections */ ba_transport_pcm_release(&t->sco.spk_pcm); ba_transport_pcm_release(&t->sco.mic_pcm); - ba_transport_pcms_unlock(t); + ba_transport_pcms_full_unlock(t); r->codec_selection_done = false; /* delegate set codec to RFCOMM thread */ diff --git a/src/ba-transport.h b/src/ba-transport.h index 34c371866..6d0023b50 100644 --- a/src/ba-transport.h +++ b/src/ba-transport.h @@ -104,6 +104,9 @@ struct ba_transport_pcm { double scale; } volume[2]; + /* new PCM client mutex */ + pthread_mutex_t client_mtx; + /* exported PCM D-Bus API */ char *ba_dbus_path; bool ba_dbus_exported; @@ -341,9 +344,6 @@ void ba_transport_unref(struct ba_transport *t); struct ba_transport_pcm *ba_transport_pcm_ref(struct ba_transport_pcm *pcm); void ba_transport_pcm_unref(struct ba_transport_pcm *pcm); -int ba_transport_pcms_lock(struct ba_transport *t); -int ba_transport_pcms_unlock(struct ba_transport *t); - int ba_transport_select_codec_a2dp( struct ba_transport *t, const struct a2dp_sep *sep); diff --git a/src/bluealsa-dbus.c b/src/bluealsa-dbus.c index edd014943..8a91d6d55 100644 --- a/src/bluealsa-dbus.c +++ b/src/bluealsa-dbus.c @@ -446,17 +446,25 @@ static void bluealsa_pcm_open(GDBusMethodInvocation *inv, void *userdata) { /* Prevent two (or more) clients trying to * open the same PCM at the same time. */ - pthread_mutex_lock(&pcm->mutex); + pthread_mutex_lock(&pcm->client_mtx); + + pthread_mutex_lock(&t->codec_id_mtx); + const uint16_t codec_id = t->codec_id; + pthread_mutex_unlock(&t->codec_id_mtx); /* preliminary check whether HFP codes is selected */ if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO && - t->codec_id == HFP_CODEC_UNDEFINED) { + codec_id == HFP_CODEC_UNDEFINED) { g_dbus_method_invocation_return_error(inv, G_DBUS_ERROR, G_DBUS_ERROR_FAILED, "HFP audio codec not selected"); goto fail; } - if (pcm->fd != -1) { + pthread_mutex_lock(&pcm->mutex); + const int pcm_fd = pcm->fd; + pthread_mutex_unlock(&pcm->mutex); + + if (pcm_fd != -1) { g_dbus_method_invocation_return_error(inv, G_DBUS_ERROR, G_DBUS_ERROR_FAILED, "%s", strerror(EBUSY)); goto fail; @@ -508,10 +516,12 @@ static void bluealsa_pcm_open(GDBusMethodInvocation *inv, void *userdata) { } + pthread_mutex_lock(&pcm->mutex); /* get correct PIPE endpoint - PIPE is unidirectional */ pcm->fd = pcm_fds[is_sink ? 0 : 1]; /* set newly opened PCM as active */ pcm->active = true; + pthread_mutex_unlock(&pcm->mutex); GIOChannel *ch = g_io_channel_unix_new(pcm_fds[2]); g_io_channel_set_close_on_unref(ch, TRUE); @@ -525,18 +535,17 @@ static void bluealsa_pcm_open(GDBusMethodInvocation *inv, void *userdata) { /* notify our audio thread that the FIFO is ready */ ba_transport_thread_signal_send(th, BA_TRANSPORT_THREAD_SIGNAL_PCM_OPEN); - pthread_mutex_unlock(&pcm->mutex); - int fds[2] = { pcm_fds[is_sink ? 1 : 0], pcm_fds[3] }; GUnixFDList *fd_list = g_unix_fd_list_new_from_array(fds, 2); g_dbus_method_invocation_return_value_with_unix_fd_list(inv, g_variant_new("(hh)", 0, 1), fd_list); g_object_unref(fd_list); + pthread_mutex_unlock(&pcm->client_mtx); return; fail: - pthread_mutex_unlock(&pcm->mutex); + pthread_mutex_unlock(&pcm->client_mtx); /* clean up created file descriptors */ for (i = 0; i < ARRAYSIZE(pcm_fds); i++) if (pcm_fds[i] != -1)