>> Basically we need to protect two things:
>> - The BE links
>> - The concurrent accesses to BEs
>> The former belongs to each FE that holds the links, and the FE stream
>> lock would cover. The latter is rather a per-BE business.
>>
>> An oft-seen risk of multiple locks is deadlocking, but in this case,
>> as long as we keep the lock order FE->BE, nothing wrong can happen.
>
> famous last words "nothing wrong can happen." :-)
>
> I already added a helper to do this FE lock, I can easily replace the
> implementation to remove the spin_lock and use the FE PCM lock.
> we might even add the lock in the definition of for_each_dpcm_be() to
> avoid misses.
>
> Let me try this out today, thanks for the suggestions.
well, it's not successful at all...
When I replace the existing dpcm_lock with the FE PCM lock as you
suggested, without any additional changes, speaker-test produces valid
audio on the endpoints, but if I try a Ctrl-C or limit the number of
loops with the '-l' option, I hear an endless loop on the same buffer
and I have to power cycle my test device to stop the sound.
See 2 patches attached, the first patch with the introduction of the
helper works fine, the second with the use of the FE PCM lock doesn't.
In hindsight I am glad I worked on minimal patches, one after the other,
to identify problems.
And when I add the BE lock, then nothing happens. Device stuck and no
audio...
There must be something we're missing related to the locking...
My work version is at
https://github.com/plbossart/sound/tree/fix/dpcm-lock5 if anyone wants
to take a look.
>From f0a7068b50057eb918821dbcda6d448f49f5f1c4 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
Date: Thu, 7 Oct 2021 15:35:23 -0500
Subject: [PATCH 2/2] ASoC: soc-pcm: remove dpcm spin_lock, use PCM stream lock
There is no need for a DPCM-specific lock at the card level, we can
use the FE-specific PCM lock instead. In addition, these PCM lock will
rely on either a spin-lock or a mutex depending on atomicity.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
---
include/sound/soc.h | 2 --
sound/soc/soc-core.c | 1 -
sound/soc/soc-pcm.c | 4 ++--
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e6dd8a257c5..5872a8864f3b 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -893,8 +893,6 @@ struct snd_soc_card {
struct mutex pcm_mutex;
enum snd_soc_pcm_subclass pcm_subclass;
- spinlock_t dpcm_lock;
-
int (*probe)(struct snd_soc_card *card);
int (*late_probe)(struct snd_soc_card *card);
int (*remove)(struct snd_soc_card *card);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c830e96afba2..51ef9917ca98 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2339,7 +2339,6 @@ 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);
return snd_soc_bind_card(card);
}
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 52851827d53f..c1dbd8633587 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -31,13 +31,13 @@
void snd_soc_dpcm_fe_lock_irq(struct snd_soc_pcm_runtime *fe, int stream)
{
- spin_lock_irq(&fe->card->dpcm_lock);
+ snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(fe, stream));
}
EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_lock_irq);
void snd_soc_dpcm_fe_unlock_irq(struct snd_soc_pcm_runtime *fe, int stream)
{
- spin_unlock_irq(&fe->card->dpcm_lock);
+ snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(fe, stream));
}
EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_unlock_irq);
--
2.25.1
>From c35ccac18235f6f50e2d0e9fb6167612dcc753f7 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
Date: Wed, 29 Sep 2021 11:30:19 -0500
Subject: [PATCH 1/2] ASoC: soc-pcm: introduce
snd_soc_dpcm_fe_lock_irq/unlock_irq()
In preparation for more changes, add two new helpers to gradually
modify the DPCM locks.
Since DPCM functions are not used from interrupt handlers, we can only
use the lock_irq case.
While most of the uses of DPCM are internal to soc-pcm.c, some drivers
in soc/fsl and soc/sh do make use of DPCM-related loops that will
require protection, adding EXPORT_SYMBOL_GPL() is needed for those
drivers.
The stream argument is unused in this patch but will be enabled in
follow-up patches.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
---
include/sound/soc-dpcm.h | 3 +++
sound/soc/soc-pcm.c | 42 +++++++++++++++++++++++-----------------
2 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 9c00118603e7..8ed40b8f3da8 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -151,4 +151,7 @@ bool dpcm_end_walk_at_be(struct snd_soc_dapm_widget *widget, enum snd_soc_dapm_d
#define dpcm_be_dai_startup_unwind(fe, stream) dpcm_be_dai_stop(fe, stream, 0, NULL)
#define dpcm_be_dai_shutdown(fe, stream) dpcm_be_dai_stop(fe, stream, 1, NULL)
+void snd_soc_dpcm_fe_lock_irq(struct snd_soc_pcm_runtime *fe, int stream);
+void snd_soc_dpcm_fe_unlock_irq(struct snd_soc_pcm_runtime *fe, int stream);
+
#endif
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 19539300d94d..52851827d53f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -29,6 +29,18 @@
#define DPCM_MAX_BE_USERS 8
+void snd_soc_dpcm_fe_lock_irq(struct snd_soc_pcm_runtime *fe, int stream)
+{
+ spin_lock_irq(&fe->card->dpcm_lock);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_lock_irq);
+
+void snd_soc_dpcm_fe_unlock_irq(struct snd_soc_pcm_runtime *fe, int stream)
+{
+ spin_unlock_irq(&fe->card->dpcm_lock);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_unlock_irq);
+
/* 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);
@@ -85,7 +97,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 +124,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
goto out;
}
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+ snd_soc_dpcm_fe_lock_irq(fe, stream);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
params = &dpcm->hw_params;
@@ -134,7 +145,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);
+ snd_soc_dpcm_fe_unlock_irq(fe, stream);
out:
return offset;
}
@@ -1141,7 +1152,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 +1167,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);
+ snd_soc_dpcm_fe_lock_irq(fe, stream);
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);
+ snd_soc_dpcm_fe_unlock_irq(fe, stream);
dev_dbg(fe->dev, "connected new DPCM %s path %s %s %s\n",
stream ? "capture" : "playback", fe->dai_link->name,
@@ -1203,7 +1213,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 +1231,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);
+ snd_soc_dpcm_fe_lock_irq(fe, stream);
list_del(&dpcm->list_be);
list_del(&dpcm->list_fe);
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ snd_soc_dpcm_fe_unlock_irq(fe, stream);
kfree(dpcm);
}
}
@@ -1451,12 +1460,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);
+ snd_soc_dpcm_fe_lock_irq(fe, stream);
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);
+ snd_soc_dpcm_fe_unlock_irq(fe, stream);
}
void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
@@ -2374,7 +2382,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);
@@ -2443,7 +2450,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);
+ snd_soc_dpcm_fe_lock_irq(fe, stream);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -2455,7 +2462,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);
+ snd_soc_dpcm_fe_unlock_irq(fe, stream);
if (ret < 0)
dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
@@ -2855,10 +2862,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);
+ snd_soc_dpcm_fe_lock_irq(fe, stream);
for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe)
@@ -2872,7 +2878,7 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
}
}
}
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+ snd_soc_dpcm_fe_unlock_irq(fe, stream);
/* it's safe to do this BE DAI */
return ret;
--
2.25.1
