From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> To: alsa-devel@alsa-project.org Cc: tiwai@suse.de, broonie@kernel.org, vkoul@kernel.org, Sameer Pujar <spujar@nvidia.com>, Gyeongtaek Lee <gt82.lee@samsung.com>, Peter Ujfalusi <peter.ujfalusi@linux.intel.com>, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, Liam Girdwood <lgirdwood@gmail.com>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, linux-kernel@vger.kernel.org (open list) Subject: [RFC PATCH v3 12/13] ASoC: soc-pcm: test refcount before triggering Date: Wed, 13 Oct 2021 09:30:49 -0500 [thread overview] Message-ID: <20211013143050.244444-13-pierre-louis.bossart@linux.intel.com> (raw) In-Reply-To: <20211013143050.244444-1-pierre-louis.bossart@linux.intel.com> 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 the BE PCM stream lock. 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 8ed40b8f3da8..9464c588f71d 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 BE stream pcm lock */ }; #define for_each_dpcm_fe(be, stream, _dpcm) \ diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index d115e9409c14..1967980d0f79 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1600,7 +1600,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++; } @@ -2096,14 +2096,21 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, snd_soc_dpcm_be_lock_irqsave(be, stream, flags); 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)) goto unlock_be; + be->dpcm[stream].be_start++; + if (be->dpcm[stream].be_start != 1) + goto unlock_be; + ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start--; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; @@ -2111,9 +2118,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND)) goto unlock_be; + be->dpcm[stream].be_start++; + if (be->dpcm[stream].be_start != 1) + goto unlock_be; + ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start--; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; @@ -2121,9 +2134,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) goto unlock_be; + be->dpcm[stream].be_start++; + if (be->dpcm[stream].be_start != 1) + goto unlock_be; + ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start--; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; @@ -2132,12 +2151,18 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) goto unlock_be; - if (!snd_soc_dpcm_can_be_free_stop_locked(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) goto unlock_be; 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 unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP; break; @@ -2145,12 +2170,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) goto unlock_be; - if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) + be->dpcm[stream].be_start--; + if (be->dpcm[stream].be_start != 0) goto unlock_be; ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start++; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND; break; @@ -2158,12 +2186,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) goto unlock_be; - if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) + be->dpcm[stream].be_start--; + if (be->dpcm[stream].be_start != 0) goto unlock_be; ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start++; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED; break; -- 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> To: alsa-devel@alsa-project.org Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>, tiwai@suse.de, open list <linux-kernel@vger.kernel.org>, Sameer Pujar <spujar@nvidia.com>, Takashi Iwai <tiwai@suse.com>, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, Liam Girdwood <lgirdwood@gmail.com>, vkoul@kernel.org, broonie@kernel.org, Gyeongtaek Lee <gt82.lee@samsung.com>, Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Subject: [RFC PATCH v3 12/13] ASoC: soc-pcm: test refcount before triggering Date: Wed, 13 Oct 2021 09:30:49 -0500 [thread overview] Message-ID: <20211013143050.244444-13-pierre-louis.bossart@linux.intel.com> (raw) In-Reply-To: <20211013143050.244444-1-pierre-louis.bossart@linux.intel.com> 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 the BE PCM stream lock. 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 8ed40b8f3da8..9464c588f71d 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 BE stream pcm lock */ }; #define for_each_dpcm_fe(be, stream, _dpcm) \ diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index d115e9409c14..1967980d0f79 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1600,7 +1600,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++; } @@ -2096,14 +2096,21 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, snd_soc_dpcm_be_lock_irqsave(be, stream, flags); 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)) goto unlock_be; + be->dpcm[stream].be_start++; + if (be->dpcm[stream].be_start != 1) + goto unlock_be; + ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start--; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; @@ -2111,9 +2118,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND)) goto unlock_be; + be->dpcm[stream].be_start++; + if (be->dpcm[stream].be_start != 1) + goto unlock_be; + ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start--; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; @@ -2121,9 +2134,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) goto unlock_be; + be->dpcm[stream].be_start++; + if (be->dpcm[stream].be_start != 1) + goto unlock_be; + ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start--; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; @@ -2132,12 +2151,18 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) goto unlock_be; - if (!snd_soc_dpcm_can_be_free_stop_locked(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) goto unlock_be; 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 unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP; break; @@ -2145,12 +2170,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) goto unlock_be; - if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) + be->dpcm[stream].be_start--; + if (be->dpcm[stream].be_start != 0) goto unlock_be; ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start++; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND; break; @@ -2158,12 +2186,15 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) goto unlock_be; - if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) + be->dpcm[stream].be_start--; + if (be->dpcm[stream].be_start != 0) goto unlock_be; ret = soc_pcm_trigger(be_substream, cmd); - if (ret) + if (ret) { + be->dpcm[stream].be_start++; goto unlock_be; + } be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED; break; -- 2.25.1
next prev parent reply other threads:[~2021-10-13 14:33 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-13 14:30 [RFC PATCH v3 00/13] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 01/13] ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 02/13] ASoC: soc-pcm: don't export local functions, use static Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 03/13] ASoC: soc-pcm: use proper indentation on 'continue' Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 04/13] ASoC: soc-pcm: introduce snd_soc_dpcm_fe_lock_irq/unlock_irq() Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-15 6:24 ` Sameer Pujar 2021-10-15 6:24 ` Sameer Pujar 2021-10-15 12:24 ` Pierre-Louis Bossart 2021-10-15 12:24 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 05/13] ASoC: soc-pcm: align BE 'atomicity' with that of the FE Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-15 6:24 ` Sameer Pujar 2021-10-15 6:24 ` Sameer Pujar 2021-10-15 7:39 ` Takashi Iwai 2021-10-15 7:39 ` Takashi Iwai 2021-10-15 11:22 ` Pierre-Louis Bossart 2021-10-15 11:22 ` Pierre-Louis Bossart 2021-10-15 12:04 ` Pierre-Louis Bossart 2021-10-15 12:04 ` Pierre-Louis Bossart 2021-10-15 15:38 ` Takashi Iwai 2021-10-15 15:38 ` Takashi Iwai 2021-10-15 16:22 ` Pierre-Louis Bossart 2021-10-15 16:22 ` Pierre-Louis Bossart 2021-10-15 16:56 ` Takashi Iwai 2021-10-15 16:56 ` Takashi Iwai 2021-10-15 17:08 ` Pierre-Louis Bossart 2021-10-15 17:08 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 06/13] ASoC: soc-pcm: remove dpcm spin_lock, use PCM stream lock Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 07/13] ASoC: soc-pcm: protect for_each_dpcm_be() loops Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-15 6:24 ` Sameer Pujar 2021-10-15 6:24 ` Sameer Pujar 2021-10-15 11:02 ` Pierre-Louis Bossart 2021-10-15 11:02 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 08/13] ASoC: soc-compress: " Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 09/13] ASoC: sh: rcar: " Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 10/13] ASoC: fsl: asrc_dma: " Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 11/13] ASoC: soc-pcm: serialize BE triggers Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart [this message] 2021-10-13 14:30 ` [RFC PATCH v3 12/13] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart 2021-10-13 14:30 ` [RFC PATCH v3 13/13] ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE Pierre-Louis Bossart 2021-10-13 14:30 ` Pierre-Louis Bossart
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211013143050.244444-13-pierre-louis.bossart@linux.intel.com \ --to=pierre-louis.bossart@linux.intel.com \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=gt82.lee@samsung.com \ --cc=kuninori.morimoto.gx@renesas.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=peter.ujfalusi@linux.intel.com \ --cc=spujar@nvidia.com \ --cc=tiwai@suse.com \ --cc=tiwai@suse.de \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.