From a3ae02d88411f1f7b3da9ef66065cb94c1bf1189 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Mon, 23 May 2005 08:56:48 +0000 Subject: [PATCH] Fix semaphore deadlocks - Fixed unbalanced semaphores (which may cause deadlock) - Do semaphore-up before blocking calls for communication with the server - Don't discard semaphores on the client side - Open slave PCMs always in non-blocking mode to avoid blocking by semaphore with the secondary open --- src/pcm/pcm_direct.c | 8 +++--- src/pcm/pcm_dmix.c | 60 +++++++++++++++++++---------------------- src/pcm/pcm_dshare.c | 64 +++++++++++++++++++++----------------------- src/pcm/pcm_dsnoop.c | 58 +++++++++++++++++++-------------------- 4 files changed, 88 insertions(+), 102 deletions(-) diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 14e4b8a5..a0f88da0 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -76,10 +76,8 @@ int snd_pcm_direct_semaphore_discard(snd_pcm_direct_t *dmix) int snd_pcm_direct_semaphore_down(snd_pcm_direct_t *dmix, int sem_num) { - struct sembuf op[2] = { { 0, 0, SEM_UNDO }, { 0, 1, SEM_UNDO | IPC_NOWAIT } }; + struct sembuf op[2] = { { sem_num, 0, 0 }, { sem_num, 1, SEM_UNDO } }; assert(dmix->semid >= 0); - op[0].sem_num = sem_num; - op[1].sem_num = sem_num; if (semop(dmix->semid, op, 2) < 0) return -errno; return 0; @@ -87,9 +85,8 @@ int snd_pcm_direct_semaphore_down(snd_pcm_direct_t *dmix, int sem_num) int snd_pcm_direct_semaphore_up(snd_pcm_direct_t *dmix, int sem_num) { - struct sembuf op = { 0, -1, SEM_UNDO | IPC_NOWAIT }; + struct sembuf op = { sem_num, -1, SEM_UNDO | IPC_NOWAIT }; assert(dmix->semid >= 0); - op.sem_num = sem_num; if (semop(dmix->semid, &op, 1) < 0) return -errno; return 0; @@ -252,6 +249,7 @@ static void server_job(snd_pcm_direct_t *dmix) snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT); if (shmctl(dmix->shmid, IPC_STAT, &buf) < 0) { snd_pcm_direct_shm_discard(dmix); + snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); continue; } server_printf("DIRECT SERVER: nattch = %i\n", (int)buf.shm_nattch); diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index 71ed8a94..cca0cfa0 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -585,12 +585,8 @@ static int snd_pcm_dmix_close(snd_pcm_t *pcm) if (dmix->client) snd_pcm_direct_client_discard(dmix); shm_sum_discard(dmix); - if (snd_pcm_direct_shm_discard(dmix) > 0) { - if (snd_pcm_direct_semaphore_discard(dmix) < 0) - snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); - } else { - snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); - } + snd_pcm_direct_shm_discard(dmix); + snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); if (dmix->bindings) free(dmix->bindings); pcm->private_data = NULL; @@ -748,12 +744,12 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, dmix = calloc(1, sizeof(snd_pcm_direct_t)); if (!dmix) { ret = -ENOMEM; - goto _err; + goto _err_nosem; } ret = snd_pcm_direct_parse_bindings(dmix, bindings); if (ret < 0) - goto _err; + goto _err_nosem; dmix->ipc_key = ipc_key; dmix->ipc_perm = ipc_perm; @@ -769,13 +765,13 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, ret = snd_pcm_direct_semaphore_create_or_connect(dmix); if (ret < 0) { SNDERR("unable to create IPC semaphore"); - goto _err; + goto _err_nosem; } ret = snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT); if (ret < 0) { snd_pcm_direct_semaphore_discard(dmix); if (--fail_sem_loop <= 0) - goto _err; + goto _err_nosem; continue; } break; @@ -795,7 +791,7 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, dmix->sync_ptr = snd_pcm_dmix_sync_ptr; if (first_instance) { - ret = snd_pcm_open_slave(&spcm, root, sconf, stream, mode); + ret = snd_pcm_open_slave(&spcm, root, sconf, stream, mode | SND_PCM_NONBLOCK); if (ret < 0) { SNDERR("unable to open slave"); goto _err; @@ -825,12 +821,15 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, dmix->shmptr->type = spcm->type; } else { + /* up semaphore to avoid deadlock */ + snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); ret = snd_pcm_direct_client_connect(dmix); if (ret < 0) { SNDERR("unable to connect client"); - return ret; + goto _err_nosem; } + snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT); ret = snd_pcm_hw_open_fd(&spcm, "dmix_client", dmix->hw_fd, 0, 0); if (ret < 0) { SNDERR("unable to open hardware"); @@ -883,27 +882,24 @@ int snd_pcm_dmix_open(snd_pcm_t **pcmp, const char *name, return 0; _err: + if (dmix->timer) + snd_timer_close(dmix->timer); + if (dmix->server) + snd_pcm_direct_server_discard(dmix); + if (dmix->client) + snd_pcm_direct_client_discard(dmix); + if (spcm) + snd_pcm_close(spcm); + if (dmix->u.dmix.shmid_sum >= 0) + shm_sum_discard(dmix); + if (dmix->shmid >= 0) + snd_pcm_direct_shm_discard(dmix); + if (snd_pcm_direct_semaphore_discard(dmix) < 0) + snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); + _err_nosem: if (dmix) { - if (dmix->timer) - snd_timer_close(dmix->timer); - if (dmix->server) - snd_pcm_direct_server_discard(dmix); - if (dmix->client) - snd_pcm_direct_client_discard(dmix); - if (spcm) - snd_pcm_close(spcm); - if (dmix->u.dmix.shmid_sum >= 0) - shm_sum_discard(dmix); - if (dmix->shmid >= 0) { - if (snd_pcm_direct_shm_discard(dmix) > 0) { - if (dmix->semid >= 0) { - if (snd_pcm_direct_semaphore_discard(dmix) < 0) - snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT); - } - } - } - if (dmix->bindings) - free(dmix->bindings); + if (dmix->bindings) + free(dmix->bindings); free(dmix); } if (pcm) diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index 097e2ba6..2ea97730 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -459,12 +459,8 @@ static int snd_pcm_dshare_close(snd_pcm_t *pcm) snd_pcm_direct_server_discard(dshare); if (dshare->client) snd_pcm_direct_client_discard(dshare); - if (snd_pcm_direct_shm_discard(dshare) > 0) { - if (snd_pcm_direct_semaphore_discard(dshare) < 0) - snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT); - } else { - snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT); - } + snd_pcm_direct_shm_discard(dshare); + snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT); if (dshare->bindings) free(dshare->bindings); pcm->private_data = NULL; @@ -614,17 +610,17 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, dshare = calloc(1, sizeof(snd_pcm_direct_t)); if (!dshare) { ret = -ENOMEM; - goto _err; + goto _err_nosem; } ret = snd_pcm_direct_parse_bindings(dshare, bindings); if (ret < 0) - goto _err; + goto _err_nosem; if (!dshare->bindings) { SNDERR("dshare: specify bindings!!!"); ret = -EINVAL; - goto _err; + goto _err_nosem; } dshare->ipc_key = ipc_key; @@ -634,20 +630,20 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, ret = snd_pcm_new(&pcm, dshare->type = SND_PCM_TYPE_DSHARE, name, stream, mode); if (ret < 0) - goto _err; + goto _err_nosem; while (1) { ret = snd_pcm_direct_semaphore_create_or_connect(dshare); if (ret < 0) { SNDERR("unable to create IPC semaphore"); - goto _err; + goto _err_nosem; } ret = snd_pcm_direct_semaphore_down(dshare, DIRECT_IPC_SEM_CLIENT); if (ret < 0) { snd_pcm_direct_semaphore_discard(dshare); if (--fail_sem_loop <= 0) - goto _err; + goto _err_nosem; continue; } break; @@ -667,7 +663,7 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, dshare->sync_ptr = snd_pcm_dshare_sync_ptr; if (first_instance) { - ret = snd_pcm_open_slave(&spcm, root, sconf, stream, mode); + ret = snd_pcm_open_slave(&spcm, root, sconf, stream, mode | SND_PCM_NONBLOCK); if (ret < 0) { SNDERR("unable to open slave"); goto _err; @@ -694,12 +690,15 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, dshare->shmptr->type = spcm->type; } else { + /* up semaphore to avoid deadlock */ + snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT); ret = snd_pcm_direct_client_connect(dshare); if (ret < 0) { SNDERR("unable to connect client"); - return ret; + goto _err_nosem; } + snd_pcm_direct_semaphore_down(dshare, DIRECT_IPC_SEM_CLIENT); ret = snd_pcm_hw_open_fd(&spcm, "dshare_client", dshare->hw_fd, 0, 0); if (ret < 0) { SNDERR("unable to open hardware"); @@ -751,27 +750,24 @@ int snd_pcm_dshare_open(snd_pcm_t **pcmp, const char *name, return 0; _err: + if (dshare->shmptr) + dshare->shmptr->u.dshare.chn_mask &= ~dshare->u.dshare.chn_mask; + if (dshare->timer) + snd_timer_close(dshare->timer); + if (dshare->server) + snd_pcm_direct_server_discard(dshare); + if (dshare->client) + snd_pcm_direct_client_discard(dshare); + if (spcm) + snd_pcm_close(spcm); + if (dshare->shmid >= 0) + snd_pcm_direct_shm_discard(dshare); + if (snd_pcm_direct_semaphore_discard(dshare) < 0) + snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT); + _err_nosem: if (dshare) { - if (dshare->shmptr) - dshare->shmptr->u.dshare.chn_mask &= ~dshare->u.dshare.chn_mask; - if (dshare->timer) - snd_timer_close(dshare->timer); - if (dshare->server) - snd_pcm_direct_server_discard(dshare); - if (dshare->client) - snd_pcm_direct_client_discard(dshare); - if (spcm) - snd_pcm_close(spcm); - if (dshare->shmid >= 0) { - if (snd_pcm_direct_shm_discard(dshare) > 0) { - if (dshare->semid >= 0) { - if (snd_pcm_direct_semaphore_discard(dshare) < 0) - snd_pcm_direct_semaphore_up(dshare, DIRECT_IPC_SEM_CLIENT); - } - } - } - if (dshare->bindings) - free(dshare->bindings); + if (dshare->bindings) + free(dshare->bindings); free(dshare); } if (pcm) diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 73d0b6f1..02c71091 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -366,12 +366,8 @@ static int snd_pcm_dsnoop_close(snd_pcm_t *pcm) snd_pcm_direct_server_discard(dsnoop); if (dsnoop->client) snd_pcm_direct_client_discard(dsnoop); - if (snd_pcm_direct_shm_discard(dsnoop) > 0) { - if (snd_pcm_direct_semaphore_discard(dsnoop) < 0) - snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT); - } else { - snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT); - } + snd_pcm_direct_shm_discard(dsnoop); + snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT); if (dsnoop->bindings) free(dsnoop->bindings); pcm->private_data = NULL; @@ -510,12 +506,12 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, dsnoop = calloc(1, sizeof(snd_pcm_direct_t)); if (!dsnoop) { ret = -ENOMEM; - goto _err; + goto _err_nosem; } ret = snd_pcm_direct_parse_bindings(dsnoop, bindings); if (ret < 0) - goto _err; + goto _err_nosem; dsnoop->ipc_key = ipc_key; dsnoop->ipc_perm = ipc_perm; @@ -524,20 +520,20 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, ret = snd_pcm_new(&pcm, dsnoop->type = SND_PCM_TYPE_DSNOOP, name, stream, mode); if (ret < 0) - goto _err; + goto _err_nosem; while (1) { ret = snd_pcm_direct_semaphore_create_or_connect(dsnoop); if (ret < 0) { SNDERR("unable to create IPC semaphore"); - goto _err; + goto _err_nosem; } ret = snd_pcm_direct_semaphore_down(dsnoop, DIRECT_IPC_SEM_CLIENT); if (ret < 0) { snd_pcm_direct_semaphore_discard(dsnoop); if (--fail_sem_loop <= 0) - goto _err; + goto _err_nosem; continue; } break; @@ -557,7 +553,7 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, dsnoop->sync_ptr = snd_pcm_dsnoop_sync_ptr; if (first_instance) { - ret = snd_pcm_open_slave(&spcm, root, sconf, stream, mode); + ret = snd_pcm_open_slave(&spcm, root, sconf, stream, mode | SND_PCM_NONBLOCK); if (ret < 0) { SNDERR("unable to open slave"); goto _err; @@ -584,12 +580,15 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, dsnoop->shmptr->type = spcm->type; } else { + /* up semaphore to avoid deadlock */ + snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT); ret = snd_pcm_direct_client_connect(dsnoop); if (ret < 0) { SNDERR("unable to connect client"); - return ret; + goto _err_nosem; } + snd_pcm_direct_semaphore_down(dsnoop, DIRECT_IPC_SEM_CLIENT); ret = snd_pcm_hw_open_fd(&spcm, "dsnoop_client", dsnoop->hw_fd, 0, 0); if (ret < 0) { SNDERR("unable to open hardware"); @@ -634,25 +633,22 @@ int snd_pcm_dsnoop_open(snd_pcm_t **pcmp, const char *name, return 0; _err: + if (dsnoop->timer) + snd_timer_close(dsnoop->timer); + if (dsnoop->server) + snd_pcm_direct_server_discard(dsnoop); + if (dsnoop->client) + snd_pcm_direct_client_discard(dsnoop); + if (spcm) + snd_pcm_close(spcm); + if (dsnoop->shmid >= 0) + snd_pcm_direct_shm_discard(dsnoop); + if (snd_pcm_direct_semaphore_discard(dsnoop) < 0) + snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT); + _err_nosem: if (dsnoop) { - if (dsnoop->timer) - snd_timer_close(dsnoop->timer); - if (dsnoop->server) - snd_pcm_direct_server_discard(dsnoop); - if (dsnoop->client) - snd_pcm_direct_client_discard(dsnoop); - if (spcm) - snd_pcm_close(spcm); - if (dsnoop->shmid >= 0) { - if (snd_pcm_direct_shm_discard(dsnoop) > 0) { - if (dsnoop->semid >= 0) { - if (snd_pcm_direct_semaphore_discard(dsnoop) < 0) - snd_pcm_direct_semaphore_up(dsnoop, DIRECT_IPC_SEM_CLIENT); - } - } - } - if (dsnoop->bindings) - free(dsnoop->bindings); + if (dsnoop->bindings) + free(dsnoop->bindings); free(dsnoop); } if (pcm) -- 2.47.1