On Thu, 07 Oct 2021 20:13:22 +0200, Pierre-Louis Bossart wrote:
Using snd_pcm_stream_lock_irqsave(be_substream, flags); will prevent multiple triggers indeed, but the state management is handled by dpcm_lock, so I think we have to use dpcm_lock/mutex in all BE transitions.
if ((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))
if ((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))
The stream lock can be put around those appropriate places, I suppose?
I doubled checked the code a bit more, and all functions using be->dpcm[stream].state and be->dpcm[stream].users are protected by the card->mutex.
The exceptions are dpcm_be_dai_trigger() and dpcm_show_state() so we probably don't need to worry too much about these fields.
I am more nervous about that the dpcm_lock was supposed to protect. It was added in "ASoC: dpcm: prevent snd_soc_dpcm use after free" to solve a race condition, according to the commit log between
void dpcm_be_disconnect( ... list_del(&dpcm->list_be); list_del(&dpcm->list_fe); kfree(dpcm); ...
and for_each_dpcm_fe() for_each_dpcm_be*()
That would suggest that every one of those loops should be protected, but that's not the case at all. In some cases the spinlock is taken *inside* of the loops.
I think this is what explains the problem reported by Gyeongtaek Lee in
https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.c...
the for_each_dpcm_be() loop in dpcm_be_dai_trigger() is NOT protected.
But if we add a spin-lock in there, the atomicity remains a problem.
I think the only solution is to follow the example of the PCM case, where the type of lock depends on the FE types, with the assumption that there are no mixed atomic/non-atomic FE configurations.
Yes, and I guess we can simply replace those all card->dpcm_lock with FE's stream lock. It'll solve the atomicity problem, too, and the FE stream lock can be applied outside the loop of dpcm_be_disconnect() gracefully.
And, this should solve the race with dpcm_be_dai_trigger() as well, because it's called from dpcm_fe_dai_trigger() that is called already inside the FE's stream lock held by PCM core. A PoC is something like below. (I replaced the superfluous *_irqsave with *_irq there)
The scenario above (using the FE stream lock) is one missing piece, though: the compress API seems using the DPCM, and this might not work. It needs more verification.
BTW, looking through the current code, I wonder how the snd_pcm_stream_lock_irq() call in dpcm_set_fe_update_state() doesn't deadlock when nonatomic=true. The function may be called from dpcm_fe_dai_prepare(), which is a PCM prepare callback for FE. And, a PCM prepare callback is called always inside the stream mutex, which is *the* stream lock in the case of nonatomic mode. Maybe I might overlook something obvious...
thanks,
Takashi
--- 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 48f71bb81a2f..c171e5f431b9 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -38,6 +38,15 @@ static inline const char *soc_codec_dai_name(struct snd_soc_pcm_runtime *rtd) return (rtd)->num_codecs == 1 ? asoc_rtd_to_codec(rtd, 0)->name : "multicodec"; }
+#define dpcm_fe_stream_lock_irq(fe, stream) \ + snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(fe, stream)) +#define dpcm_fe_stream_unlock_irq(fe, stream) \ + snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(fe, stream)) +#define dpcm_fe_stream_lock_irqsave(fe, stream, flags) \ + snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(fe, stream), flags) +#define dpcm_fe_stream_unlock_irqrestore(fe, stream, flags) \ + snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(fe, stream), flags) + #ifdef CONFIG_DEBUG_FS static const char *dpcm_state_string(enum snd_soc_dpcm_state state) { @@ -73,7 +82,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, @@ -101,7 +109,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe, goto out; }
- spin_lock_irqsave(&fe->card->dpcm_lock, flags); + dpcm_fe_stream_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be; params = &dpcm->hw_params; @@ -122,7 +130,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); + dpcm_fe_stream_unlock_irq(fe, stream); out: return offset; } @@ -1129,7 +1137,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) { @@ -1141,14 +1148,14 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe, if (!dpcm) return -ENOMEM;
+ dpcm_fe_stream_lock_irq(fe, stream); dpcm->be = be; 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); 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); + dpcm_fe_stream_unlock_irq(fe, stream);
dev_dbg(fe->dev, "connected new DPCM %s path %s %s %s\n", stream ? "capture" : "playback", fe->dai_link->name, @@ -1191,8 +1198,8 @@ 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;
+ dpcm_fe_stream_lock_irq(fe, stream); for_each_dpcm_be_safe(fe, stream, dpcm, d) { dev_dbg(fe->dev, "ASoC: BE %s disconnect check for %s\n", stream ? "capture" : "playback", @@ -1210,12 +1217,11 @@ 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); list_del(&dpcm->list_be); list_del(&dpcm->list_fe); - spin_unlock_irqrestore(&fe->card->dpcm_lock, flags); kfree(dpcm); } + dpcm_fe_stream_unlock_irq(fe, stream); }
/* get BE for DAI widget and stream */ @@ -1429,12 +1435,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); + dpcm_fe_stream_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); + dpcm_fe_stream_unlock_irq(fe, stream); }
void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream, @@ -2352,7 +2357,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); @@ -2421,7 +2425,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); + dpcm_fe_stream_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -2433,7 +2437,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); + dpcm_fe_stream_unlock_irq(fe, stream);
if (ret < 0) dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret); @@ -2843,10 +2847,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); + dpcm_fe_stream_lock_irq(fe, stream); for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe) @@ -2860,7 +2863,7 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe, } } } - spin_unlock_irqrestore(&fe->card->dpcm_lock, flags); + dpcm_fe_stream_unlock_irq(fe, stream);
/* it's safe to do this BE DAI */ return ret;