Skip to content

Commit 0959403

Browse files
arkqarkq
committed
Guard PCM access by a dedicated client mutex
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.
1 parent 7ae09a4 commit 0959403

File tree

3 files changed

+72
-47
lines changed

3 files changed

+72
-47
lines changed

‎src/ba-transport.c

+54-38
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ static int transport_pcm_init(
9494
ba_transport_pcm_volume_set(&pcm->volume[1], NULL, NULL, NULL);
9595

9696
pthread_mutex_init(&pcm->mutex, NULL);
97+
pthread_mutex_init(&pcm->client_mtx, NULL);
9798
pthread_cond_init(&pcm->cond, NULL);
9899

99100
pcm->ba_dbus_path = g_strdup_printf("%s/%s/%s",
@@ -111,6 +112,7 @@ static void transport_pcm_free(
111112
pthread_mutex_unlock(&pcm->mutex);
112113

113114
pthread_mutex_destroy(&pcm->mutex);
115+
pthread_mutex_destroy(&pcm->client_mtx);
114116
pthread_cond_destroy(&pcm->cond);
115117

116118
if (pcm->ba_dbus_path != NULL)
@@ -150,6 +152,48 @@ void ba_transport_pcm_volume_set(
150152

151153
}
152154

155+
static int ba_transport_pcms_full_lock(struct ba_transport *t) {
156+
if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) {
157+
/* lock client mutexes first to avoid deadlock */
158+
pthread_mutex_lock(&t->a2dp.pcm.client_mtx);
159+
pthread_mutex_lock(&t->a2dp.pcm_bc.client_mtx);
160+
/* lock PCM data mutexes */
161+
pthread_mutex_lock(&t->a2dp.pcm.mutex);
162+
pthread_mutex_lock(&t->a2dp.pcm_bc.mutex);
163+
return 0;
164+
}
165+
if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) {
166+
/* lock client mutexes first to avoid deadlock */
167+
pthread_mutex_lock(&t->sco.spk_pcm.client_mtx);
168+
pthread_mutex_lock(&t->sco.mic_pcm.client_mtx);
169+
/* lock PCM data mutexes */
170+
pthread_mutex_lock(&t->sco.spk_pcm.mutex);
171+
pthread_mutex_lock(&t->sco.mic_pcm.mutex);
172+
return 0;
173+
}
174+
errno = EINVAL;
175+
return -1;
176+
}
177+
178+
static int ba_transport_pcms_full_unlock(struct ba_transport *t) {
179+
if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) {
180+
pthread_mutex_unlock(&t->a2dp.pcm.mutex);
181+
pthread_mutex_unlock(&t->a2dp.pcm_bc.mutex);
182+
pthread_mutex_unlock(&t->a2dp.pcm.client_mtx);
183+
pthread_mutex_unlock(&t->a2dp.pcm_bc.client_mtx);
184+
return 0;
185+
}
186+
if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) {
187+
pthread_mutex_unlock(&t->sco.spk_pcm.mutex);
188+
pthread_mutex_unlock(&t->sco.mic_pcm.mutex);
189+
pthread_mutex_unlock(&t->sco.spk_pcm.client_mtx);
190+
pthread_mutex_unlock(&t->sco.mic_pcm.client_mtx);
191+
return 0;
192+
}
193+
errno = EINVAL;
194+
return -1;
195+
}
196+
153197
static int transport_thread_init(
154198
struct ba_transport_thread *th,
155199
struct ba_transport *t) {
@@ -392,9 +436,11 @@ static void transport_threads_cancel(struct ba_transport *t) {
392436

393437
static void transport_threads_cancel_if_no_clients(struct ba_transport *t) {
394438

395-
/* Hold PCM locks, so no client will open a PCM in
396-
* the middle of our PCM inactivity check. */
397-
ba_transport_pcms_lock(t);
439+
/* Hold PCM client and data locks. The data lock is required because we
440+
* are going to check the PCM FIFO file descriptor. The client lock is
441+
* required to prevent PCM clients from opening PCM in the middle of our
442+
* inactivity check. */
443+
ba_transport_pcms_full_lock(t);
398444

399445
/* Hold BT lock, because we are going to modify
400446
* the IO transports stopping flag. */
@@ -428,7 +474,7 @@ static void transport_threads_cancel_if_no_clients(struct ba_transport *t) {
428474

429475
final:
430476
pthread_mutex_unlock(&t->bt_fd_mtx);
431-
ba_transport_pcms_unlock(t);
477+
ba_transport_pcms_full_unlock(t);
432478

433479
if (stop) {
434480
transport_threads_cancel(t);
@@ -900,7 +946,7 @@ void ba_transport_destroy(struct ba_transport *t) {
900946
/* stop transport IO threads */
901947
ba_transport_stop(t);
902948

903-
ba_transport_pcms_lock(t);
949+
ba_transport_pcms_full_lock(t);
904950

905951
/* terminate on-going PCM connections - exit PCM controllers */
906952
if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) {
@@ -915,7 +961,7 @@ void ba_transport_destroy(struct ba_transport *t) {
915961
/* make sure that transport is released */
916962
ba_transport_release(t);
917963

918-
ba_transport_pcms_unlock(t);
964+
ba_transport_pcms_full_unlock(t);
919965

920966
ba_transport_unref(t);
921967
}
@@ -992,36 +1038,6 @@ void ba_transport_pcm_unref(struct ba_transport_pcm *pcm) {
9921038
ba_transport_unref(pcm->t);
9931039
}
9941040

995-
int ba_transport_pcms_lock(struct ba_transport *t) {
996-
if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) {
997-
pthread_mutex_lock(&t->a2dp.pcm.mutex);
998-
pthread_mutex_lock(&t->a2dp.pcm_bc.mutex);
999-
return 0;
1000-
}
1001-
if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) {
1002-
pthread_mutex_lock(&t->sco.spk_pcm.mutex);
1003-
pthread_mutex_lock(&t->sco.mic_pcm.mutex);
1004-
return 0;
1005-
}
1006-
errno = EINVAL;
1007-
return -1;
1008-
}
1009-
1010-
int ba_transport_pcms_unlock(struct ba_transport *t) {
1011-
if (t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP) {
1012-
pthread_mutex_unlock(&t->a2dp.pcm.mutex);
1013-
pthread_mutex_unlock(&t->a2dp.pcm_bc.mutex);
1014-
return 0;
1015-
}
1016-
if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO) {
1017-
pthread_mutex_unlock(&t->sco.spk_pcm.mutex);
1018-
pthread_mutex_unlock(&t->sco.mic_pcm.mutex);
1019-
return 0;
1020-
}
1021-
errno = EINVAL;
1022-
return -1;
1023-
}
1024-
10251041
int ba_transport_select_codec_a2dp(
10261042
struct ba_transport *t,
10271043
const struct a2dp_sep *sep) {
@@ -1096,11 +1112,11 @@ int ba_transport_select_codec_sco(
10961112
/* stop transport IO threads */
10971113
ba_transport_stop(t);
10981114

1099-
ba_transport_pcms_lock(t);
1115+
ba_transport_pcms_full_lock(t);
11001116
/* release ongoing PCM connections */
11011117
ba_transport_pcm_release(&t->sco.spk_pcm);
11021118
ba_transport_pcm_release(&t->sco.mic_pcm);
1103-
ba_transport_pcms_unlock(t);
1119+
ba_transport_pcms_full_unlock(t);
11041120

11051121
r->codec_selection_done = false;
11061122
/* delegate set codec to RFCOMM thread */

‎src/ba-transport.h

+3-3
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,9 @@ struct ba_transport_pcm {
104104
double scale;
105105
} volume[2];
106106

107+
/* new PCM client mutex */
108+
pthread_mutex_t client_mtx;
109+
107110
/* exported PCM D-Bus API */
108111
char *ba_dbus_path;
109112
bool ba_dbus_exported;
@@ -341,9 +344,6 @@ void ba_transport_unref(struct ba_transport *t);
341344
struct ba_transport_pcm *ba_transport_pcm_ref(struct ba_transport_pcm *pcm);
342345
void ba_transport_pcm_unref(struct ba_transport_pcm *pcm);
343346

344-
int ba_transport_pcms_lock(struct ba_transport *t);
345-
int ba_transport_pcms_unlock(struct ba_transport *t);
346-
347347
int ba_transport_select_codec_a2dp(
348348
struct ba_transport *t,
349349
const struct a2dp_sep *sep);

‎src/bluealsa-dbus.c

+15-6
Original file line numberDiff line numberDiff line change
@@ -446,17 +446,25 @@ static void bluealsa_pcm_open(GDBusMethodInvocation *inv, void *userdata) {
446446

447447
/* Prevent two (or more) clients trying to
448448
* open the same PCM at the same time. */
449-
pthread_mutex_lock(&pcm->mutex);
449+
pthread_mutex_lock(&pcm->client_mtx);
450+
451+
pthread_mutex_lock(&t->codec_id_mtx);
452+
const uint16_t codec_id = t->codec_id;
453+
pthread_mutex_unlock(&t->codec_id_mtx);
450454

451455
/* preliminary check whether HFP codes is selected */
452456
if (t->profile & BA_TRANSPORT_PROFILE_MASK_SCO &&
453-
t->codec_id == HFP_CODEC_UNDEFINED) {
457+
codec_id == HFP_CODEC_UNDEFINED) {
454458
g_dbus_method_invocation_return_error(inv, G_DBUS_ERROR,
455459
G_DBUS_ERROR_FAILED, "HFP audio codec not selected");
456460
goto fail;
457461
}
458462

459-
if (pcm->fd != -1) {
463+
pthread_mutex_lock(&pcm->mutex);
464+
const int pcm_fd = pcm->fd;
465+
pthread_mutex_unlock(&pcm->mutex);
466+
467+
if (pcm_fd != -1) {
460468
g_dbus_method_invocation_return_error(inv, G_DBUS_ERROR,
461469
G_DBUS_ERROR_FAILED, "%s", strerror(EBUSY));
462470
goto fail;
@@ -508,10 +516,12 @@ static void bluealsa_pcm_open(GDBusMethodInvocation *inv, void *userdata) {
508516

509517
}
510518

519+
pthread_mutex_lock(&pcm->mutex);
511520
/* get correct PIPE endpoint - PIPE is unidirectional */
512521
pcm->fd = pcm_fds[is_sink ? 0 : 1];
513522
/* set newly opened PCM as active */
514523
pcm->active = true;
524+
pthread_mutex_unlock(&pcm->mutex);
515525

516526
GIOChannel *ch = g_io_channel_unix_new(pcm_fds[2]);
517527
g_io_channel_set_close_on_unref(ch, TRUE);
@@ -525,18 +535,17 @@ static void bluealsa_pcm_open(GDBusMethodInvocation *inv, void *userdata) {
525535
/* notify our audio thread that the FIFO is ready */
526536
ba_transport_thread_signal_send(th, BA_TRANSPORT_THREAD_SIGNAL_PCM_OPEN);
527537

528-
pthread_mutex_unlock(&pcm->mutex);
529-
530538
int fds[2] = { pcm_fds[is_sink ? 1 : 0], pcm_fds[3] };
531539
GUnixFDList *fd_list = g_unix_fd_list_new_from_array(fds, 2);
532540
g_dbus_method_invocation_return_value_with_unix_fd_list(inv,
533541
g_variant_new("(hh)", 0, 1), fd_list);
534542
g_object_unref(fd_list);
535543

544+
pthread_mutex_unlock(&pcm->client_mtx);
536545
return;
537546

538547
fail:
539-
pthread_mutex_unlock(&pcm->mutex);
548+
pthread_mutex_unlock(&pcm->client_mtx);
540549
/* clean up created file descriptors */
541550
for (i = 0; i < ARRAYSIZE(pcm_fds); i++)
542551
if (pcm_fds[i] != -1)

0 commit comments

Comments
 (0)