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.com/ > > 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;
