[RFC PATCH v3 00/13] ASoC : soc-pcm: fix trigger race conditions with shared BE
We've been adding a 'deep buffer' PCM device to several SOF topologies in order to reduce power consumption. The typical use-case would be music playback over a headset: this additional PCM device provides more buffering and longer latencies, leaving the rest of the system sleep for longer periods. Notifications and 'regular' low-latency audio playback would still use the 'normal' PCM device and be mixed with the 'deep buffer' before rendering on the headphone endpoint. The tentative direction would be to expose this alternate device to PulseAudio/PipeWire/CRAS via the UCM SectionModifier definitions.
That seemed a straightforward topology change until our automated validation stress tests started reporting issues on SoundWire platforms, when e.g. two START triggers might be send and conversely the STOP trigger is never sent. The SoundWire stream state management flagged inconsistent states when the two 'normal' and 'deep buffer' devices are used concurrently with rapid play/stop/pause monkey testing.
Looking at the soc-pcm.c code, it seems that the BE state management needs a lot of love.
a) there is no consistent protection for the BE state. In some parts of the code, the state updates are protected by a spinlock but in the trigger they are not. When we open/play/close the two PCM devices in stress tests, we end-up testing a state that is being modified. That can't be good.
b) there is a conceptual deadlock: on stop we check the FE states to see if a shared BE can be stopped, but since we trigger the BE first the FE states have not been modified yet, so the TRIGGER_STOP is never sent.
This patchset suggests the removal of the dedicated 'dpcm_lock', and the use of the FE PCM lock before walking through the BE list, a mutual exclusion between triggers using the BE PCM lock, and the use of a refcount to decide when to trigger the BE. With these patches I am able to run our entire validation suite without any issues with this new 'deep buffer' topology, and no regressions on existing solutions [1]
One might ask 'how come we didn't see this earlier'? The answer is probably that the .trigger callbacks in most implementations seems to perform DAPM operations, and sending the triggers multiple times is not an issue. In the case of SoundWire, we do use the .trigger callback to reconfigure the bus using the 'bank switch' mechanism. It could be acceptable to tolerate a trigger multiple times, but the deadlock on stop cannot be fixed at the SoundWire layer alone.
I chose to send this patchset as an RFCv3 to gather more feedback and make use others know about DPCM issues. We're going to spend more time on this but if others can provide feedback/test results it would be greatly appreciated. The change in the locking model could be problematic on other platforms so we do want more time to comment/test before even considering a merge.
Opens:
1) is this the right solution? The DPCM code is far from simple, has notions such as SND_SOC_DPCM_UPDATE_NO and 'trigger_pending' that I have no background on. It's not clear if these cases are still needed with the locking changes.
2) There are other reports of kernel oopses [2] that seem related to the lack of protection. I'd be good to confirm if this patchset solve these problems as well.
[1] https://github.com/thesofproject/linux/pull/3146 [2] https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.c...
changes since RFCv2: Removal of dpcm_lock to use FE PCM locks (credits to Takashi Iwai for the suggestion). The FE PCM lock is now used before each use of for_each_dpcm_be() - with the exception of the trigger where the lock is already taken. This change is also applied in drivers which make use of this loop (compress, SH, FSL). Addition of BE PCM lock to deal with mutual exclusion between triggers for the same BE. Alignment of the BE atomicity on the FE on connections, this is required to avoid sleeping in atomic context. Additional cleanups (indentation, static functions)
changes since RFC v1: Removed unused function Removed exported symbols only used in soc-pcm.c, used static instead Use a mutex instead of a spinlock Protect all for_each_dpcm_be() loops Fix bugs introduced in the refcount
Pierre-Louis Bossart (13): ASoC: soc-pcm: remove snd_soc_dpcm_fe_can_update() ASoC: soc-pcm: don't export local functions, use static ASoC: soc-pcm: use proper indentation on 'continue' ASoC: soc-pcm: introduce snd_soc_dpcm_fe_lock_irq/unlock_irq() ASoC: soc-pcm: align BE 'atomicity' with that of the FE ASoC: soc-pcm: remove dpcm spin_lock, use PCM stream lock ASoC: soc-pcm: protect for_each_dpcm_be() loops ASoC: soc-compress: protect for_each_dpcm_be() loops ASoC: sh: rcar: protect for_each_dpcm_be() loops ASoC: fsl: asrc_dma: protect for_each_dpcm_be() loops ASoC: soc-pcm: serialize BE triggers ASoC: soc-pcm: test refcount before triggering ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE
include/sound/soc-dpcm.h | 20 +-- include/sound/soc.h | 2 - sound/soc/fsl/fsl_asrc_dma.c | 2 + sound/soc/sh/rcar/core.c | 2 + sound/soc/soc-compress.c | 4 + sound/soc/soc-core.c | 1 - sound/soc/soc-pcm.c | 253 ++++++++++++++++++++++++----------- 7 files changed, 187 insertions(+), 97 deletions(-)
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 0aa0ae9703d1..2790379015ca 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2815,15 +2815,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)
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 2790379015ca..d428a8815d39 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"; @@ -2816,7 +2828,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) || @@ -2825,7 +2837,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 * @@ -2871,7 +2882,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[] = { @@ -2882,13 +2893,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[] = { @@ -2900,4 +2910,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);
Minor realignment before more changes.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index d428a8815d39..19539300d94d 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1856,7 +1856,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)
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@linux.intel.com --- 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;
On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
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@linux.intel.com
include/sound/soc-dpcm.h | 3 +++ sound/soc/soc-pcm.c | 42 +++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 18 deletions(-)
1. Till this patch and with DEBUG_LOCKDEP config enabled, I see following warning: "WARNING: CPU: 0 PID: 0 at kernel/locking/irqflag-debug.c:10 warn_bogus_irq_restore+0x30/0x40"
However test passed though. Interestingly it was seen only for the first time I ran a 2x1 mixer test.
2. Also after I load sound modules and card gets registered, I see some hw_param() calls for FE and BE. This seems harmless at this point, but is getting problematic with subsequent patches. This was not happening earier.
On 10/15/21 1:24 AM, Sameer Pujar wrote:
On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
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@linux.intel.com
include/sound/soc-dpcm.h | 3 +++ sound/soc/soc-pcm.c | 42 +++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 18 deletions(-)
- Till this patch and with DEBUG_LOCKDEP config enabled, I see
Did you mean "with this patch included", yes?
following warning: "WARNING: CPU: 0 PID: 0 at kernel/locking/irqflag-debug.c:10 warn_bogus_irq_restore+0x30/0x40"
However test passed though. Interestingly it was seen only for the first time I ran a 2x1 mixer test.
- Also after I load sound modules and card gets registered, I see some
hw_param() calls for FE and BE. This seems harmless at this point, but is getting problematic with subsequent patches. This was not happening earier.
This patch doesn't change any of the flow, it just adds a wrapper in preparation for the transition to the FE pcm lock.
The only change is that we use spin_lock_irq instead of the irqsave/restore version, but that was also Takashi's recommendation.
the test results would suggest that on Tegra DPCM functions are used from an interrupt context?
Since the flow for DPCM is based on taking a lock for the FE first, we need to make sure during the connection between a BE and an FE that they both use the same 'atomicity', otherwise we may sleep in atomic context.
If the FE is nonatomic, this patch forces the BE to be nonatomic as well. That should have no negative impact since the BE 'inherits' the FE properties.
However, if the FE is atomic and the BE is not, then the configuration is flagged as invalid.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-pcm.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 52851827d53f..f22bbf95319d 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1151,13 +1151,33 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe, struct snd_soc_pcm_runtime *be, int stream) { + struct snd_pcm_substream *fe_substream; + struct snd_pcm_substream *be_substream; struct snd_soc_dpcm *dpcm;
/* only add new dpcms */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { - if (dpcm->be == be && dpcm->fe == fe) + if (dpcm->be == be && dpcm->fe == fe) { + snd_soc_dpcm_fe_unlock_irq(fe, stream); return 0; + } + } + fe_substream = snd_soc_dpcm_get_substream(fe, stream); + be_substream = snd_soc_dpcm_get_substream(be, stream); + + if (!fe_substream->pcm->nonatomic && be_substream->pcm->nonatomic) { + dev_err(be->dev, "%s: FE is atomic but BE is nonatomic, invalid configuration\n", + __func__); + snd_soc_dpcm_fe_unlock_irq(fe, stream); + return -EINVAL; } + if (fe_substream->pcm->nonatomic && !be_substream->pcm->nonatomic) { + dev_warn(be->dev, "%s: FE is nonatomic but BE is not, forcing BE as nonatomic\n", + __func__); + be_substream->pcm->nonatomic = 1; + } + snd_soc_dpcm_fe_unlock_irq(fe, stream);
dpcm = kzalloc(sizeof(struct snd_soc_dpcm), GFP_KERNEL); if (!dpcm)
On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
Since the flow for DPCM is based on taking a lock for the FE first, we need to make sure during the connection between a BE and an FE that they both use the same 'atomicity', otherwise we may sleep in atomic context.
If the FE is nonatomic, this patch forces the BE to be nonatomic as well. That should have no negative impact since the BE 'inherits' the FE properties.
However, if the FE is atomic and the BE is not, then the configuration is flagged as invalid.
In normal PCM, atomicity seems to apply only for trigger(). Other callbacks like prepare, hw_params are executed in non-atomic context. So when 'nonatomic' flag is false, still it is possible to sleep in a prepare or hw_param callback and this is true for FE as well. So I am not sure if atomicity is applicable as a whole even for FE.
At this point it does not cause serious problems, but with subsequent patches (especially when patch 7/13 is picked) I see failures. Please refer to patch 7/13 thread for more details.
I am wondering if it is possible to only use locks internally for DPCM state management and decouple BE callbacks from this, like normal PCMs do?
On Fri, 15 Oct 2021 08:24:41 +0200, Sameer Pujar wrote:
On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
Since the flow for DPCM is based on taking a lock for the FE first, we need to make sure during the connection between a BE and an FE that they both use the same 'atomicity', otherwise we may sleep in atomic context.
If the FE is nonatomic, this patch forces the BE to be nonatomic as well. That should have no negative impact since the BE 'inherits' the FE properties.
However, if the FE is atomic and the BE is not, then the configuration is flagged as invalid.
In normal PCM, atomicity seems to apply only for trigger(). Other callbacks like prepare, hw_params are executed in non-atomic context. So when 'nonatomic' flag is false, still it is possible to sleep in a prepare or hw_param callback and this is true for FE as well. So I am not sure if atomicity is applicable as a whole even for FE.
At this point it does not cause serious problems, but with subsequent patches (especially when patch 7/13 is picked) I see failures. Please refer to patch 7/13 thread for more details.
I am wondering if it is possible to only use locks internally for DPCM state management and decouple BE callbacks from this, like normal PCMs do?
Actually the patch looks like an overkill by adding the FE stream lock at every loop, and this caused the problem, AFAIU.
Basically we need to protect the link addition / deletion while the list traversal (there is a need for protection of BE vs BE access race, but that's a different code path). For the normal cases, it seems already protected by card->pcm_mutex, but the problem is the FE trigger case. It was attempted by dpcm_lock, but that failed because it couldn't be applied properly there.
That said, what we'd need is only: - Drop dpcm_lock codes once - Put FE stream lock around dpcm_be_connect() and dpcm_be_disconnect()
That should suffice for the race at trigger. The FE stream lock is already taken at trigger callback, and the rest list addition / deletion are called from different code paths without stream locks, so the explicit FE stream lock is needed there.
In addition, a lock around dpcm_show_state() might be needed to be replaced with card->pcm_mutex, and we may need to revisit whether all other paths take card->pcm_mutex.
Also, BE-vs-BE race can be protected by taking a BE lock inside dpcm_be_dai_trigger().
Takashi
In normal PCM, atomicity seems to apply only for trigger(). Other callbacks like prepare, hw_params are executed in non-atomic context. So when 'nonatomic' flag is false, still it is possible to sleep in a prepare or hw_param callback and this is true for FE as well. So I am not sure if atomicity is applicable as a whole even for FE.
The typical path is snd_pcm_elapsed() on the FE, which will trigger the BE. When we add the BE lock in patch7, things break: what matters is the FE context, the locks used for the BE have to be aligned with the FE atomicity.
My test results show the problem: https://github.com/thesofproject/linux/pull/3209#issuecomment-941229502 and this patch fixes the issue.
I am all ears if someone has a better solution, but the problem is real.
I chose to add this patch first, before the BE lock is added in dpcm_be_dai_trigger(), and if this causes problems already there are even more issues in DPCM :-(
If this patch causes issues outside of the trigger phase, then maybe we could just change the BE nonatomic flag temporarily and restore it after taking the lock, but IMHO something else is broken in other drivers.
At this point it does not cause serious problems, but with subsequent patches (especially when patch 7/13 is picked) I see failures. Please refer to patch 7/13 thread for more details.
I am wondering if it is possible to only use locks internally for DPCM state management and decouple BE callbacks from this, like normal PCMs do?
Actually the patch looks like an overkill by adding the FE stream lock at every loop, and this caused the problem, AFAIU.
Basically we need to protect the link addition / deletion while the list traversal (there is a need for protection of BE vs BE access race, but that's a different code path). For the normal cases, it seems already protected by card->pcm_mutex, but the problem is the FE trigger case. It was attempted by dpcm_lock, but that failed because it couldn't be applied properly there.
That said, what we'd need is only:
- Drop dpcm_lock codes once
I am not able to follow this sentence, what did you mean here?
- Put FE stream lock around dpcm_be_connect() and dpcm_be_disconnect()
That should suffice for the race at trigger. The FE stream lock is already taken at trigger callback, and the rest list addition / deletion are called from different code paths without stream locks, so the explicit FE stream lock is needed there.
I am not able to follow what you meant after "the rest". This sentence mentions the FE stream lock in two places, but the second is not clear to me.
In addition, a lock around dpcm_show_state() might be needed to be replaced with card->pcm_mutex, and we may need to revisit whether all other paths take card->pcm_mutex.
What happens if we show the state while a trigger happens? That's my main concern with using two separate locks (pcm_mutex and FE stream lock) to protect the same list, there are still windows of time where the list is not protected.
same with the use of for_each_dpcm_be() in soc-compress, SH and FSL drivers, there's no other mutex there.
My approach might have been overkill in some places, but it's comprehensive.
Also, BE-vs-BE race can be protected by taking a BE lock inside dpcm_be_dai_trigger().
that's what I did, no?
On 10/15/21 6:22 AM, Pierre-Louis Bossart wrote:
If this patch causes issues outside of the trigger phase, then maybe we could just change the BE nonatomic flag temporarily and restore it after taking the lock, but IMHO something else is broken in other drivers.
Now that I think of it, this wouldn't work, the type of locks for the BEs has to be set before the trigger: the DPCM flow is that we start from the FEs and find which BEs need to be triggered. That would prevent us from modifying a global BE property - each FE is independent.
On Fri, 15 Oct 2021 13:22:50 +0200, Pierre-Louis Bossart wrote:
At this point it does not cause serious problems, but with subsequent patches (especially when patch 7/13 is picked) I see failures. Please refer to patch 7/13 thread for more details.
I am wondering if it is possible to only use locks internally for DPCM state management and decouple BE callbacks from this, like normal PCMs do?
Actually the patch looks like an overkill by adding the FE stream lock at every loop, and this caused the problem, AFAIU.
Basically we need to protect the link addition / deletion while the list traversal (there is a need for protection of BE vs BE access race, but that's a different code path). For the normal cases, it seems already protected by card->pcm_mutex, but the problem is the FE trigger case. It was attempted by dpcm_lock, but that failed because it couldn't be applied properly there.
That said, what we'd need is only:
- Drop dpcm_lock codes once
I am not able to follow this sentence, what did you mean here?
Just remove all dpcm_lock usages one to replace with a new one.
- Put FE stream lock around dpcm_be_connect() and dpcm_be_disconnect()
That should suffice for the race at trigger. The FE stream lock is already taken at trigger callback, and the rest list addition / deletion are called from different code paths without stream locks, so the explicit FE stream lock is needed there.
I am not able to follow what you meant after "the rest". This sentence mentions the FE stream lock in two places, but the second is not clear to me.
The FE stream locks are necessary only two points: at adding and deleting the BE in the link. We used dpcm_lock in other places, but those are superfluous or would make problem if converted to a FE stream lock.
In addition, a lock around dpcm_show_state() might be needed to be replaced with card->pcm_mutex, and we may need to revisit whether all other paths take card->pcm_mutex.
What happens if we show the state while a trigger happens? That's my main concern with using two separate locks (pcm_mutex and FE stream lock) to protect the same list, there are still windows of time where the list is not protected.
With the proper use of mutex, the list itself is protected. If we need to protect the concurrent access to each BE in the show method, an additional BE lock is needed in that part. But that's a subtle issue, as the link traversal itself is protected by the mutex.
same with the use of for_each_dpcm_be() in soc-compress, SH and FSL drivers, there's no other mutex there.
My approach might have been overkill in some places, but it's comprehensive.
Also, BE-vs-BE race can be protected by taking a BE lock inside dpcm_be_dai_trigger().
that's what I did, no?
Right.
So what I wrote is like the patches below. Those three should be applicable on top of the latest Linus tree. It's merely a PoC, and it doesn't take the compress-offload usage into account at all, but this should illustrate my idea.
Takashi
The FE stream locks are necessary only two points: at adding and deleting the BE in the link. We used dpcm_lock in other places, but those are superfluous or would make problem if converted to a FE stream lock.
I must be missing a fundamental concept here - possibly a set of concepts...
It is my understanding that the FE-BE connection can be updated dynamically without any relationship to the usual ALSA steps, e.g. as a result of a control being changed by a user.
So if you only protect the addition/removal, isn't there a case where the for_each_dpcm_be() loop would either miss a BE or point to an invalid one?
In other words, don't we need the *same* lock to be used a) before changing and b) walking through the list?
I also don't get what would happen if the dpcm_lock was converted to an FE stream lock. It works fine in my tests, so if there's limitation I didn't see it.
In addition, a lock around dpcm_show_state() might be needed to be replaced with card->pcm_mutex, and we may need to revisit whether all other paths take card->pcm_mutex.
What happens if we show the state while a trigger happens? That's my main concern with using two separate locks (pcm_mutex and FE stream lock) to protect the same list, there are still windows of time where the list is not protected.
With the proper use of mutex, the list itself is protected. If we need to protect the concurrent access to each BE in the show method, an additional BE lock is needed in that part. But that's a subtle issue, as the link traversal itself is protected by the mutex.
If I look at your patch2, dpcm_be_disconnect() protects the list removal with the fe stream lock, but the show state is protected by both the pcm_mutex and the fe stream lock.
I have not been able to figure out when you need a) the pcm_mutex only b) the fe stream lock only c) both pcm_mutex and fe stream lock
On Fri, 15 Oct 2021 18:22:58 +0200, Pierre-Louis Bossart wrote:
The FE stream locks are necessary only two points: at adding and deleting the BE in the link. We used dpcm_lock in other places, but those are superfluous or would make problem if converted to a FE stream lock.
I must be missing a fundamental concept here - possibly a set of concepts...
It is my understanding that the FE-BE connection can be updated dynamically without any relationship to the usual ALSA steps, e.g. as a result of a control being changed by a user.
So if you only protect the addition/removal, isn't there a case where the for_each_dpcm_be() loop would either miss a BE or point to an invalid one?
No, for sleepable context, pcm_mutex is *always* taken when adding/deleting a BE, and that's the main protection for the link. The BE stream lock is taken additionally over it at adding/deleting a BE, just for the code path via FE and BE trigger.
In other words, don't we need the *same* lock to be used a) before changing and b) walking through the list?
I also don't get what would happen if the dpcm_lock was converted to an FE stream lock. It works fine in my tests, so if there's limitation I didn't see it.
dpcm_lock was put in the places that could be recursively taken. So this caused some deadlock, I suppose.
In addition, a lock around dpcm_show_state() might be needed to be replaced with card->pcm_mutex, and we may need to revisit whether all other paths take card->pcm_mutex.
What happens if we show the state while a trigger happens? That's my main concern with using two separate locks (pcm_mutex and FE stream lock) to protect the same list, there are still windows of time where the list is not protected.
With the proper use of mutex, the list itself is protected. If we need to protect the concurrent access to each BE in the show method, an additional BE lock is needed in that part. But that's a subtle issue, as the link traversal itself is protected by the mutex.
If I look at your patch2, dpcm_be_disconnect() protects the list removal with the fe stream lock, but the show state is protected by both the pcm_mutex and the fe stream lock.
No, show_state() itself doesn't take any lock, but its caller dpcm_state_read_file() takes the pcm_mutex. That protects the list addition / deletion.
I have not been able to figure out when you need a) the pcm_mutex only b) the fe stream lock only c) both pcm_mutex and fe stream lock
The pcm_mutex is needed for every sleepable function that treat DPCM FE link, but the mutex is taken only at the upper level, i.e. the top-most caller like PCM ops FE itself or the DAPM calls.
That said, pcm_mutex is the top-most protection of BE links in FE. But, there is a code path where a mutex can't be used, and that's the FE and BE trigger. For protecting against this, the FE stream lock is taken only at the placing both adding and deleting a BE *in addition*. At those places, both pcm_mutex and FE stream lock are taken.
BE stream lock is taken in addition below the above mutex and FE locks.
Takashi
I have not been able to figure out when you need a) the pcm_mutex only b) the fe stream lock only c) both pcm_mutex and fe stream lock
The pcm_mutex is needed for every sleepable function that treat DPCM FE link, but the mutex is taken only at the upper level, i.e. the top-most caller like PCM ops FE itself or the DAPM calls.
That said, pcm_mutex is the top-most protection of BE links in FE. But, there is a code path where a mutex can't be used, and that's the FE and BE trigger. For protecting against this, the FE stream lock is taken only at the placing both adding and deleting a BE *in addition*. At those places, both pcm_mutex and FE stream lock are taken.
BE stream lock is taken in addition below the above mutex and FE locks.
Thanks Takashi, now I get the idea. Makes sense indeed. I'll make sure to add this explanation to the commit message/cover so that it's not lost.
I added your three patches to my tests, so far so good, code is that https://github.com/thesofproject/linux/pull/3215
Thanks and have a nice week-end. -Pierre
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 locks will rely on either a spin-lock or a mutex depending on atomicity.
Since the FE PCM lock is already taken during the trigger, new _locked versions of the helpers snd_soc_dpcm_can_be_free_stop() and snd_soc_dpcm_check_state() are introduced. Without these changes a conceptual deadlock happens on TRIGGER_STOP.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/sound/soc.h | 2 -- sound/soc/soc-core.c | 1 - sound/soc/soc-pcm.c | 55 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 44 insertions(+), 14 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 80ca260595fd..b029e07ad1e1 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 f22bbf95319d..797f0d114c83 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);
@@ -45,6 +45,9 @@ EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_unlock_irq); static int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe, struct snd_soc_pcm_runtime *be, int stream);
+static int snd_soc_dpcm_can_be_free_stop_locked(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); @@ -2101,7 +2104,7 @@ 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 (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) continue;
ret = soc_pcm_trigger(be_substream, cmd); @@ -2114,7 +2117,7 @@ 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)) + if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) continue;
ret = soc_pcm_trigger(be_substream, cmd); @@ -2127,7 +2130,7 @@ 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)) + if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) continue;
ret = soc_pcm_trigger(be_substream, cmd); @@ -2873,18 +2876,17 @@ struct snd_pcm_substream * } EXPORT_SYMBOL_GPL(snd_soc_dpcm_get_substream);
-static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe, - struct snd_soc_pcm_runtime *be, - int stream, - const enum snd_soc_dpcm_state *states, - int num_states) +static int snd_soc_dpcm_check_state_locked(struct snd_soc_pcm_runtime *fe, + struct snd_soc_pcm_runtime *be, + int stream, + const enum snd_soc_dpcm_state *states, + int num_states) { struct snd_soc_dpcm *dpcm; int state; int ret = 1; int i;
- snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe) @@ -2898,12 +2900,43 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe, } } } + + /* it's safe to do this BE DAI */ + return ret; +} + +static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe, + struct snd_soc_pcm_runtime *be, + int stream, + const enum snd_soc_dpcm_state *states, + int num_states) +{ + int ret; + + snd_soc_dpcm_fe_lock_irq(fe, stream); + ret = snd_soc_dpcm_check_state_locked(fe, be, stream, states, num_states); snd_soc_dpcm_fe_unlock_irq(fe, stream);
/* it's safe to do this BE DAI */ return ret; }
+/* + * 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. + */ +static int snd_soc_dpcm_can_be_free_stop_locked(struct snd_soc_pcm_runtime *fe, + struct snd_soc_pcm_runtime *be, int stream) +{ + const enum snd_soc_dpcm_state state[] = { + SND_SOC_DPCM_STATE_START, + SND_SOC_DPCM_STATE_PAUSED, + SND_SOC_DPCM_STATE_SUSPEND, + }; + + return snd_soc_dpcm_check_state_locked(fe, be, stream, state, ARRAY_SIZE(state)); +} + /* * 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.
The D in DPCM stands for 'dynamic', which means that connections between FE and BE can evolve.
Commit a97648697790 ("ASoC: dpcm: prevent snd_soc_dpcm use after free") started to protect some of the for_each_dpcm_be() loops, but there are still many cases that were not modified.
This patch adds protection for all the remaining loops, with the notable exception of the dpcm_be_dai_trigger(), where the lock is already taken at a higher level, e.g. in snd_pcm_period_elapsed().
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-pcm.c | 86 ++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 47 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 797f0d114c83..5f2368059e14 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -42,15 +42,12 @@ void snd_soc_dpcm_fe_unlock_irq(struct snd_soc_pcm_runtime *fe, int stream) 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); - static int snd_soc_dpcm_can_be_free_stop_locked(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); +static int snd_soc_dpcm_can_be_params_locked(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, @@ -335,6 +332,7 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir, { struct snd_soc_dpcm *dpcm;
+ snd_soc_dpcm_fe_lock_irq(fe, dir); for_each_dpcm_be(fe, dir, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be; @@ -348,6 +346,8 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
snd_soc_dapm_stream_event(be, dir, event); } + snd_soc_dpcm_fe_unlock_irq(fe, dir); +
snd_soc_dapm_stream_event(fe, dir, event);
@@ -1237,6 +1237,7 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream) { struct snd_soc_dpcm *dpcm, *d;
+ snd_soc_dpcm_fe_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", @@ -1254,12 +1255,11 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
dpcm_remove_debugfs_state(dpcm);
- snd_soc_dpcm_fe_lock_irq(fe, stream); list_del(&dpcm->list_be); list_del(&dpcm->list_fe); - snd_soc_dpcm_fe_unlock_irq(fe, stream); kfree(dpcm); } + snd_soc_dpcm_fe_unlock_irq(fe, stream); }
/* get BE for DAI widget and stream */ @@ -1386,6 +1386,7 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream, int prune = 0;
/* Destroy any old FE <--> BE connections */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { if (dpcm_be_is_active(dpcm, stream, *list_)) continue; @@ -1397,6 +1398,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++; } + snd_soc_dpcm_fe_unlock_irq(fe, stream);
dev_dbg(fe->dev, "ASoC: found %d old BE paths for pruning\n", prune); return prune; @@ -1496,13 +1498,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 */ + snd_soc_dpcm_fe_lock_irq(fe, stream); 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) { + snd_soc_dpcm_fe_unlock_irq(fe, stream); return; + }
/* is this op for this BE ? */ if (!snd_soc_dpcm_be_can_update(fe, be, stream)) @@ -1532,6 +1537,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; } + snd_soc_dpcm_fe_unlock_irq(fe, stream); }
int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream) @@ -1541,6 +1547,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 */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_pcm_substream *be_substream;
@@ -1591,11 +1598,13 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream) be->dpcm[stream].state = SND_SOC_DPCM_STATE_OPEN; count++; } + snd_soc_dpcm_fe_unlock_irq(fe, stream);
return count;
unwind: dpcm_be_dai_startup_rollback(fe, stream, dpcm); + snd_soc_dpcm_fe_unlock_irq(fe, stream);
dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n", __func__, be->dai_link->name, err); @@ -1650,6 +1659,7 @@ static void dpcm_runtime_setup_be_format(struct snd_pcm_substream *substream) * if FE want to use it (= dpcm_merged_format) */
+ snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be; struct snd_soc_pcm_stream *codec_stream; @@ -1668,6 +1678,7 @@ static void dpcm_runtime_setup_be_format(struct snd_pcm_substream *substream) soc_pcm_hw_update_format(hw, codec_stream); } } + snd_soc_dpcm_fe_unlock_irq(fe, stream); }
static void dpcm_runtime_setup_be_chan(struct snd_pcm_substream *substream) @@ -1685,7 +1696,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) */ - + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be; struct snd_soc_pcm_stream *cpu_stream; @@ -1716,6 +1727,7 @@ static void dpcm_runtime_setup_be_chan(struct snd_pcm_substream *substream) soc_pcm_hw_update_chan(hw, codec_stream); } } + snd_soc_dpcm_fe_unlock_irq(fe, stream); }
static void dpcm_runtime_setup_be_rate(struct snd_pcm_substream *substream) @@ -1733,7 +1745,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) */ - + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be; struct snd_soc_pcm_stream *pcm; @@ -1753,6 +1765,7 @@ static void dpcm_runtime_setup_be_rate(struct snd_pcm_substream *substream) soc_pcm_hw_update_rate(hw, pcm); } } + snd_soc_dpcm_fe_unlock_irq(fe, stream); }
static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, @@ -1775,6 +1788,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, }
/* apply symmetry for BE */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be; struct snd_pcm_substream *be_substream = @@ -1800,6 +1814,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, } } error: + snd_soc_dpcm_fe_unlock_irq(fe, stream); if (err < 0) dev_err(fe->dev, "ASoC: %s failed (%d)\n", __func__, err);
@@ -1875,6 +1890,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 */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be; @@ -1886,7 +1902,7 @@ void dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream) continue;
/* only free hw when no longer used - check all FEs */ - if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream)) + if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) continue;
/* do not free hw if this BE is used by other FE */ @@ -1908,6 +1924,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; } + snd_soc_dpcm_fe_unlock_irq(fe, stream); }
static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream) @@ -1941,6 +1958,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) struct snd_soc_dpcm *dpcm; int ret;
+ snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { be = dpcm->be; be_substream = snd_soc_dpcm_get_substream(be, stream); @@ -1963,7 +1981,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) sizeof(struct snd_pcm_hw_params));
/* only allow hw_params() if no connected FEs are running */ - if (!snd_soc_dpcm_can_be_params(fe, be, stream)) + if (!snd_soc_dpcm_can_be_params_locked(fe, be, stream)) continue;
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN) && @@ -1980,6 +1998,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; } + snd_soc_dpcm_fe_unlock_irq(fe, stream); return 0;
unwind: @@ -1995,7 +2014,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) continue;
/* only allow hw_free() if no connected FEs are running */ - if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream)) + if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) continue;
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_OPEN) && @@ -2006,6 +2025,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
soc_pcm_hw_free(be_substream); } + snd_soc_dpcm_fe_unlock_irq(fe, stream);
return ret; } @@ -2290,6 +2310,7 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream) struct snd_soc_dpcm *dpcm; int ret = 0;
+ snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be; @@ -2315,6 +2336,7 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream)
be->dpcm[stream].state = SND_SOC_DPCM_STATE_PREPARE; } + snd_soc_dpcm_fe_unlock_irq(fe, stream);
if (ret < 0) dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret); @@ -2588,8 +2610,10 @@ static void dpcm_fe_dai_cleanup(struct snd_pcm_substream *fe_substream) int stream = fe_substream->stream;
/* mark FE's links ready to prune */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; + snd_soc_dpcm_fe_unlock_irq(fe, stream);
dpcm_be_disconnect(fe, stream);
@@ -2905,22 +2929,6 @@ static int snd_soc_dpcm_check_state_locked(struct snd_soc_pcm_runtime *fe, return ret; }
-static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe, - struct snd_soc_pcm_runtime *be, - int stream, - const enum snd_soc_dpcm_state *states, - int num_states) -{ - int ret; - - snd_soc_dpcm_fe_lock_irq(fe, stream); - ret = snd_soc_dpcm_check_state_locked(fe, be, stream, states, num_states); - snd_soc_dpcm_fe_unlock_irq(fe, stream); - - /* it's safe to do this BE DAI */ - return ret; -} - /* * 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. @@ -2937,27 +2945,11 @@ static int snd_soc_dpcm_can_be_free_stop_locked(struct snd_soc_pcm_runtime *fe, return snd_soc_dpcm_check_state_locked(fe, be, stream, state, ARRAY_SIZE(state)); }
-/* - * 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. - */ -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[] = { - SND_SOC_DPCM_STATE_START, - SND_SOC_DPCM_STATE_PAUSED, - SND_SOC_DPCM_STATE_SUSPEND, - }; - - return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state)); -} - /* * 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. */ -static int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe, +static int snd_soc_dpcm_can_be_params_locked(struct snd_soc_pcm_runtime *fe, struct snd_soc_pcm_runtime *be, int stream) { const enum snd_soc_dpcm_state state[] = { @@ -2967,5 +2959,5 @@ static int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe, SND_SOC_DPCM_STATE_PREPARE, };
- return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state)); + return snd_soc_dpcm_check_state_locked(fe, be, stream, state, ARRAY_SIZE(state)); }
On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
The D in DPCM stands for 'dynamic', which means that connections between FE and BE can evolve.
Commit a97648697790 ("ASoC: dpcm: prevent snd_soc_dpcm use after free") started to protect some of the for_each_dpcm_be() loops, but there are still many cases that were not modified.
This patch adds protection for all the remaining loops, with the notable exception of the dpcm_be_dai_trigger(), where the lock is already taken at a higher level, e.g. in snd_pcm_period_elapsed().
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/soc-pcm.c | 86 ++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 47 deletions(-)
After this, once I load sound card there are warning prints and failure:
[ 71.224324] WARNING: CPU: 3 PID: 574 at drivers/firmware/tegra/bpmp.c:362 tegra_bpmp_transfer+0x2d0/0x328 [ 71.238032] ---[ end trace 88d978f78a82134f ]--- [ 71.243033] WARNING: CPU: 3 PID: 574 at drivers/firmware/tegra/bpmp.c:362 tegra_bpmp_transfer+0x2d0/0x328 [ 71.257022] ---[ end trace 88d978f78a821350 ]--- [ 71.261965] tegra-audio-graph-card sound: Can't set plla rate for 270950400, err: -22 ...
This happens because, now the atomicity is propagated to BE callbacks where the clock settings are done in hw_param(). On Tegra, the clock APIs are served by BPMP and warning is seen because of below.
int tegra_bpmp_transfer() {
=> if (WARN_ON(irqs_disabled())) return -EPERM;
...
}
This results in hw_param() failure and all tests fail at my end.
On 10/15/21 1:24 AM, Sameer Pujar wrote:
On 10/13/2021 8:00 PM, Pierre-Louis Bossart wrote:
The D in DPCM stands for 'dynamic', which means that connections between FE and BE can evolve.
Commit a97648697790 ("ASoC: dpcm: prevent snd_soc_dpcm use after free") started to protect some of the for_each_dpcm_be() loops, but there are still many cases that were not modified.
This patch adds protection for all the remaining loops, with the notable exception of the dpcm_be_dai_trigger(), where the lock is already taken at a higher level, e.g. in snd_pcm_period_elapsed().
Signed-off-by: Pierre-Louis Bossart
pierre-louis.bossart@linux.intel.com
sound/soc/soc-pcm.c | 86 ++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 47 deletions(-)
After this, once I load sound card there are warning prints and failure:
[ 71.224324] WARNING: CPU: 3 PID: 574 at drivers/firmware/tegra/bpmp.c:362 tegra_bpmp_transfer+0x2d0/0x328 [ 71.238032] ---[ end trace 88d978f78a82134f ]--- [ 71.243033] WARNING: CPU: 3 PID: 574 at drivers/firmware/tegra/bpmp.c:362 tegra_bpmp_transfer+0x2d0/0x328 [ 71.257022] ---[ end trace 88d978f78a821350 ]--- [ 71.261965] tegra-audio-graph-card sound: Can't set plla rate for 270950400, err: -22 ...
This happens because, now the atomicity is propagated to BE callbacks where the clock settings are done in hw_param(). On Tegra, the clock APIs are served by BPMP and warning is seen because of below.
Sorry, I don't understand this part.
if the FEs used on Tegra have the nonatomic property set to zero, nothing will be propagated really.
This patch was required on the Intel side, because ALL FEs are nonatomic but some BEs are not, so I had issues when connecting a nonatomic FE to an atomic BE. See e.g. https://github.com/thesofproject/linux/pull/3209#issuecomment-941229502
Follow the locking model used within soc-pcm.c
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-compress.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 8e2494a9f3a7..a1fc4083c88a 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -158,8 +158,10 @@ static int soc_compr_open_fe(struct snd_compr_stream *cstream) ret = dpcm_be_dai_startup(fe, stream); if (ret < 0) { /* clean up all links */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; + snd_soc_dpcm_fe_unlock_irq(fe, stream);
dpcm_be_disconnect(fe, stream); fe->dpcm[stream].runtime = NULL; @@ -224,8 +226,10 @@ static int soc_compr_free_fe(struct snd_compr_stream *cstream) dpcm_be_dai_shutdown(fe, stream);
/* mark FE's links ready to prune */ + snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; + snd_soc_dpcm_fe_unlock_irq(fe, stream);
dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
Follow the locking model used within soc-pcm.c
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sh/rcar/core.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 978bd0406729..ccd5c2d1cdc6 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1511,6 +1511,7 @@ static int rsnd_hw_params(struct snd_soc_component *component, struct snd_soc_dpcm *dpcm; int stream = substream->stream;
+ snd_soc_dpcm_fe_lock_irq(fe, stream); for_each_dpcm_be(fe, stream, dpcm) { struct snd_pcm_hw_params *be_params = &dpcm->hw_params;
@@ -1519,6 +1520,7 @@ static int rsnd_hw_params(struct snd_soc_component *component, if (params_rate(hw_params) != params_rate(be_params)) io->converted_rate = params_rate(be_params); } + snd_soc_dpcm_fe_unlock_irq(fe, stream); if (io->converted_chan) dev_dbg(dev, "convert channels = %d\n", io->converted_chan); if (io->converted_rate) {
Follow the locking model used within soc-pcm.c
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/fsl/fsl_asrc_dma.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c index cd9b36ec0ecb..b67097503836 100644 --- a/sound/soc/fsl/fsl_asrc_dma.c +++ b/sound/soc/fsl/fsl_asrc_dma.c @@ -151,6 +151,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component, int ret, width;
/* Fetch the Back-End dma_data from DPCM */ + snd_soc_dpcm_fe_lock_irq(rtd, stream); for_each_dpcm_be(rtd, stream, dpcm) { struct snd_soc_pcm_runtime *be = dpcm->be; struct snd_pcm_substream *substream_be; @@ -164,6 +165,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component, dev_be = dai->dev; break; } + snd_soc_dpcm_fe_unlock_irq(rtd, stream);
if (!dma_params_be) { dev_err(dev, "failed to get the substream of Back-End\n");
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.
This patch relies on the existing BE PCM lock, which takes atomicity into account. The locking model assumes that all interactions start with the FE, so that there is no deadlock between FE and BE locks.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-pcm.c | 49 +++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 5f2368059e14..d115e9409c14 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -41,6 +41,12 @@ void snd_soc_dpcm_fe_unlock_irq(struct snd_soc_pcm_runtime *fe, int stream) } EXPORT_SYMBOL_GPL(snd_soc_dpcm_fe_unlock_irq);
+#define snd_soc_dpcm_be_lock_irqsave(be, stream, flags) \ + snd_pcm_stream_lock_irqsave(snd_soc_dpcm_get_substream(be, stream), flags) + +#define snd_soc_dpcm_be_unlock_irqrestore(be, stream, flags) \ + snd_pcm_stream_unlock_irqrestore(snd_soc_dpcm_get_substream(be, stream), flags) + /* can this BE stop and free */ static int snd_soc_dpcm_can_be_free_stop_locked(struct snd_soc_pcm_runtime *fe, struct snd_soc_pcm_runtime *be, int stream); @@ -2071,6 +2077,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, { struct snd_soc_pcm_runtime *be; struct snd_soc_dpcm *dpcm; + unsigned long flags; int ret = 0;
for_each_dpcm_be(fe, stream, dpcm) { @@ -2086,85 +2093,89 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, dev_dbg(be->dev, "ASoC: trigger BE %s cmd %d\n", be->dai_link->name, cmd);
+ 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) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) - continue; + goto unlock_be;
ret = soc_pcm_trigger(be_substream, cmd); if (ret) - goto end; + goto unlock_be;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; case SNDRV_PCM_TRIGGER_RESUME: if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_SUSPEND)) - continue; + goto unlock_be;
ret = soc_pcm_trigger(be_substream, cmd); if (ret) - goto end; + goto unlock_be;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) - continue; + goto unlock_be;
ret = soc_pcm_trigger(be_substream, cmd); if (ret) - goto end; + goto unlock_be;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; case SNDRV_PCM_TRIGGER_STOP: if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) - continue; + goto unlock_be;
if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) - continue; + goto unlock_be;
ret = soc_pcm_trigger(be_substream, cmd); if (ret) - goto end; + goto unlock_be;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP; break; case SNDRV_PCM_TRIGGER_SUSPEND: if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) - continue; + goto unlock_be;
if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) - continue; + goto unlock_be;
ret = soc_pcm_trigger(be_substream, cmd); if (ret) - goto end; + goto unlock_be;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND; break; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) - continue; + goto unlock_be;
if (!snd_soc_dpcm_can_be_free_stop_locked(fe, be, stream)) - continue; + goto unlock_be;
ret = soc_pcm_trigger(be_substream, cmd); if (ret) - goto end; + goto unlock_be;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED; break; } +unlock_be: + snd_soc_dpcm_be_unlock_irqrestore(be, stream, flags); + if (ret < 0) { + dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n", + __func__, be->dai_link->name, ret); + break; + } } -end: - if (ret < 0) - dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n", - __func__, be->dai_link->name, ret); return ret; } EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
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;
A BE connected to more than one FE, e.g. in a mixer case, can go through the following transitions.
play FE1 -> BE state is START pause FE1 -> BE state is PAUSED play FE2 -> BE state is START stop FE2 -> BE state is STOP (see note [1] below) release FE1 -> BE state is START stop FE1 -> BE state is STOP
play FE1 -> BE state is START pause FE1 -> BE state is PAUSED play FE2 -> BE state is START release FE1 -> BE state is START stop FE2 -> BE state is START stop FE1 -> BE state is STOP
The existing code for PAUSE_RELEASE only allows for the case where the BE is paused, which clearly would not work in the sequences above.
Extend the allowed states to restart the BE when PAUSE_RELEASE is received.
[1] the existing logic does not move the BE state back to PAUSED when the FE2 is stopped. This patch does not change the logic; it would be painful to keep a history of changes on the FE side, the state machine is already rather complicated with transitions based on the last BE state and the trigger type.
Reported-by: Bard Liao bard.liao@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/soc-pcm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 1967980d0f79..88d60d5b60a8 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2131,7 +2131,9 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream, be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; break; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) + if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) && + (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++;
participants (3)
-
Pierre-Louis Bossart -
Sameer Pujar -
Takashi Iwai