* [RFC PATCH v2 1/5] ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update()
[not found] <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com>
@ 2021-10-04 22:54 ` Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 2/5] ASoC: soc-pcm: don't export local functions, use static Pierre-Louis Bossart
` (3 subsequent siblings)
4 siblings, 0 replies; 5+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-04 22:54 UTC (permalink / raw)
To: alsa-devel
Cc: tiwai, broonie, vkoul, Sameer Pujar, Gyeongtaek Lee,
Peter Ujfalusi, Kuninori Morimoto, Pierre-Louis Bossart,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, open list
This function is not used anywhere, including soc-pcm.c
Remove dead code.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
include/sound/soc-dpcm.h | 3 ---
sound/soc/soc-pcm.c | 9 ---------
2 files changed, 12 deletions(-)
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index bc7af90099a8..72d45ad47ee3 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -121,9 +121,6 @@ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream);
-/* is the current PCM operation for this FE ? */
-int snd_soc_dpcm_fe_can_update(struct snd_soc_pcm_runtime *fe, int stream);
-
/* is the current PCM operation for this BE ? */
int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index fc1854e3e43f..17476e3219ea 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2805,15 +2805,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
return ret;
}
-/* is the current PCM operation for this FE ? */
-int snd_soc_dpcm_fe_can_update(struct snd_soc_pcm_runtime *fe, int stream)
-{
- if (fe->dpcm[stream].runtime_update == SND_SOC_DPCM_UPDATE_FE)
- return 1;
- return 0;
-}
-EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_can_update);
-
/* is the current PCM operation for this BE ? */
int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v2 2/5] ASoC: soc-pcm: don't export local functions, use static
[not found] <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com>
2021-10-04 22:54 ` [RFC PATCH v2 1/5] ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() Pierre-Louis Bossart
@ 2021-10-04 22:54 ` Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 3/5] ASoC: soc-pcm: replace dpcm_lock with dpcm_mutex Pierre-Louis Bossart
` (2 subsequent siblings)
4 siblings, 0 replies; 5+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-04 22:54 UTC (permalink / raw)
To: alsa-devel
Cc: tiwai, broonie, vkoul, Sameer Pujar, Gyeongtaek Lee,
Peter Ujfalusi, Kuninori Morimoto, Pierre-Louis Bossart,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, open list
No one uses the following functions outside of soc-pcm.c
snd_soc_dpcm_can_be_free_stop()
snd_soc_dpcm_can_be_params()
snd_soc_dpcm_be_can_update()
In preparation for locking changes, move them to static functions and
remove the EXPORT_SYMBOL_GPL()
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
include/sound/soc-dpcm.h | 12 ------------
sound/soc/soc-pcm.c | 21 +++++++++++++++------
2 files changed, 15 insertions(+), 18 deletions(-)
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 72d45ad47ee3..9c00118603e7 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -113,18 +113,6 @@ struct snd_soc_dpcm_runtime {
#define for_each_dpcm_be_rollback(fe, stream, _dpcm) \
list_for_each_entry_continue_reverse(_dpcm, &(fe)->dpcm[stream].be_clients, list_be)
-/* can this BE stop and free */
-int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
- struct snd_soc_pcm_runtime *be, int stream);
-
-/* can this BE perform a hw_params() */
-int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
- struct snd_soc_pcm_runtime *be, int stream);
-
-/* is the current PCM operation for this BE ? */
-int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
- struct snd_soc_pcm_runtime *be, int stream);
-
/* get the substream for this BE */
struct snd_pcm_substream *
snd_soc_dpcm_get_substream(struct snd_soc_pcm_runtime *be, int stream);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 17476e3219ea..360811f8a18c 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -29,6 +29,18 @@
#define DPCM_MAX_BE_USERS 8
+/* can this BE stop and free */
+static int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
+ struct snd_soc_pcm_runtime *be, int stream);
+
+/* can this BE perform a hw_params() */
+static int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
+ struct snd_soc_pcm_runtime *be, int stream);
+
+/* is the current PCM operation for this BE ? */
+static int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
+ struct snd_soc_pcm_runtime *be, int stream);
+
static inline const char *soc_cpu_dai_name(struct snd_soc_pcm_runtime *rtd)
{
return (rtd)->num_cpus == 1 ? asoc_rtd_to_cpu(rtd, 0)->name : "multicpu";
@@ -2806,7 +2818,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num)
}
/* is the current PCM operation for this BE ? */
-int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
+static int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
{
if ((fe->dpcm[stream].runtime_update == SND_SOC_DPCM_UPDATE_FE) ||
@@ -2815,7 +2827,6 @@ int snd_soc_dpcm_be_can_update(struct snd_soc_pcm_runtime *fe,
return 1;
return 0;
}
-EXPORT_SYMBOL_GPL(snd_soc_dpcm_be_can_update);
/* get the substream for this BE */
struct snd_pcm_substream *
@@ -2861,7 +2872,7 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
* We can only hw_free, stop, pause or suspend a BE DAI if any of it's FE
* are not running, paused or suspended for the specified stream direction.
*/
-int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
+static int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
{
const enum snd_soc_dpcm_state state[] = {
@@ -2872,13 +2883,12 @@ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
}
-EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_free_stop);
/*
* We can only change hw params a BE DAI if any of it's FE are not prepared,
* running, paused or suspended for the specified stream direction.
*/
-int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
+static int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
{
const enum snd_soc_dpcm_state state[] = {
@@ -2890,4 +2900,3 @@ int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
}
-EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_params);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v2 3/5] ASoC: soc-pcm: replace dpcm_lock with dpcm_mutex
[not found] <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com>
2021-10-04 22:54 ` [RFC PATCH v2 1/5] ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 2/5] ASoC: soc-pcm: don't export local functions, use static Pierre-Louis Bossart
@ 2021-10-04 22:54 ` Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 4/5] ASoC: soc-pcm: protect for_each_dpcm_be() loops " Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 5/5] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart
4 siblings, 0 replies; 5+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-04 22:54 UTC (permalink / raw)
To: alsa-devel
Cc: tiwai, broonie, vkoul, Sameer Pujar, Gyeongtaek Lee,
Peter Ujfalusi, Kuninori Morimoto, Pierre-Louis Bossart,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, open list
It's not clear why a spinlock was used, and even less why the
'dpcm_lock' is used with the irqsave/irqrestore: DPCM functions are
typically not used in interrupt context.
Move to a mutex which will allow us to sleep for longer periods of
time and handle non-atomic triggers.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
include/sound/soc.h | 2 +-
sound/soc/soc-core.c | 2 +-
sound/soc/soc-pcm.c | 30 ++++++++++++------------------
3 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e6dd8a257c5..c13d8ec72d62 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -893,7 +893,7 @@ struct snd_soc_card {
struct mutex pcm_mutex;
enum snd_soc_pcm_subclass pcm_subclass;
- spinlock_t dpcm_lock;
+ struct mutex dpcm_mutex; /* protect all dpcm states and updates */
int (*probe)(struct snd_soc_card *card);
int (*late_probe)(struct snd_soc_card *card);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c830e96afba2..e94d43c392e0 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2339,7 +2339,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
mutex_init(&card->mutex);
mutex_init(&card->dapm_mutex);
mutex_init(&card->pcm_mutex);
- spin_lock_init(&card->dpcm_lock);
+ mutex_init(&card->dpcm_mutex);
return snd_soc_bind_card(card);
}
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 360811f8a18c..af12593b033a 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -85,7 +85,6 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
struct snd_pcm_hw_params *params = &fe->dpcm[stream].hw_params;
struct snd_soc_dpcm *dpcm;
ssize_t offset = 0;
- unsigned long flags;
/* FE state */
offset += scnprintf(buf + offset, size - offset,
@@ -113,7 +112,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
goto out;
}
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
params = &dpcm->hw_params;
@@ -134,7 +133,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
params_channels(params),
params_rate(params));
}
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ mutex_unlock(&fe->card->dpcm_mutex);
out:
return offset;
}
@@ -1141,7 +1140,6 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
{
struct snd_soc_dpcm *dpcm;
- unsigned long flags;
/* only add new dpcms */
for_each_dpcm_be(fe, stream, dpcm) {
@@ -1157,10 +1155,10 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
dpcm->fe = fe;
be->dpcm[stream].runtime = fe->dpcm[stream].runtime;
dpcm->state = SND_SOC_DPCM_LINK_STATE_NEW;
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ mutex_lock(&fe->card->dpcm_mutex);
list_add(&dpcm->list_be, &fe->dpcm[stream].be_clients);
list_add(&dpcm->list_fe, &be->dpcm[stream].fe_clients);
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ mutex_unlock(&fe->card->dpcm_mutex);
dev_dbg(fe->dev, "connected new DPCM %s path %s %s %s\n",
stream ? "capture" : "playback", fe->dai_link->name,
@@ -1203,7 +1201,6 @@ static void dpcm_be_reparent(struct snd_soc_pcm_runtime *fe,
void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
{
struct snd_soc_dpcm *dpcm, *d;
- unsigned long flags;
for_each_dpcm_be_safe(fe, stream, dpcm, d) {
dev_dbg(fe->dev, "ASoC: BE %s disconnect check for %s\n",
@@ -1222,10 +1219,10 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
dpcm_remove_debugfs_state(dpcm);
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ mutex_lock(&fe->card->dpcm_mutex);
list_del(&dpcm->list_be);
list_del(&dpcm->list_fe);
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ mutex_unlock(&fe->card->dpcm_mutex);
kfree(dpcm);
}
}
@@ -1441,12 +1438,11 @@ int dpcm_process_paths(struct snd_soc_pcm_runtime *fe,
void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
{
struct snd_soc_dpcm *dpcm;
- unsigned long flags;
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm)
dpcm_set_be_update_state(dpcm->be, stream, SND_SOC_DPCM_UPDATE_NO);
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ mutex_unlock(&fe->card->dpcm_mutex);
}
void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
@@ -2364,7 +2360,6 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
struct snd_soc_dpcm *dpcm;
enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
int ret = 0;
- unsigned long flags;
dev_dbg(fe->dev, "ASoC: runtime %s open on FE %s\n",
stream ? "capture" : "playback", fe->dai_link->name);
@@ -2433,7 +2428,7 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
dpcm_be_dai_shutdown(fe, stream);
disconnect:
/* disconnect any pending BEs */
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -2445,7 +2440,7 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
be->dpcm[stream].state == SND_SOC_DPCM_STATE_NEW)
dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
}
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ mutex_unlock(&fe->card->dpcm_mutex);
if (ret < 0)
dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
@@ -2845,10 +2840,9 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
struct snd_soc_dpcm *dpcm;
int state;
int ret = 1;
- unsigned long flags;
int i;
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe)
@@ -2862,7 +2856,7 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
}
}
}
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ mutex_unlock(&fe->card->dpcm_mutex);
/* it's safe to do this BE DAI */
return ret;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v2 4/5] ASoC: soc-pcm: protect for_each_dpcm_be() loops with dpcm_mutex
[not found] <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com>
` (2 preceding siblings ...)
2021-10-04 22:54 ` [RFC PATCH v2 3/5] ASoC: soc-pcm: replace dpcm_lock with dpcm_mutex Pierre-Louis Bossart
@ 2021-10-04 22:54 ` Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 5/5] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart
4 siblings, 0 replies; 5+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-04 22:54 UTC (permalink / raw)
To: alsa-devel
Cc: tiwai, broonie, vkoul, Sameer Pujar, Gyeongtaek Lee,
Peter Ujfalusi, Kuninori Morimoto, Pierre-Louis Bossart,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, open list
When more than one FE is connected to a BE, e.g. in a mixing use case,
the BE can be triggered multiple times when the FE are opened/started
concurrently. This race condition is problematic in the case of
SoundWire BE dailinks, and this is not desirable in a general
case. The code carefully checks when the BE can be stopped or
hw_free'ed, but the trigger code does not use any mutual exclusion.
This can be fixed by using the dpcm_mutex.
As discussed on the alsa-mailing list [1], there is an additional
issue that the BEs can be disconnected dynamically, leading to
potential accesses to freed memory.
This patch suggests a general protection of the for_each_dpcm_be()
loop with the dpcm_mutex, to solve both the problem of multiple
triggers and BE dynamic addition/removal.
[1] https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.com/
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
sound/soc/soc-pcm.c | 48 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 9 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index af12593b033a..6089e0c1bf9f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -320,6 +320,7 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
{
struct snd_soc_dpcm *dpcm;
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, dir, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -333,6 +334,8 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
snd_soc_dapm_stream_event(be, dir, event);
}
+ mutex_unlock(&fe->card->dpcm_mutex);
+
snd_soc_dapm_stream_event(fe, dir, event);
@@ -1142,10 +1145,14 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
struct snd_soc_dpcm *dpcm;
/* only add new dpcms */
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
- if (dpcm->be == be && dpcm->fe == fe)
+ if (dpcm->be == be && dpcm->fe == fe) {
+ mutex_unlock(&fe->card->dpcm_mutex);
return 0;
+ }
}
+ mutex_unlock(&fe->card->dpcm_mutex);
dpcm = kzalloc(sizeof(struct snd_soc_dpcm), GFP_KERNEL);
if (!dpcm)
@@ -1202,6 +1209,7 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
{
struct snd_soc_dpcm *dpcm, *d;
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be_safe(fe, stream, dpcm, d) {
dev_dbg(fe->dev, "ASoC: BE %s disconnect check for %s\n",
stream ? "capture" : "playback",
@@ -1219,12 +1227,11 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
dpcm_remove_debugfs_state(dpcm);
- mutex_lock(&fe->card->dpcm_mutex);
list_del(&dpcm->list_be);
list_del(&dpcm->list_fe);
- mutex_unlock(&fe->card->dpcm_mutex);
kfree(dpcm);
}
+ mutex_unlock(&fe->card->dpcm_mutex);
}
/* get BE for DAI widget and stream */
@@ -1351,6 +1358,7 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream,
int prune = 0;
/* Destroy any old FE <--> BE connections */
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
if (dpcm_be_is_active(dpcm, stream, *list_))
continue;
@@ -1362,6 +1370,7 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream,
dpcm_set_be_update_state(dpcm->be, stream, SND_SOC_DPCM_UPDATE_BE);
prune++;
}
+ mutex_unlock(&fe->card->dpcm_mutex);
dev_dbg(fe->dev, "ASoC: found %d old BE paths for pruning\n", prune);
return prune;
@@ -1451,13 +1460,16 @@ void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
struct snd_soc_dpcm *dpcm;
/* disable any enabled and non active backends */
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_pcm_substream *be_substream =
snd_soc_dpcm_get_substream(be, stream);
- if (dpcm == last)
+ if (dpcm == last) {
+ mutex_unlock(&fe->card->dpcm_mutex);
return;
+ }
/* is this op for this BE ? */
if (!snd_soc_dpcm_be_can_update(fe, be, stream))
@@ -1487,6 +1499,7 @@ void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
be_substream->runtime = NULL;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
}
+ mutex_unlock(&fe->card->dpcm_mutex);
}
int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
@@ -1496,6 +1509,7 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
int err, count = 0;
/* only startup BE DAIs that are either sinks or sources to this FE DAI */
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_pcm_substream *be_substream;
@@ -1546,11 +1560,13 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
be->dpcm[stream].state = SND_SOC_DPCM_STATE_OPEN;
count++;
}
+ mutex_unlock(&fe->card->dpcm_mutex);
return count;
unwind:
dpcm_be_dai_startup_rollback(fe, stream, dpcm);
+ mutex_unlock(&fe->card->dpcm_mutex);
dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n",
__func__, be->dai_link->name, err);
@@ -1605,6 +1621,7 @@ static void dpcm_runtime_setup_be_format(struct snd_pcm_substream *substream)
* if FE want to use it (= dpcm_merged_format)
*/
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_soc_pcm_stream *codec_stream;
@@ -1623,6 +1640,7 @@ static void dpcm_runtime_setup_be_format(struct snd_pcm_substream *substream)
soc_pcm_hw_update_format(hw, codec_stream);
}
}
+ mutex_unlock(&fe->card->dpcm_mutex);
}
static void dpcm_runtime_setup_be_chan(struct snd_pcm_substream *substream)
@@ -1640,7 +1658,7 @@ static void dpcm_runtime_setup_be_chan(struct snd_pcm_substream *substream)
* It returns merged BE codec channel;
* if FE want to use it (= dpcm_merged_chan)
*/
-
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_soc_pcm_stream *cpu_stream;
@@ -1671,6 +1689,7 @@ static void dpcm_runtime_setup_be_chan(struct snd_pcm_substream *substream)
soc_pcm_hw_update_chan(hw, codec_stream);
}
}
+ mutex_unlock(&fe->card->dpcm_mutex);
}
static void dpcm_runtime_setup_be_rate(struct snd_pcm_substream *substream)
@@ -1688,7 +1707,7 @@ static void dpcm_runtime_setup_be_rate(struct snd_pcm_substream *substream)
* It returns merged BE codec channel;
* if FE want to use it (= dpcm_merged_chan)
*/
-
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_soc_pcm_stream *pcm;
@@ -1708,6 +1727,7 @@ static void dpcm_runtime_setup_be_rate(struct snd_pcm_substream *substream)
soc_pcm_hw_update_rate(hw, pcm);
}
}
+ mutex_unlock(&fe->card->dpcm_mutex);
}
static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
@@ -1730,6 +1750,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
}
/* apply symmetry for BE */
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_pcm_substream *be_substream =
@@ -1755,6 +1776,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream,
}
}
error:
+ mutex_unlock(&fe->card->dpcm_mutex);
if (err < 0)
dev_err(fe->dev, "ASoC: %s failed (%d)\n", __func__, err);
@@ -1830,6 +1852,7 @@ void dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream)
/* only hw_params backends that are either sinks or sources
* to this frontend DAI */
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -1842,7 +1865,7 @@ void dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream)
/* only free hw when no longer used - check all FEs */
if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
- continue;
+ continue;
/* do not free hw if this BE is used by other FE */
if (be->dpcm[stream].users > 1)
@@ -1863,6 +1886,7 @@ void dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream)
be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
}
+ mutex_unlock(&fe->card->dpcm_mutex);
}
static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
@@ -1896,6 +1920,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
struct snd_soc_dpcm *dpcm;
int ret;
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
be = dpcm->be;
be_substream = snd_soc_dpcm_get_substream(be, stream);
@@ -1935,6 +1960,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_PARAMS;
}
+ mutex_unlock(&fe->card->dpcm_mutex);
return 0;
unwind:
@@ -1961,6 +1987,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
soc_pcm_hw_free(be_substream);
}
+ mutex_unlock(&fe->card->dpcm_mutex);
return ret;
}
@@ -2008,6 +2035,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
struct snd_soc_dpcm *dpcm;
int ret = 0;
+ mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_pcm_substream *be_substream;
@@ -2097,6 +2125,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
}
}
end:
+ mutex_unlock(&fe->card->dpcm_mutex);
if (ret < 0)
dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n",
__func__, be->dai_link->name, ret);
@@ -2831,6 +2860,7 @@ struct snd_pcm_substream *
}
EXPORT_SYMBOL_GPL(snd_soc_dpcm_get_substream);
+/* This must be called with fe->card->dpcm_mutex held */
static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be,
int stream,
@@ -2842,7 +2872,6 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
int ret = 1;
int i;
- mutex_lock(&fe->card->dpcm_mutex);
for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe)
@@ -2856,7 +2885,6 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
}
}
}
- mutex_unlock(&fe->card->dpcm_mutex);
/* it's safe to do this BE DAI */
return ret;
@@ -2865,6 +2893,7 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
/*
* We can only hw_free, stop, pause or suspend a BE DAI if any of it's FE
* are not running, paused or suspended for the specified stream direction.
+ * This must be called with fe->card->dpcm_mutex held.
*/
static int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
@@ -2881,6 +2910,7 @@ static int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
/*
* We can only change hw params a BE DAI if any of it's FE are not prepared,
* running, paused or suspended for the specified stream direction.
+ * This must be called with fe->card->dpcm_mutex held.
*/
static int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream)
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v2 5/5] ASoC: soc-pcm: test refcount before triggering
[not found] <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com>
` (3 preceding siblings ...)
2021-10-04 22:54 ` [RFC PATCH v2 4/5] ASoC: soc-pcm: protect for_each_dpcm_be() loops " Pierre-Louis Bossart
@ 2021-10-04 22:54 ` Pierre-Louis Bossart
4 siblings, 0 replies; 5+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-04 22:54 UTC (permalink / raw)
To: alsa-devel
Cc: tiwai, broonie, vkoul, Sameer Pujar, Gyeongtaek Lee,
Peter Ujfalusi, Kuninori Morimoto, Pierre-Louis Bossart,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, open list
On start/pause_release/resume, when more than one FE is connected to
the same BE, it's possible that the trigger is sent more than
once. This is not desirable, we only want to trigger a BE once, which
is straightforward to implement with a refcount.
For stop/pause/suspend, the problem is more complicated: the check
implemented in snd_soc_dpcm_can_be_free_stop() may fail due to a
conceptual deadlock when we trigger the BE before the FE. In this
case, the FE states have not yet changed, so there are corner cases
where the TRIGGER_STOP is never sent - the dual case of start where
multiple triggers might be sent.
This patch suggests an unconditional trigger in all cases, without
checking the FE states, using a refcount protected by a mutex.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
include/sound/soc-dpcm.h | 2 ++
sound/soc/soc-pcm.c | 53 +++++++++++++++++++++++++++++++---------
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 9c00118603e7..6d84e2c60ca8 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -101,6 +101,8 @@ struct snd_soc_dpcm_runtime {
enum snd_soc_dpcm_state state;
int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
+
+ int be_start; /* refcount protected by card's dpcm_mutex */
};
#define for_each_dpcm_fe(be, stream, _dpcm) \
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 6089e0c1bf9f..ed898d83fe17 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1556,7 +1556,7 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
goto unwind;
}
-
+ be->dpcm[stream].be_start = 0;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_OPEN;
count++;
}
@@ -2051,14 +2051,21 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
switch (cmd) {
case SNDRV_PCM_TRIGGER_START:
- if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) &&
+ if (!be->dpcm[stream].be_start &&
+ (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) &&
(be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) &&
(be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
continue;
+ be->dpcm[stream].be_start++;
+ if (be->dpcm[stream].be_start != 1)
+ continue;
+
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start--;
goto end;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2066,9 +2073,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND))
continue;
+ be->dpcm[stream].be_start++;
+ if (be->dpcm[stream].be_start != 1)
+ continue;
+
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start--;
goto end;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2076,9 +2089,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
continue;
+ be->dpcm[stream].be_start++;
+ if (be->dpcm[stream].be_start != 1)
+ continue;
+
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start--;
goto end;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2087,12 +2106,18 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
(be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
continue;
- if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+ if (be->dpcm[stream].state == SND_SOC_DPCM_STATE_START)
+ be->dpcm[stream].be_start--;
+
+ if (be->dpcm[stream].be_start != 0)
continue;
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ if (be->dpcm[stream].state == SND_SOC_DPCM_STATE_START)
+ be->dpcm[stream].be_start++;
goto end;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
break;
@@ -2100,12 +2125,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
continue;
- if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+ be->dpcm[stream].be_start--;
+ if (be->dpcm[stream].be_start != 0)
continue;
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start++;
goto end;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
break;
@@ -2113,12 +2141,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
continue;
- if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+ be->dpcm[stream].be_start--;
+ if (be->dpcm[stream].be_start != 0)
continue;
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start++;
goto end;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-04 22:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20211004225441.233375-1-pierre-louis.bossart@linux.intel.com>
2021-10-04 22:54 ` [RFC PATCH v2 1/5] ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 2/5] ASoC: soc-pcm: don't export local functions, use static Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 3/5] ASoC: soc-pcm: replace dpcm_lock with dpcm_mutex Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 4/5] ASoC: soc-pcm: protect for_each_dpcm_be() loops " Pierre-Louis Bossart
2021-10-04 22:54 ` [RFC PATCH v2 5/5] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).