Skip to content

Commit

Permalink
Guard PCM access by a dedicated client mutex
Browse files Browse the repository at this point in the history 
Reusing PCM internal mutex for clients synchronization causes troubles
with transport termination - transport thread termination might get
stuck on mutex causing deadlock. Internal PCM mutex should not be held
for a long period of time. It is used every time the PCM FIFO file
descriptor is accessed - which happens a lot in a transport IO thread.
  • Loading branch information
@arkq
arkq committed Nov 26, 2022
1 parent 7ae09a4 commit 0959403
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 47 deletions.
92 changes: 54 additions & 38 deletions src/ba-transport.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 */
Expand Down
6 changes: 3 additions & 3 deletions src/ba-transport.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 15 additions & 6 deletions src/bluealsa-dbus.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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)
Expand Down

0 comments on commit 0959403

Please sign in to comment.