* [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE
@ 2021-12-07 17:37 Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 1/6] ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure Pierre-Louis Bossart
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Kuninori Morimoto, tiwai, Sameer Pujar, linux-kernel,
Pierre-Louis Bossart, vkoul, broonie, Gyeongtaek Lee,
Peter Ujfalusi
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
follows the design suggested by Takashi Iwai. By default the
protection relies on the 'pcm_mutex', except for the FE and BE
triggers where the mutex cannot be used. In this case, the FE PCM
lock is used instead. In the cases where a BE is added/removed, the
pcm_mutex and FE PCM lock are both taken. In addition, the BE PCM
lock is used to serialize access to a shared 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]. The tests were reproduced by
Bard Liao for SoundWire devices.
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 level alone.
Opens:
1) The issues reported by Nvidia on the RFCv3 may or may not be
present. We'd need test results to make sure the locking update does
not introduce a regression on Tegra.
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.com/
changes since RFCv3:
Used two patches from Takashi. We now use the pcm_mutex, the FE stream
lock when adding and deleting a BE, and the BE stream lock to handle
concurrency between streams using the same BE.
Added a patch to use GFP_ATOMIC for the DPCM structure.
Fixed PAUSE_RELEASE transition (GitHub comment from Kai Vehmanen)
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 (4):
ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure
ASoC: soc-pcm: align BE 'atomicity' with that of the FE
ASoC: soc-pcm: test refcount before triggering
ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE
Takashi Iwai (2):
ASoC: soc-pcm: Fix and cleanup DPCM locking
ASoC: soc-pcm: serialize BE triggers
include/sound/soc-dpcm.h | 2 +
include/sound/soc.h | 2 -
sound/soc/soc-core.c | 1 -
sound/soc/soc-pcm.c | 351 +++++++++++++++++++++++++++------------
4 files changed, 246 insertions(+), 110 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/6] ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
@ 2021-12-07 17:37 ` Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE Pierre-Louis Bossart
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Guennadi Liakhovetski, Kai Vehmanen, Kuninori Morimoto,
Liam Girdwood, tiwai, Bard Liao, Sameer Pujar, Takashi Iwai,
linux-kernel, Pierre-Louis Bossart, vkoul, broonie,
Ranjani Sridharan, Gyeongtaek Lee, Peter Ujfalusi
We allocate a structure in dpcm_be_connect(), which may be called in
atomic context. Using GFP_KERNEL is not quite right, we have to use
GFP_ATOMIC to prevent the allocator from sleeping.
Suggested-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@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 3b4412183344..f66808dfb508 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1113,7 +1113,7 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
return 0;
}
- dpcm = kzalloc(sizeof(struct snd_soc_dpcm), GFP_KERNEL);
+ dpcm = kzalloc(sizeof(struct snd_soc_dpcm), GFP_ATOMIC);
if (!dpcm)
return -ENOMEM;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 1/6] ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure Pierre-Louis Bossart
@ 2021-12-07 17:37 ` Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking Pierre-Louis Bossart
` (4 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Pierre-Louis Bossart,
vkoul, broonie, Ranjani Sridharan, Gyeongtaek Lee,
Peter Ujfalusi
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>
[ removed FE stream lock by tiwai ]
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
sound/soc/soc-pcm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index f66808dfb508..3a34b71fd3c1 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1104,6 +1104,8 @@ 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;
unsigned long flags;
@@ -1113,6 +1115,20 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
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__);
+ 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;
+ }
+
dpcm = kzalloc(sizeof(struct snd_soc_dpcm), GFP_ATOMIC);
if (!dpcm)
return -ENOMEM;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 1/6] ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE Pierre-Louis Bossart
@ 2021-12-07 17:37 ` Pierre-Louis Bossart
[not found] ` <CGME20211217141541eucas1p1bdcb8b91e8a772229391b525d6adbf3b@eucas1p1.samsung.com>
2021-12-07 17:37 ` [PATCH 4/6] ASoC: soc-pcm: serialize BE triggers Pierre-Louis Bossart
` (3 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Pierre-Louis Bossart,
vkoul, broonie, Ranjani Sridharan, Gyeongtaek Lee,
Peter Ujfalusi
From: Takashi Iwai <tiwai@suse.de>
The existing locking for DPCM has several issues
a) a confusing mix of card->mutex and card->pcm_mutex.
b) a dpcm_lock spinlock added inconsistently and on paths that could
be recursively taken. The use of irqsave/irqrestore was also overkill.
The suggested model is:
1) The pcm_mutex is the top-most protection of BE links in the FE. The
pcm_mutex is applied always on either the top PCM callbacks or the
external call from DAPM, not taken in the internal functions.
2) the FE stream lock is taken in higher levels before invoking
dpcm_be_dai_trigger()
3) when adding and deleting a BE, both the pcm_mutex and FE stream
lock are taken.
Signed-off-by: Takashi Iwai <tiwai@suse.de>
[clarification of commit message by plbossart]
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
include/sound/soc.h | 2 -
sound/soc/soc-core.c | 1 -
sound/soc/soc-pcm.c | 229 ++++++++++++++++++++++++++++---------------
3 files changed, 152 insertions(+), 80 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 dcf6be4c4aaa..1d62160f96b1 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2315,7 +2315,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 3a34b71fd3c1..2e282c42bac2 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -27,6 +27,31 @@
#include <sound/soc-link.h>
#include <sound/initval.h>
+static inline void snd_soc_dpcm_mutex_lock(struct snd_soc_pcm_runtime *rtd)
+{
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+}
+
+static inline void snd_soc_dpcm_mutex_unlock(struct snd_soc_pcm_runtime *rtd)
+{
+ mutex_unlock(&rtd->card->pcm_mutex);
+}
+
+#define snd_soc_dpcm_mutex_assert_held(rtd) \
+ lockdep_assert_held(&(rtd)->card->pcm_mutex)
+
+static inline void snd_soc_dpcm_stream_lock_irq(struct snd_soc_pcm_runtime *rtd,
+ int stream)
+{
+ snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(rtd, stream));
+}
+
+static inline void snd_soc_dpcm_stream_unlock_irq(struct snd_soc_pcm_runtime *rtd,
+ int stream)
+{
+ snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(rtd, stream));
+}
+
#define DPCM_MAX_BE_USERS 8
static inline const char *soc_cpu_dai_name(struct snd_soc_pcm_runtime *rtd)
@@ -73,7 +98,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 +125,6 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
goto out;
}
- spin_lock_irqsave(&fe->card->dpcm_lock, flags);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
params = &dpcm->hw_params;
@@ -122,7 +145,6 @@ 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);
out:
return offset;
}
@@ -145,11 +167,13 @@ static ssize_t dpcm_state_read_file(struct file *file, char __user *user_buf,
if (!buf)
return -ENOMEM;
+ snd_soc_dpcm_mutex_lock(fe);
for_each_pcm_streams(stream)
if (snd_soc_dai_stream_valid(asoc_rtd_to_cpu(fe, 0), stream))
offset += dpcm_show_state(fe, stream,
buf + offset,
out_count - offset);
+ snd_soc_dpcm_mutex_unlock(fe);
ret = simple_read_from_buffer(user_buf, count, ppos, buf, offset);
@@ -221,14 +245,14 @@ static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe,
struct snd_pcm_substream *substream =
snd_soc_dpcm_get_substream(fe, stream);
- snd_pcm_stream_lock_irq(substream);
+ snd_soc_dpcm_stream_lock_irq(fe, stream);
if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) {
dpcm_fe_dai_do_trigger(substream,
fe->dpcm[stream].trigger_pending - 1);
fe->dpcm[stream].trigger_pending = 0;
}
fe->dpcm[stream].runtime_update = state;
- snd_pcm_stream_unlock_irq(substream);
+ snd_soc_dpcm_stream_unlock_irq(fe, stream);
}
static void dpcm_set_be_update_state(struct snd_soc_pcm_runtime *be,
@@ -256,7 +280,7 @@ void snd_soc_runtime_action(struct snd_soc_pcm_runtime *rtd,
struct snd_soc_dai *dai;
int i;
- lockdep_assert_held(&rtd->card->pcm_mutex);
+ snd_soc_dpcm_mutex_assert_held(rtd);
for_each_rtd_dais(rtd, i, dai)
snd_soc_dai_action(dai, stream, action);
@@ -309,6 +333,8 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
{
struct snd_soc_dpcm *dpcm;
+ snd_soc_dpcm_mutex_assert_held(fe);
+
for_each_dpcm_be(fe, dir, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -646,14 +672,14 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream,
return ret;
}
-static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
+static int soc_pcm_clean(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream, int rollback)
{
- struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
struct snd_soc_component *component;
struct snd_soc_dai *dai;
int i;
- mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ snd_soc_dpcm_mutex_assert_held(rtd);
if (!rollback)
snd_soc_runtime_deactivate(rtd, substream->stream);
@@ -665,9 +691,6 @@ static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
soc_pcm_components_close(substream, rollback);
-
- mutex_unlock(&rtd->card->pcm_mutex);
-
snd_soc_pcm_component_pm_runtime_put(rtd, substream, rollback);
for_each_rtd_components(rtd, i, component)
@@ -682,9 +705,21 @@ static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
* freed here. The cpu DAI, codec DAI, machine and components are also
* shutdown.
*/
+static int __soc_pcm_close(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream)
+{
+ return soc_pcm_clean(rtd, substream, 0);
+}
+
+/* PCM close ops for non-DPCM streams */
static int soc_pcm_close(struct snd_pcm_substream *substream)
{
- return soc_pcm_clean(substream, 0);
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+
+ snd_soc_dpcm_mutex_lock(rtd);
+ soc_pcm_clean(rtd, substream, 0);
+ snd_soc_dpcm_mutex_unlock(rtd);
+ return 0;
}
static int soc_hw_sanity_check(struct snd_pcm_substream *substream)
@@ -730,21 +765,21 @@ static int soc_hw_sanity_check(struct snd_pcm_substream *substream)
* then initialized and any private data can be allocated. This also calls
* startup for the cpu DAI, component, machine and codec DAI.
*/
-static int soc_pcm_open(struct snd_pcm_substream *substream)
+static int __soc_pcm_open(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream)
{
- struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
struct snd_soc_component *component;
struct snd_soc_dai *dai;
int i, ret = 0;
+ snd_soc_dpcm_mutex_assert_held(rtd);
+
for_each_rtd_components(rtd, i, component)
pinctrl_pm_select_default_state(component->dev);
ret = snd_soc_pcm_component_pm_runtime_get(rtd, substream);
if (ret < 0)
- goto pm_err;
-
- mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ goto err;
ret = soc_pcm_components_open(substream);
if (ret < 0)
@@ -791,16 +826,26 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
snd_soc_runtime_activate(rtd, substream->stream);
ret = 0;
err:
- mutex_unlock(&rtd->card->pcm_mutex);
-pm_err:
if (ret < 0) {
- soc_pcm_clean(substream, 1);
+ soc_pcm_clean(rtd, substream, 1);
dev_err(rtd->dev, "%s() failed (%d)", __func__, ret);
}
return ret;
}
+/* PCM open ops for non-DPCM streams */
+static int soc_pcm_open(struct snd_pcm_substream *substream)
+{
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ int ret;
+
+ snd_soc_dpcm_mutex_lock(rtd);
+ ret = __soc_pcm_open(rtd, substream);
+ snd_soc_dpcm_mutex_unlock(rtd);
+ return ret;
+}
+
static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd)
{
/*
@@ -816,13 +861,13 @@ static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd)
* rate, etc. This function is non atomic and can be called multiple times,
* it can refer to the runtime info.
*/
-static int soc_pcm_prepare(struct snd_pcm_substream *substream)
+static int __soc_pcm_prepare(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream)
{
- struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
struct snd_soc_dai *dai;
int i, ret = 0;
- mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ snd_soc_dpcm_mutex_assert_held(rtd);
ret = snd_soc_link_prepare(substream);
if (ret < 0)
@@ -850,14 +895,24 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
snd_soc_dai_digital_mute(dai, 0, substream->stream);
out:
- mutex_unlock(&rtd->card->pcm_mutex);
-
if (ret < 0)
dev_err(rtd->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
return ret;
}
+/* PCM prepare ops for non-DPCM streams */
+static int soc_pcm_prepare(struct snd_pcm_substream *substream)
+{
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ int ret;
+
+ snd_soc_dpcm_mutex_lock(rtd);
+ ret = __soc_pcm_prepare(rtd, substream);
+ snd_soc_dpcm_mutex_unlock(rtd);
+ return ret;
+}
+
static void soc_pcm_codec_params_fixup(struct snd_pcm_hw_params *params,
unsigned int mask)
{
@@ -869,13 +924,13 @@ static void soc_pcm_codec_params_fixup(struct snd_pcm_hw_params *params,
interval->max = channels;
}
-static int soc_pcm_hw_clean(struct snd_pcm_substream *substream, int rollback)
+static int soc_pcm_hw_clean(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream, int rollback)
{
- struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
struct snd_soc_dai *dai;
int i;
- mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ snd_soc_dpcm_mutex_assert_held(rtd);
/* clear the corresponding DAIs parameters when going to be inactive */
for_each_rtd_dais(rtd, i, dai) {
@@ -900,16 +955,28 @@ static int soc_pcm_hw_clean(struct snd_pcm_substream *substream, int rollback)
if (snd_soc_dai_stream_valid(dai, substream->stream))
snd_soc_dai_hw_free(dai, substream, rollback);
- mutex_unlock(&rtd->card->pcm_mutex);
return 0;
}
/*
* Frees resources allocated by hw_params, can be called multiple times
*/
+static int __soc_pcm_hw_free(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream)
+{
+ return soc_pcm_hw_clean(rtd, substream, 0);
+}
+
+/* hw_free PCM ops for non-DPCM streams */
static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
{
- return soc_pcm_hw_clean(substream, 0);
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ int ret;
+
+ snd_soc_dpcm_mutex_lock(rtd);
+ ret = __soc_pcm_hw_free(rtd, substream);
+ snd_soc_dpcm_mutex_unlock(rtd);
+ return ret;
}
/*
@@ -917,15 +984,15 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
* function can also be called multiple times and can allocate buffers
* (using snd_pcm_lib_* ). It's non-atomic.
*/
-static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params *params)
+static int __soc_pcm_hw_params(struct snd_soc_pcm_runtime *rtd,
+ struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
{
- struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
struct snd_soc_dai *cpu_dai;
struct snd_soc_dai *codec_dai;
int i, ret = 0;
- mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ snd_soc_dpcm_mutex_assert_held(rtd);
ret = soc_pcm_params_symmetry(substream, params);
if (ret)
@@ -997,16 +1064,27 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
ret = snd_soc_pcm_component_hw_params(substream, params);
out:
- mutex_unlock(&rtd->card->pcm_mutex);
-
if (ret < 0) {
- soc_pcm_hw_clean(substream, 1);
+ soc_pcm_hw_clean(rtd, substream, 1);
dev_err(rtd->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
}
return ret;
}
+/* hw_params PCM ops for non-DPCM streams */
+static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+ int ret;
+
+ snd_soc_dpcm_mutex_lock(rtd);
+ ret = __soc_pcm_hw_params(rtd, substream, params);
+ snd_soc_dpcm_mutex_unlock(rtd);
+ return ret;
+}
+
static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
@@ -1107,7 +1185,8 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
struct snd_pcm_substream *fe_substream;
struct snd_pcm_substream *be_substream;
struct snd_soc_dpcm *dpcm;
- unsigned long flags;
+
+ snd_soc_dpcm_mutex_assert_held(fe);
/* only add new dpcms */
for_each_dpcm_be(fe, stream, dpcm) {
@@ -1137,10 +1216,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_stream_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_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,
@@ -1183,8 +1262,10 @@ 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;
+ snd_soc_dpcm_mutex_assert_held(fe);
+
+ snd_soc_dpcm_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",
@@ -1202,12 +1283,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);
}
+ snd_soc_dpcm_stream_unlock_irq(fe, stream);
}
/* get BE for DAI widget and stream */
@@ -1431,12 +1511,9 @@ 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);
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);
}
void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
@@ -1472,12 +1549,12 @@ void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
continue;
if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE) {
- soc_pcm_hw_free(be_substream);
+ __soc_pcm_hw_free(be, be_substream);
be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
}
}
- soc_pcm_close(be_substream);
+ __soc_pcm_close(be, be_substream);
be_substream->runtime = NULL;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
}
@@ -1525,7 +1602,7 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
stream ? "capture" : "playback", be->dai_link->name);
be_substream->runtime = be->dpcm[stream].runtime;
- err = soc_pcm_open(be_substream);
+ err = __soc_pcm_open(be, be_substream);
if (err < 0) {
be->dpcm[stream].users--;
if (be->dpcm[stream].users < 0)
@@ -1769,7 +1846,7 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
dev_dbg(fe->dev, "ASoC: open FE %s\n", fe->dai_link->name);
/* start the DAI frontend */
- ret = soc_pcm_open(fe_substream);
+ ret = __soc_pcm_open(fe, fe_substream);
if (ret < 0)
goto unwind;
@@ -1800,6 +1877,8 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
int stream = substream->stream;
+ snd_soc_dpcm_mutex_assert_held(fe);
+
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
/* shutdown the BEs */
@@ -1808,7 +1887,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
dev_dbg(fe->dev, "ASoC: close FE %s\n", fe->dai_link->name);
/* now shutdown the frontend */
- soc_pcm_close(substream);
+ __soc_pcm_close(fe, substream);
/* run the stream stop event */
dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
@@ -1853,7 +1932,7 @@ void dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream)
dev_dbg(be->dev, "ASoC: hw_free BE %s\n",
be->dai_link->name);
- soc_pcm_hw_free(be_substream);
+ __soc_pcm_hw_free(be, be_substream);
be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
}
@@ -1864,13 +1943,13 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
int stream = substream->stream;
- mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+ snd_soc_dpcm_mutex_lock(fe);
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
dev_dbg(fe->dev, "ASoC: hw_free FE %s\n", fe->dai_link->name);
/* call hw_free on the frontend */
- soc_pcm_hw_free(substream);
+ soc_pcm_hw_clean(fe, substream, 0);
/* only hw_params backends that are either sinks or sources
* to this frontend DAI */
@@ -1879,7 +1958,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
- mutex_unlock(&fe->card->mutex);
+ snd_soc_dpcm_mutex_unlock(fe);
return 0;
}
@@ -1923,7 +2002,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
dev_dbg(be->dev, "ASoC: hw_params BE %s\n",
be->dai_link->name);
- ret = soc_pcm_hw_params(be_substream, &dpcm->hw_params);
+ ret = __soc_pcm_hw_params(be, be_substream, &dpcm->hw_params);
if (ret < 0)
goto unwind;
@@ -1953,7 +2032,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
(be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP))
continue;
- soc_pcm_hw_free(be_substream);
+ __soc_pcm_hw_free(be, be_substream);
}
return ret;
@@ -1965,7 +2044,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
int ret, stream = substream->stream;
- mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+ snd_soc_dpcm_mutex_lock(fe);
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
memcpy(&fe->dpcm[stream].hw_params, params,
@@ -1979,7 +2058,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
params_channels(params), params_format(params));
/* call hw_params on the frontend */
- ret = soc_pcm_hw_params(substream, params);
+ ret = __soc_pcm_hw_params(fe, substream, params);
if (ret < 0)
dpcm_be_dai_hw_free(fe, stream);
else
@@ -1987,7 +2066,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
out:
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
- mutex_unlock(&fe->card->mutex);
+ snd_soc_dpcm_mutex_unlock(fe);
if (ret < 0)
dev_err(fe->dev, "ASoC: %s failed (%d)\n", __func__, ret);
@@ -2258,7 +2337,7 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream)
dev_dbg(be->dev, "ASoC: prepare BE %s\n",
be->dai_link->name);
- ret = soc_pcm_prepare(be_substream);
+ ret = __soc_pcm_prepare(be, be_substream);
if (ret < 0)
break;
@@ -2276,7 +2355,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
int stream = substream->stream, ret = 0;
- mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+ snd_soc_dpcm_mutex_lock(fe);
dev_dbg(fe->dev, "ASoC: prepare FE %s\n", fe->dai_link->name);
@@ -2295,7 +2374,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
goto out;
/* call prepare on the frontend */
- ret = soc_pcm_prepare(substream);
+ ret = __soc_pcm_prepare(fe, substream);
if (ret < 0)
goto out;
@@ -2303,7 +2382,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
out:
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
- mutex_unlock(&fe->card->mutex);
+ snd_soc_dpcm_mutex_unlock(fe);
if (ret < 0)
dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
@@ -2354,7 +2433,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);
@@ -2423,7 +2501,6 @@ 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);
for_each_dpcm_be(fe, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -2435,7 +2512,6 @@ 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);
if (ret < 0)
dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
@@ -2510,7 +2586,7 @@ int snd_soc_dpcm_runtime_update(struct snd_soc_card *card)
struct snd_soc_pcm_runtime *fe;
int ret = 0;
- mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+ mutex_lock_nested(&card->pcm_mutex, card->pcm_subclass);
/* shutdown all old paths first */
for_each_card_rtds(card, fe) {
ret = soc_dpcm_fe_runtime_update(fe, 0);
@@ -2526,7 +2602,7 @@ int snd_soc_dpcm_runtime_update(struct snd_soc_card *card)
}
out:
- mutex_unlock(&card->mutex);
+ mutex_unlock(&card->pcm_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(snd_soc_dpcm_runtime_update);
@@ -2537,6 +2613,8 @@ static void dpcm_fe_dai_cleanup(struct snd_pcm_substream *fe_substream)
struct snd_soc_dpcm *dpcm;
int stream = fe_substream->stream;
+ snd_soc_dpcm_mutex_assert_held(fe);
+
/* mark FE's links ready to prune */
for_each_dpcm_be(fe, stream, dpcm)
dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
@@ -2551,12 +2629,12 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(fe_substream);
int ret;
- mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+ snd_soc_dpcm_mutex_lock(fe);
ret = dpcm_fe_dai_shutdown(fe_substream);
dpcm_fe_dai_cleanup(fe_substream);
- mutex_unlock(&fe->card->mutex);
+ snd_soc_dpcm_mutex_unlock(fe);
return ret;
}
@@ -2567,7 +2645,7 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
int ret;
int stream = fe_substream->stream;
- mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+ snd_soc_dpcm_mutex_lock(fe);
fe->dpcm[stream].runtime = fe_substream->runtime;
ret = dpcm_path_get(fe, stream, &list);
@@ -2584,7 +2662,7 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
dpcm_clear_pending_state(fe, stream);
dpcm_path_put(&list);
open_end:
- mutex_unlock(&fe->card->mutex);
+ snd_soc_dpcm_mutex_unlock(fe);
return ret;
}
@@ -2845,10 +2923,8 @@ 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);
for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe)
@@ -2862,7 +2938,6 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
}
}
}
- spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
/* it's safe to do this BE DAI */
return ret;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/6] ASoC: soc-pcm: serialize BE triggers
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
` (2 preceding siblings ...)
2021-12-07 17:37 ` [PATCH 3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking Pierre-Louis Bossart
@ 2021-12-07 17:37 ` Pierre-Louis Bossart
[not found] ` <CGME20211217101152eucas1p14ea09622edde5118ddd832c3ebaebdde@eucas1p1.samsung.com>
2021-12-07 17:37 ` [PATCH 5/6] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart
` (2 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Pierre-Louis Bossart,
vkoul, broonie, Ranjani Sridharan, Gyeongtaek Lee,
Peter Ujfalusi
From: Takashi Iwai <tiwai@suse.de>
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: Takashi Iwai <tiwai@suse.de>
[test, checkpatch fix and clarification of commit message by plbossart]
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
sound/soc/soc-pcm.c | 46 ++++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 2e282c42bac2..7043857e30b1 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -46,12 +46,18 @@ static inline void snd_soc_dpcm_stream_lock_irq(struct snd_soc_pcm_runtime *rtd,
snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(rtd, stream));
}
+#define snd_soc_dpcm_stream_lock_irqsave(rtd, stream, flags) \
+ snd_pcm_stream_lock_irqsave(snd_soc_dpcm_get_substream(rtd, stream), flags)
+
static inline void snd_soc_dpcm_stream_unlock_irq(struct snd_soc_pcm_runtime *rtd,
int stream)
{
snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(rtd, stream));
}
+#define snd_soc_dpcm_stream_unlock_irqrestore(rtd, stream, flags) \
+ snd_pcm_stream_unlock_irqrestore(snd_soc_dpcm_get_substream(rtd, stream), flags)
+
#define DPCM_MAX_BE_USERS 8
static inline const char *soc_cpu_dai_name(struct snd_soc_pcm_runtime *rtd)
@@ -2079,6 +2085,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) {
@@ -2087,9 +2094,11 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
be = dpcm->be;
be_substream = snd_soc_dpcm_get_substream(be, stream);
+ snd_soc_dpcm_stream_lock_irqsave(be, stream, flags);
+
/* is this op for this BE ? */
if (!snd_soc_dpcm_be_can_update(fe, be, stream))
- continue;
+ goto next;
dev_dbg(be->dev, "ASoC: trigger BE %s cmd %d\n",
be->dai_link->name, cmd);
@@ -2099,77 +2108,80 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
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 next;
ret = soc_pcm_trigger(be_substream, cmd);
if (ret)
- goto end;
+ goto next;
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 next;
ret = soc_pcm_trigger(be_substream, cmd);
if (ret)
- goto end;
+ goto next;
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 next;
ret = soc_pcm_trigger(be_substream, cmd);
if (ret)
- goto end;
+ goto next;
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 next;
if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
- continue;
+ goto next;
ret = soc_pcm_trigger(be_substream, cmd);
if (ret)
- goto end;
+ goto next;
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 next;
if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
- continue;
+ goto next;
ret = soc_pcm_trigger(be_substream, cmd);
if (ret)
- goto end;
+ goto next;
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 next;
if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
- continue;
+ goto next;
ret = soc_pcm_trigger(be_substream, cmd);
if (ret)
- goto end;
+ goto next;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
break;
}
+next:
+ snd_soc_dpcm_stream_unlock_irqrestore(be, stream, flags);
+ if (ret)
+ break;
}
-end:
if (ret < 0)
dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n",
__func__, be->dai_link->name, ret);
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/6] ASoC: soc-pcm: test refcount before triggering
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
` (3 preceding siblings ...)
2021-12-07 17:37 ` [PATCH 4/6] ASoC: soc-pcm: serialize BE triggers Pierre-Louis Bossart
@ 2021-12-07 17:37 ` Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 6/6] ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE Pierre-Louis Bossart
2021-12-15 2:04 ` [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Mark Brown
6 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Pierre-Louis Bossart,
vkoul, broonie, Ranjani Sridharan, Gyeongtaek Lee,
Peter Ujfalusi
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>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@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 bc7af90099a8..75b92d883976 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 7043857e30b1..05a0f52eb11b 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1619,7 +1619,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++;
}
@@ -2105,14 +2105,21 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
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 next;
+ be->dpcm[stream].be_start++;
+ if (be->dpcm[stream].be_start != 1)
+ goto next;
+
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start--;
goto next;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2120,9 +2127,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 next;
+ be->dpcm[stream].be_start++;
+ if (be->dpcm[stream].be_start != 1)
+ goto next;
+
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start--;
goto next;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2130,9 +2143,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 next;
+ be->dpcm[stream].be_start++;
+ if (be->dpcm[stream].be_start != 1)
+ goto next;
+
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start--;
goto next;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2141,12 +2160,18 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
(be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
goto next;
- if (!snd_soc_dpcm_can_be_free_stop(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 next;
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 next;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
break;
@@ -2154,12 +2179,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 next;
- if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+ be->dpcm[stream].be_start--;
+ if (be->dpcm[stream].be_start != 0)
goto next;
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start++;
goto next;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
break;
@@ -2167,12 +2195,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 next;
- if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+ be->dpcm[stream].be_start--;
+ if (be->dpcm[stream].be_start != 0)
goto next;
ret = soc_pcm_trigger(be_substream, cmd);
- if (ret)
+ if (ret) {
+ be->dpcm[stream].be_start++;
goto next;
+ }
be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 6/6] ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
` (4 preceding siblings ...)
2021-12-07 17:37 ` [PATCH 5/6] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart
@ 2021-12-07 17:37 ` Pierre-Louis Bossart
2021-12-15 2:04 ` [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Mark Brown
6 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-07 17:37 UTC (permalink / raw)
To: alsa-devel
Cc: Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Pierre-Louis Bossart,
vkoul, broonie, Ranjani Sridharan, Gyeongtaek Lee,
Peter Ujfalusi, Bard Liao
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
play FE1 -> BE state is START
play FE2 -> BE state is START (no change)
pause FE1 -> BE state is START (no change)
pause FE2 -> BE state is PAUSED
release FE1 -> BE state is START
release FE2 -> BE state is START (no change)
stop FE1 -> BE state is START (no change)
stop FE2 -> 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, and increase the refcount if the BE is already in START.
[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>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
sound/soc/soc-pcm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 05a0f52eb11b..7abfc48b26ca 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2140,7 +2140,10 @@ 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].be_start &&
+ (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 next;
be->dpcm[stream].be_start++;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
` (5 preceding siblings ...)
2021-12-07 17:37 ` [PATCH 6/6] ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE Pierre-Louis Bossart
@ 2021-12-15 2:04 ` Mark Brown
6 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2021-12-15 2:04 UTC (permalink / raw)
To: alsa-devel, Pierre-Louis Bossart
Cc: Kuninori Morimoto, tiwai, Sameer Pujar, linux-kernel, vkoul,
Gyeongtaek Lee, Peter Ujfalusi
On Tue, 7 Dec 2021 11:37:39 -0600, Pierre-Louis Bossart wrote:
> 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.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure
commit: d8a9c6e1f6766a16cf02b4e99a629f3c5512c183
[2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE
commit: bbf7d3b1c4f40eb02dd1dffb500ba00b0bff0303
[3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking
commit: b7898396f4bbe160f546d0c5e9fa17cca9a7d153
[4/6] ASoC: soc-pcm: serialize BE triggers
commit: b2ae80663008a7662febe7d13f14ea1b2eb0cd51
[5/6] ASoC: soc-pcm: test refcount before triggering
commit: 848aedfdc6ba25ad5652797db9266007773e44dd
[6/6] ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE
commit: 3aa1e96a2b95e2ece198f8dd01e96818971b84df
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/6] ASoC: soc-pcm: serialize BE triggers
[not found] ` <CGME20211217101152eucas1p14ea09622edde5118ddd832c3ebaebdde@eucas1p1.samsung.com>
@ 2021-12-17 10:11 ` Marek Szyprowski
[not found] ` <CGME20211217115743eucas1p10e721164cd845176f9dc5c4c2ad86838@eucas1p1.samsung.com>
0 siblings, 1 reply; 13+ messages in thread
From: Marek Szyprowski @ 2021-12-17 10:11 UTC (permalink / raw)
To: Pierre-Louis Bossart, alsa-devel
Cc: 'Linux Samsung SOC',
Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Krzysztof Kozlowski,
Ranjani Sridharan, vkoul, broonie, Gyeongtaek Lee,
Peter Ujfalusi
Dear All,
On 07.12.2021 18:37, Pierre-Louis Bossart wrote:
> From: Takashi Iwai <tiwai@suse.de>
>
> 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: Takashi Iwai <tiwai@suse.de>
> [test, checkpatch fix and clarification of commit message by plbossart]
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
This patch recently landed in linux-next (next-20211215) as commit
b2ae80663008 ("ASoC: soc-pcm: serialize BE triggers"). I found that it
triggers a warning on my test boards. This is the one from
Exynos4412-based Odroid U3 board:
# speaker-test -l1
speaker-test 1.1.8
Playback device is default
Stream parameters are 48000Hz, S16_LE, 1 channels
Using 16 octaves of pink noise
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
0 - Front Left
============================================
WARNING: possible recursive locking detected
5.16.0-rc1-00270-gb2ae80663008 #11109 Not tainted
--------------------------------------------
speaker-test/1312 is trying to acquire lock:
c1d78ca4 (&group->lock){....}-{2:2}, at: dpcm_be_dai_trigger+0x80/0x300
but task is already holding lock:
c1d788a4 (&group->lock){....}-{2:2}, at: snd_pcm_action_lock_irq+0x68/0x7c
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&group->lock);
lock(&group->lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by speaker-test/1312:
#0: c1d788a4 (&group->lock){....}-{2:2}, at:
snd_pcm_action_lock_irq+0x68/0x7c
stack backtrace:
CPU: 0 PID: 1312 Comm: speaker-test Not tainted
5.16.0-rc1-00270-gb2ae80663008 #11109
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
[<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
[<c0b65d24>] (dump_stack_lvl) from [<c0193740>]
(__lock_acquire+0x15ac/0x319c)
[<c0193740>] (__lock_acquire) from [<c0195dd8>] (lock_acquire+0x14c/0x424)
[<c0195dd8>] (lock_acquire) from [<c0b745b8>]
(_raw_spin_lock_irqsave+0x44/0x60)
[<c0b745b8>] (_raw_spin_lock_irqsave) from [<c0926b6c>]
(dpcm_be_dai_trigger+0x80/0x300)
[<c0926b6c>] (dpcm_be_dai_trigger) from [<c0927004>]
(dpcm_fe_dai_do_trigger+0x124/0x1e4)
[<c0927004>] (dpcm_fe_dai_do_trigger) from [<c090728c>]
(snd_pcm_action+0x74/0xb0)
[<c090728c>] (snd_pcm_action) from [<c0907eac>]
(snd_pcm_action_lock_irq+0x3c/0x7c)
[<c0907eac>] (snd_pcm_action_lock_irq) from [<c02f13a0>]
(sys_ioctl+0x568/0xd44)
[<c02f13a0>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x1c)
Exception stack(0xc4777fa8 to 0xc4777ff0)
7fa0: 004f5210 b6e27394 00000004 00004142 004f5398
004f5398
7fc0: 004f5210 b6e27394 00020000 00000036 00000000 00000000 bee588e8
00008000
7fe0: b6e277c4 bee58874 b6d8e888 b6c751dc
Time per period = 0.253397
max98090 1-0010: PLL unlocked
BUG: sleeping function called from invalid context at
kernel/locking/rwsem.c:1526
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1312, name:
speaker-test
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
INFO: lockdep is turned off.
irq event stamp: 8158
hardirqs last enabled at (8157): [<c0b747d0>]
_raw_spin_unlock_irqrestore+0x5c/0x60
hardirqs last disabled at (8158): [<c0b74570>] _raw_spin_lock_irq+0x58/0x5c
softirqs last enabled at (7854): [<c0101578>] __do_softirq+0x348/0x610
softirqs last disabled at (7849): [<c012e7a4>] __irq_exit_rcu+0x144/0x1ec
Preemption disabled at:
[<00000000>] 0x0
CPU: 0 PID: 1312 Comm: speaker-test Not tainted
5.16.0-rc1-00270-gb2ae80663008 #11109
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
[<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
[<c0b65d24>] (dump_stack_lvl) from [<c0158b04>]
(__might_resched+0x1c0/0x288)
[<c0158b04>] (__might_resched) from [<c0b71898>] (down_write+0x24/0x8c)
[<c0b71898>] (down_write) from [<c030ed64>]
(simple_recursive_removal+0x6c/0x370)
[<c030ed64>] (simple_recursive_removal) from [<c04d07a4>]
(debugfs_remove+0x38/0x4c)
[<c04d07a4>] (debugfs_remove) from [<c0928784>]
(dpcm_be_disconnect+0x160/0x2c4)
[<c0928784>] (dpcm_be_disconnect) from [<c092895c>]
(dpcm_fe_dai_cleanup+0x74/0xb0)
[<c092895c>] (dpcm_fe_dai_cleanup) from [<c0928d90>]
(dpcm_fe_dai_close+0xe8/0x14c)
[<c0928d90>] (dpcm_fe_dai_close) from [<c090977c>]
(snd_pcm_release_substream.part.0+0x3c/0xcc)
[<c090977c>] (snd_pcm_release_substream.part.0) from [<c0909878>]
(snd_pcm_release+0x54/0xa4)
[<c0909878>] (snd_pcm_release) from [<c02dc400>] (__fput+0x88/0x258)
[<c02dc400>] (__fput) from [<c014cd44>] (task_work_run+0x8c/0xc8)
[<c014cd44>] (task_work_run) from [<c010c08c>] (do_work_pending+0x4a4/0x598)
[<c010c08c>] (do_work_pending) from [<c0100088>]
(slow_work_pending+0xc/0x20)
Exception stack(0xc4777fb0 to 0xc4777ff8)
7fa0: 00000000 004f5260 004eaa9c
00000000
7fc0: 004f5260 004f536c 004f5210 00000006 004fb700 004e6e8c 004d6120
bee58cc4
7fe0: b6e27e64 bee58928 b6d8eda4 b6d09ac0 60000050 00000004
Let me know how I can help debugging this issue.
> ---
> sound/soc/soc-pcm.c | 46 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 2e282c42bac2..7043857e30b1 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -46,12 +46,18 @@ static inline void snd_soc_dpcm_stream_lock_irq(struct snd_soc_pcm_runtime *rtd,
> snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(rtd, stream));
> }
>
> +#define snd_soc_dpcm_stream_lock_irqsave(rtd, stream, flags) \
> + snd_pcm_stream_lock_irqsave(snd_soc_dpcm_get_substream(rtd, stream), flags)
> +
> static inline void snd_soc_dpcm_stream_unlock_irq(struct snd_soc_pcm_runtime *rtd,
> int stream)
> {
> snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(rtd, stream));
> }
>
> +#define snd_soc_dpcm_stream_unlock_irqrestore(rtd, stream, flags) \
> + snd_pcm_stream_unlock_irqrestore(snd_soc_dpcm_get_substream(rtd, stream), flags)
> +
> #define DPCM_MAX_BE_USERS 8
>
> static inline const char *soc_cpu_dai_name(struct snd_soc_pcm_runtime *rtd)
> @@ -2079,6 +2085,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) {
> @@ -2087,9 +2094,11 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
> be = dpcm->be;
> be_substream = snd_soc_dpcm_get_substream(be, stream);
>
> + snd_soc_dpcm_stream_lock_irqsave(be, stream, flags);
> +
> /* is this op for this BE ? */
> if (!snd_soc_dpcm_be_can_update(fe, be, stream))
> - continue;
> + goto next;
>
> dev_dbg(be->dev, "ASoC: trigger BE %s cmd %d\n",
> be->dai_link->name, cmd);
> @@ -2099,77 +2108,80 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
> 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 next;
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - goto end;
> + goto next;
>
> 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 next;
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - goto end;
> + goto next;
>
> 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 next;
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - goto end;
> + goto next;
>
> 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 next;
>
> if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
> - continue;
> + goto next;
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - goto end;
> + goto next;
>
> 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 next;
>
> if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
> - continue;
> + goto next;
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - goto end;
> + goto next;
>
> 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 next;
>
> if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
> - continue;
> + goto next;
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - goto end;
> + goto next;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
> break;
> }
> +next:
> + snd_soc_dpcm_stream_unlock_irqrestore(be, stream, flags);
> + if (ret)
> + break;
> }
> -end:
> if (ret < 0)
> dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n",
> __func__, be->dai_link->name, ret);
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/6] ASoC: soc-pcm: serialize BE triggers
[not found] ` <CGME20211217115743eucas1p10e721164cd845176f9dc5c4c2ad86838@eucas1p1.samsung.com>
@ 2021-12-17 11:57 ` Marek Szyprowski
0 siblings, 0 replies; 13+ messages in thread
From: Marek Szyprowski @ 2021-12-17 11:57 UTC (permalink / raw)
To: Pierre-Louis Bossart, alsa-devel
Cc: 'Linux Samsung SOC',
Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Krzysztof Kozlowski,
Ranjani Sridharan, vkoul, broonie, Gyeongtaek Lee,
Peter Ujfalusi
Dear All,
On 17.12.2021 11:11, Marek Szyprowski wrote:
> On 07.12.2021 18:37, Pierre-Louis Bossart wrote:
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> 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: Takashi Iwai <tiwai@suse.de>
>> [test, checkpatch fix and clarification of commit message by plbossart]
>> Signed-off-by: Pierre-Louis Bossart
>> <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
>
> This patch recently landed in linux-next (next-20211215) as commit
> b2ae80663008 ("ASoC: soc-pcm: serialize BE triggers").
It turned out that the issue is caused by the previous patch "[PATCH
3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking
<https://lore.kernel.org/all/20211207173745.15850-4-pierre-louis.bossart@linux.intel.com/#r>",
merged as commit b7898396f4bb ("ASoC: soc-pcm: Fix and cleanup DPCM
locking"), for some reasons 'git bisect' pointed me initially to the
wrong patch. I will re-report it under the proper patch. Reverting it
and the next patches on top of linux-next fixes the issue.
> I found that it triggers a warning on my test boards. This is the one
> from Exynos4412-based Odroid U3 board:
>
> # speaker-test -l1
>
> speaker-test 1.1.8
>
> Playback device is default
> Stream parameters are 48000Hz, S16_LE, 1 channels
> Using 16 octaves of pink noise
> Rate set to 48000Hz (requested 48000Hz)
> Buffer size range from 128 to 131072
> Period size range from 64 to 65536
> Using max buffer size 131072
> Periods = 4
> was set period_size = 32768
> was set buffer_size = 131072
> 0 - Front Left
>
> ============================================
> WARNING: possible recursive locking detected
> 5.16.0-rc1-00270-gb2ae80663008 #11109 Not tainted
> --------------------------------------------
> speaker-test/1312 is trying to acquire lock:
> c1d78ca4 (&group->lock){....}-{2:2}, at: dpcm_be_dai_trigger+0x80/0x300
>
> but task is already holding lock:
> c1d788a4 (&group->lock){....}-{2:2}, at:
> snd_pcm_action_lock_irq+0x68/0x7c
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&group->lock);
> lock(&group->lock);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 1 lock held by speaker-test/1312:
> #0: c1d788a4 (&group->lock){....}-{2:2}, at:
> snd_pcm_action_lock_irq+0x68/0x7c
>
> stack backtrace:
> CPU: 0 PID: 1312 Comm: speaker-test Not tainted
> 5.16.0-rc1-00270-gb2ae80663008 #11109
> Hardware name: Samsung Exynos (Flattened Device Tree)
> [<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
> [<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
> [<c0b65d24>] (dump_stack_lvl) from [<c0193740>]
> (__lock_acquire+0x15ac/0x319c)
> [<c0193740>] (__lock_acquire) from [<c0195dd8>]
> (lock_acquire+0x14c/0x424)
> [<c0195dd8>] (lock_acquire) from [<c0b745b8>]
> (_raw_spin_lock_irqsave+0x44/0x60)
> [<c0b745b8>] (_raw_spin_lock_irqsave) from [<c0926b6c>]
> (dpcm_be_dai_trigger+0x80/0x300)
> [<c0926b6c>] (dpcm_be_dai_trigger) from [<c0927004>]
> (dpcm_fe_dai_do_trigger+0x124/0x1e4)
> [<c0927004>] (dpcm_fe_dai_do_trigger) from [<c090728c>]
> (snd_pcm_action+0x74/0xb0)
> [<c090728c>] (snd_pcm_action) from [<c0907eac>]
> (snd_pcm_action_lock_irq+0x3c/0x7c)
> [<c0907eac>] (snd_pcm_action_lock_irq) from [<c02f13a0>]
> (sys_ioctl+0x568/0xd44)
> [<c02f13a0>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x1c)
> Exception stack(0xc4777fa8 to 0xc4777ff0)
> 7fa0: 004f5210 b6e27394 00000004 00004142 004f5398
> 004f5398
> 7fc0: 004f5210 b6e27394 00020000 00000036 00000000 00000000 bee588e8
> 00008000
> 7fe0: b6e277c4 bee58874 b6d8e888 b6c751dc
> Time per period = 0.253397
> max98090 1-0010: PLL unlocked
> BUG: sleeping function called from invalid context at
> kernel/locking/rwsem.c:1526
> in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1312, name:
> speaker-test
> preempt_count: 1, expected: 0
> RCU nest depth: 0, expected: 0
> INFO: lockdep is turned off.
> irq event stamp: 8158
> hardirqs last enabled at (8157): [<c0b747d0>]
> _raw_spin_unlock_irqrestore+0x5c/0x60
> hardirqs last disabled at (8158): [<c0b74570>]
> _raw_spin_lock_irq+0x58/0x5c
> softirqs last enabled at (7854): [<c0101578>] __do_softirq+0x348/0x610
> softirqs last disabled at (7849): [<c012e7a4>] __irq_exit_rcu+0x144/0x1ec
> Preemption disabled at:
> [<00000000>] 0x0
> CPU: 0 PID: 1312 Comm: speaker-test Not tainted
> 5.16.0-rc1-00270-gb2ae80663008 #11109
> Hardware name: Samsung Exynos (Flattened Device Tree)
> [<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
> [<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
> [<c0b65d24>] (dump_stack_lvl) from [<c0158b04>]
> (__might_resched+0x1c0/0x288)
> [<c0158b04>] (__might_resched) from [<c0b71898>] (down_write+0x24/0x8c)
> [<c0b71898>] (down_write) from [<c030ed64>]
> (simple_recursive_removal+0x6c/0x370)
> [<c030ed64>] (simple_recursive_removal) from [<c04d07a4>]
> (debugfs_remove+0x38/0x4c)
> [<c04d07a4>] (debugfs_remove) from [<c0928784>]
> (dpcm_be_disconnect+0x160/0x2c4)
> [<c0928784>] (dpcm_be_disconnect) from [<c092895c>]
> (dpcm_fe_dai_cleanup+0x74/0xb0)
> [<c092895c>] (dpcm_fe_dai_cleanup) from [<c0928d90>]
> (dpcm_fe_dai_close+0xe8/0x14c)
> [<c0928d90>] (dpcm_fe_dai_close) from [<c090977c>]
> (snd_pcm_release_substream.part.0+0x3c/0xcc)
> [<c090977c>] (snd_pcm_release_substream.part.0) from [<c0909878>]
> (snd_pcm_release+0x54/0xa4)
> [<c0909878>] (snd_pcm_release) from [<c02dc400>] (__fput+0x88/0x258)
> [<c02dc400>] (__fput) from [<c014cd44>] (task_work_run+0x8c/0xc8)
> [<c014cd44>] (task_work_run) from [<c010c08c>]
> (do_work_pending+0x4a4/0x598)
> [<c010c08c>] (do_work_pending) from [<c0100088>]
> (slow_work_pending+0xc/0x20)
> Exception stack(0xc4777fb0 to 0xc4777ff8)
> 7fa0: 00000000 004f5260 004eaa9c
> 00000000
> 7fc0: 004f5260 004f536c 004f5210 00000006 004fb700 004e6e8c 004d6120
> bee58cc4
> 7fe0: b6e27e64 bee58928 b6d8eda4 b6d09ac0 60000050 00000004
>
> Let me know how I can help debugging this issue.
>
>> ---
>> sound/soc/soc-pcm.c | 46 ++++++++++++++++++++++++++++-----------------
>> 1 file changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index 2e282c42bac2..7043857e30b1 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -46,12 +46,18 @@ static inline void
>> snd_soc_dpcm_stream_lock_irq(struct snd_soc_pcm_runtime *rtd,
>> snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(rtd, stream));
>> }
>> +#define snd_soc_dpcm_stream_lock_irqsave(rtd, stream, flags) \
>> + snd_pcm_stream_lock_irqsave(snd_soc_dpcm_get_substream(rtd,
>> stream), flags)
>> +
>> static inline void snd_soc_dpcm_stream_unlock_irq(struct
>> snd_soc_pcm_runtime *rtd,
>> int stream)
>> {
>> snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(rtd,
>> stream));
>> }
>> +#define snd_soc_dpcm_stream_unlock_irqrestore(rtd, stream, flags) \
>> + snd_pcm_stream_unlock_irqrestore(snd_soc_dpcm_get_substream(rtd,
>> stream), flags)
>> +
>> #define DPCM_MAX_BE_USERS 8
>> static inline const char *soc_cpu_dai_name(struct
>> snd_soc_pcm_runtime *rtd)
>> @@ -2079,6 +2085,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) {
>> @@ -2087,9 +2094,11 @@ int dpcm_be_dai_trigger(struct
>> snd_soc_pcm_runtime *fe, int stream,
>> be = dpcm->be;
>> be_substream = snd_soc_dpcm_get_substream(be, stream);
>> + snd_soc_dpcm_stream_lock_irqsave(be, stream, flags);
>> +
>> /* is this op for this BE ? */
>> if (!snd_soc_dpcm_be_can_update(fe, be, stream))
>> - continue;
>> + goto next;
>> dev_dbg(be->dev, "ASoC: trigger BE %s cmd %d\n",
>> be->dai_link->name, cmd);
>> @@ -2099,77 +2108,80 @@ int dpcm_be_dai_trigger(struct
>> snd_soc_pcm_runtime *fe, int stream,
>> 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 next;
>> ret = soc_pcm_trigger(be_substream, cmd);
>> if (ret)
>> - goto end;
>> + goto next;
>> 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 next;
>> ret = soc_pcm_trigger(be_substream, cmd);
>> if (ret)
>> - goto end;
>> + goto next;
>> 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 next;
>> ret = soc_pcm_trigger(be_substream, cmd);
>> if (ret)
>> - goto end;
>> + goto next;
>> 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 next;
>> if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
>> - continue;
>> + goto next;
>> ret = soc_pcm_trigger(be_substream, cmd);
>> if (ret)
>> - goto end;
>> + goto next;
>> 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 next;
>> if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
>> - continue;
>> + goto next;
>> ret = soc_pcm_trigger(be_substream, cmd);
>> if (ret)
>> - goto end;
>> + goto next;
>> 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 next;
>> if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
>> - continue;
>> + goto next;
>> ret = soc_pcm_trigger(be_substream, cmd);
>> if (ret)
>> - goto end;
>> + goto next;
>> be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
>> break;
>> }
>> +next:
>> + snd_soc_dpcm_stream_unlock_irqrestore(be, stream, flags);
>> + if (ret)
>> + break;
>> }
>> -end:
>> if (ret < 0)
>> dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n",
>> __func__, be->dai_link->name, ret);
>
> Best regards
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking
[not found] ` <CGME20211217141541eucas1p1bdcb8b91e8a772229391b525d6adbf3b@eucas1p1.samsung.com>
@ 2021-12-17 14:15 ` Marek Szyprowski
2021-12-17 16:54 ` Pierre-Louis Bossart
0 siblings, 1 reply; 13+ messages in thread
From: Marek Szyprowski @ 2021-12-17 14:15 UTC (permalink / raw)
To: Pierre-Louis Bossart, alsa-devel
Cc: 'Linux Samsung SOC',
Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Krzysztof Kozlowski,
Ranjani Sridharan, vkoul, broonie, Gyeongtaek Lee,
Peter Ujfalusi
Dear All,
On 07.12.2021 18:37, Pierre-Louis Bossart wrote:
> From: Takashi Iwai <tiwai@suse.de>
>
> The existing locking for DPCM has several issues
> a) a confusing mix of card->mutex and card->pcm_mutex.
> b) a dpcm_lock spinlock added inconsistently and on paths that could
> be recursively taken. The use of irqsave/irqrestore was also overkill.
>
> The suggested model is:
>
> 1) The pcm_mutex is the top-most protection of BE links in the FE. The
> pcm_mutex is applied always on either the top PCM callbacks or the
> external call from DAPM, not taken in the internal functions.
>
> 2) the FE stream lock is taken in higher levels before invoking
> dpcm_be_dai_trigger()
>
> 3) when adding and deleting a BE, both the pcm_mutex and FE stream
> lock are taken.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> [clarification of commit message by plbossart]
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
This patch recently landed in linux-next (next-20211215) as commit
b7898396f4bb ("ASoC: soc-pcm: Fix and cleanup DPCM locking"). I found
that after applying it, a warning is triggered on my test boards. This
is the one from Exynos4412-based Odroid U3 board:
# speaker-test -l1
speaker-test 1.1.8
Playback device is default
Stream parameters are 48000Hz, S16_LE, 1 channels
Using 16 octaves of pink noise
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
0 - Front Left
============================================
WARNING: possible recursive locking detected
5.16.0-rc1-00270-gb2ae80663008 #11109 Not tainted
--------------------------------------------
speaker-test/1312 is trying to acquire lock:
c1d78ca4 (&group->lock){....}-{2:2}, at: dpcm_be_dai_trigger+0x80/0x300
but task is already holding lock:
c1d788a4 (&group->lock){....}-{2:2}, at: snd_pcm_action_lock_irq+0x68/0x7c
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&group->lock);
lock(&group->lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by speaker-test/1312:
#0: c1d788a4 (&group->lock){....}-{2:2}, at:
snd_pcm_action_lock_irq+0x68/0x7c
stack backtrace:
CPU: 0 PID: 1312 Comm: speaker-test Not tainted
5.16.0-rc1-00270-gb2ae80663008 #11109
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
[<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
[<c0b65d24>] (dump_stack_lvl) from [<c0193740>]
(__lock_acquire+0x15ac/0x319c)
[<c0193740>] (__lock_acquire) from [<c0195dd8>] (lock_acquire+0x14c/0x424)
[<c0195dd8>] (lock_acquire) from [<c0b745b8>]
(_raw_spin_lock_irqsave+0x44/0x60)
[<c0b745b8>] (_raw_spin_lock_irqsave) from [<c0926b6c>]
(dpcm_be_dai_trigger+0x80/0x300)
[<c0926b6c>] (dpcm_be_dai_trigger) from [<c0927004>]
(dpcm_fe_dai_do_trigger+0x124/0x1e4)
[<c0927004>] (dpcm_fe_dai_do_trigger) from [<c090728c>]
(snd_pcm_action+0x74/0xb0)
[<c090728c>] (snd_pcm_action) from [<c0907eac>]
(snd_pcm_action_lock_irq+0x3c/0x7c)
[<c0907eac>] (snd_pcm_action_lock_irq) from [<c02f13a0>]
(sys_ioctl+0x568/0xd44)
[<c02f13a0>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x1c)
Exception stack(0xc4777fa8 to 0xc4777ff0)
7fa0: 004f5210 b6e27394 00000004 00004142 004f5398
004f5398
7fc0: 004f5210 b6e27394 00020000 00000036 00000000 00000000 bee588e8
00008000
7fe0: b6e277c4 bee58874 b6d8e888 b6c751dc
Time per period = 0.253397
max98090 1-0010: PLL unlocked
BUG: sleeping function called from invalid context at
kernel/locking/rwsem.c:1526
in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1312, name:
speaker-test
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
INFO: lockdep is turned off.
irq event stamp: 8158
hardirqs last enabled at (8157): [<c0b747d0>]
_raw_spin_unlock_irqrestore+0x5c/0x60
hardirqs last disabled at (8158): [<c0b74570>] _raw_spin_lock_irq+0x58/0x5c
softirqs last enabled at (7854): [<c0101578>] __do_softirq+0x348/0x610
softirqs last disabled at (7849): [<c012e7a4>] __irq_exit_rcu+0x144/0x1ec
Preemption disabled at:
[<00000000>] 0x0
CPU: 0 PID: 1312 Comm: speaker-test Not tainted
5.16.0-rc1-00270-gb2ae80663008 #11109
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
[<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
[<c0b65d24>] (dump_stack_lvl) from [<c0158b04>]
(__might_resched+0x1c0/0x288)
[<c0158b04>] (__might_resched) from [<c0b71898>] (down_write+0x24/0x8c)
[<c0b71898>] (down_write) from [<c030ed64>]
(simple_recursive_removal+0x6c/0x370)
[<c030ed64>] (simple_recursive_removal) from [<c04d07a4>]
(debugfs_remove+0x38/0x4c)
[<c04d07a4>] (debugfs_remove) from [<c0928784>]
(dpcm_be_disconnect+0x160/0x2c4)
[<c0928784>] (dpcm_be_disconnect) from [<c092895c>]
(dpcm_fe_dai_cleanup+0x74/0xb0)
[<c092895c>] (dpcm_fe_dai_cleanup) from [<c0928d90>]
(dpcm_fe_dai_close+0xe8/0x14c)
[<c0928d90>] (dpcm_fe_dai_close) from [<c090977c>]
(snd_pcm_release_substream.part.0+0x3c/0xcc)
[<c090977c>] (snd_pcm_release_substream.part.0) from [<c0909878>]
(snd_pcm_release+0x54/0xa4)
[<c0909878>] (snd_pcm_release) from [<c02dc400>] (__fput+0x88/0x258)
[<c02dc400>] (__fput) from [<c014cd44>] (task_work_run+0x8c/0xc8)
[<c014cd44>] (task_work_run) from [<c010c08c>]
(do_work_pending+0x4a4/0x598)
[<c010c08c>] (do_work_pending) from [<c0100088>]
(slow_work_pending+0xc/0x20)
Exception stack(0xc4777fb0 to 0xc4777ff8)
7fa0: 00000000 004f5260 004eaa9c
00000000
7fc0: 004f5260 004f536c 004f5210 00000006 004fb700 004e6e8c 004d6120
bee58cc4
7fe0: b6e27e64 bee58928 b6d8eda4 b6d09ac0 60000050 00000004
Let me know how I can help debugging this issue.
> ---
> include/sound/soc.h | 2 -
> sound/soc/soc-core.c | 1 -
> sound/soc/soc-pcm.c | 229 ++++++++++++++++++++++++++++---------------
> 3 files changed, 152 insertions(+), 80 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 dcf6be4c4aaa..1d62160f96b1 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2315,7 +2315,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 3a34b71fd3c1..2e282c42bac2 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -27,6 +27,31 @@
> #include <sound/soc-link.h>
> #include <sound/initval.h>
>
> +static inline void snd_soc_dpcm_mutex_lock(struct snd_soc_pcm_runtime *rtd)
> +{
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> +}
> +
> +static inline void snd_soc_dpcm_mutex_unlock(struct snd_soc_pcm_runtime *rtd)
> +{
> + mutex_unlock(&rtd->card->pcm_mutex);
> +}
> +
> +#define snd_soc_dpcm_mutex_assert_held(rtd) \
> + lockdep_assert_held(&(rtd)->card->pcm_mutex)
> +
> +static inline void snd_soc_dpcm_stream_lock_irq(struct snd_soc_pcm_runtime *rtd,
> + int stream)
> +{
> + snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(rtd, stream));
> +}
> +
> +static inline void snd_soc_dpcm_stream_unlock_irq(struct snd_soc_pcm_runtime *rtd,
> + int stream)
> +{
> + snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(rtd, stream));
> +}
> +
> #define DPCM_MAX_BE_USERS 8
>
> static inline const char *soc_cpu_dai_name(struct snd_soc_pcm_runtime *rtd)
> @@ -73,7 +98,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 +125,6 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
> goto out;
> }
>
> - spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> for_each_dpcm_be(fe, stream, dpcm) {
> struct snd_soc_pcm_runtime *be = dpcm->be;
> params = &dpcm->hw_params;
> @@ -122,7 +145,6 @@ 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);
> out:
> return offset;
> }
> @@ -145,11 +167,13 @@ static ssize_t dpcm_state_read_file(struct file *file, char __user *user_buf,
> if (!buf)
> return -ENOMEM;
>
> + snd_soc_dpcm_mutex_lock(fe);
> for_each_pcm_streams(stream)
> if (snd_soc_dai_stream_valid(asoc_rtd_to_cpu(fe, 0), stream))
> offset += dpcm_show_state(fe, stream,
> buf + offset,
> out_count - offset);
> + snd_soc_dpcm_mutex_unlock(fe);
>
> ret = simple_read_from_buffer(user_buf, count, ppos, buf, offset);
>
> @@ -221,14 +245,14 @@ static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe,
> struct snd_pcm_substream *substream =
> snd_soc_dpcm_get_substream(fe, stream);
>
> - snd_pcm_stream_lock_irq(substream);
> + snd_soc_dpcm_stream_lock_irq(fe, stream);
> if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) {
> dpcm_fe_dai_do_trigger(substream,
> fe->dpcm[stream].trigger_pending - 1);
> fe->dpcm[stream].trigger_pending = 0;
> }
> fe->dpcm[stream].runtime_update = state;
> - snd_pcm_stream_unlock_irq(substream);
> + snd_soc_dpcm_stream_unlock_irq(fe, stream);
> }
>
> static void dpcm_set_be_update_state(struct snd_soc_pcm_runtime *be,
> @@ -256,7 +280,7 @@ void snd_soc_runtime_action(struct snd_soc_pcm_runtime *rtd,
> struct snd_soc_dai *dai;
> int i;
>
> - lockdep_assert_held(&rtd->card->pcm_mutex);
> + snd_soc_dpcm_mutex_assert_held(rtd);
>
> for_each_rtd_dais(rtd, i, dai)
> snd_soc_dai_action(dai, stream, action);
> @@ -309,6 +333,8 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
> {
> struct snd_soc_dpcm *dpcm;
>
> + snd_soc_dpcm_mutex_assert_held(fe);
> +
> for_each_dpcm_be(fe, dir, dpcm) {
>
> struct snd_soc_pcm_runtime *be = dpcm->be;
> @@ -646,14 +672,14 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream,
> return ret;
> }
>
> -static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
> +static int soc_pcm_clean(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream, int rollback)
> {
> - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> struct snd_soc_component *component;
> struct snd_soc_dai *dai;
> int i;
>
> - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> + snd_soc_dpcm_mutex_assert_held(rtd);
>
> if (!rollback)
> snd_soc_runtime_deactivate(rtd, substream->stream);
> @@ -665,9 +691,6 @@ static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
>
> soc_pcm_components_close(substream, rollback);
>
> -
> - mutex_unlock(&rtd->card->pcm_mutex);
> -
> snd_soc_pcm_component_pm_runtime_put(rtd, substream, rollback);
>
> for_each_rtd_components(rtd, i, component)
> @@ -682,9 +705,21 @@ static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
> * freed here. The cpu DAI, codec DAI, machine and components are also
> * shutdown.
> */
> +static int __soc_pcm_close(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream)
> +{
> + return soc_pcm_clean(rtd, substream, 0);
> +}
> +
> +/* PCM close ops for non-DPCM streams */
> static int soc_pcm_close(struct snd_pcm_substream *substream)
> {
> - return soc_pcm_clean(substream, 0);
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +
> + snd_soc_dpcm_mutex_lock(rtd);
> + soc_pcm_clean(rtd, substream, 0);
> + snd_soc_dpcm_mutex_unlock(rtd);
> + return 0;
> }
>
> static int soc_hw_sanity_check(struct snd_pcm_substream *substream)
> @@ -730,21 +765,21 @@ static int soc_hw_sanity_check(struct snd_pcm_substream *substream)
> * then initialized and any private data can be allocated. This also calls
> * startup for the cpu DAI, component, machine and codec DAI.
> */
> -static int soc_pcm_open(struct snd_pcm_substream *substream)
> +static int __soc_pcm_open(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream)
> {
> - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> struct snd_soc_component *component;
> struct snd_soc_dai *dai;
> int i, ret = 0;
>
> + snd_soc_dpcm_mutex_assert_held(rtd);
> +
> for_each_rtd_components(rtd, i, component)
> pinctrl_pm_select_default_state(component->dev);
>
> ret = snd_soc_pcm_component_pm_runtime_get(rtd, substream);
> if (ret < 0)
> - goto pm_err;
> -
> - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> + goto err;
>
> ret = soc_pcm_components_open(substream);
> if (ret < 0)
> @@ -791,16 +826,26 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> snd_soc_runtime_activate(rtd, substream->stream);
> ret = 0;
> err:
> - mutex_unlock(&rtd->card->pcm_mutex);
> -pm_err:
> if (ret < 0) {
> - soc_pcm_clean(substream, 1);
> + soc_pcm_clean(rtd, substream, 1);
> dev_err(rtd->dev, "%s() failed (%d)", __func__, ret);
> }
>
> return ret;
> }
>
> +/* PCM open ops for non-DPCM streams */
> +static int soc_pcm_open(struct snd_pcm_substream *substream)
> +{
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + int ret;
> +
> + snd_soc_dpcm_mutex_lock(rtd);
> + ret = __soc_pcm_open(rtd, substream);
> + snd_soc_dpcm_mutex_unlock(rtd);
> + return ret;
> +}
> +
> static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd)
> {
> /*
> @@ -816,13 +861,13 @@ static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd)
> * rate, etc. This function is non atomic and can be called multiple times,
> * it can refer to the runtime info.
> */
> -static int soc_pcm_prepare(struct snd_pcm_substream *substream)
> +static int __soc_pcm_prepare(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream)
> {
> - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> struct snd_soc_dai *dai;
> int i, ret = 0;
>
> - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> + snd_soc_dpcm_mutex_assert_held(rtd);
>
> ret = snd_soc_link_prepare(substream);
> if (ret < 0)
> @@ -850,14 +895,24 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
> snd_soc_dai_digital_mute(dai, 0, substream->stream);
>
> out:
> - mutex_unlock(&rtd->card->pcm_mutex);
> -
> if (ret < 0)
> dev_err(rtd->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
>
> return ret;
> }
>
> +/* PCM prepare ops for non-DPCM streams */
> +static int soc_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + int ret;
> +
> + snd_soc_dpcm_mutex_lock(rtd);
> + ret = __soc_pcm_prepare(rtd, substream);
> + snd_soc_dpcm_mutex_unlock(rtd);
> + return ret;
> +}
> +
> static void soc_pcm_codec_params_fixup(struct snd_pcm_hw_params *params,
> unsigned int mask)
> {
> @@ -869,13 +924,13 @@ static void soc_pcm_codec_params_fixup(struct snd_pcm_hw_params *params,
> interval->max = channels;
> }
>
> -static int soc_pcm_hw_clean(struct snd_pcm_substream *substream, int rollback)
> +static int soc_pcm_hw_clean(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream, int rollback)
> {
> - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> struct snd_soc_dai *dai;
> int i;
>
> - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> + snd_soc_dpcm_mutex_assert_held(rtd);
>
> /* clear the corresponding DAIs parameters when going to be inactive */
> for_each_rtd_dais(rtd, i, dai) {
> @@ -900,16 +955,28 @@ static int soc_pcm_hw_clean(struct snd_pcm_substream *substream, int rollback)
> if (snd_soc_dai_stream_valid(dai, substream->stream))
> snd_soc_dai_hw_free(dai, substream, rollback);
>
> - mutex_unlock(&rtd->card->pcm_mutex);
> return 0;
> }
>
> /*
> * Frees resources allocated by hw_params, can be called multiple times
> */
> +static int __soc_pcm_hw_free(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream)
> +{
> + return soc_pcm_hw_clean(rtd, substream, 0);
> +}
> +
> +/* hw_free PCM ops for non-DPCM streams */
> static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> {
> - return soc_pcm_hw_clean(substream, 0);
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + int ret;
> +
> + snd_soc_dpcm_mutex_lock(rtd);
> + ret = __soc_pcm_hw_free(rtd, substream);
> + snd_soc_dpcm_mutex_unlock(rtd);
> + return ret;
> }
>
> /*
> @@ -917,15 +984,15 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> * function can also be called multiple times and can allocate buffers
> * (using snd_pcm_lib_* ). It's non-atomic.
> */
> -static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> - struct snd_pcm_hw_params *params)
> +static int __soc_pcm_hw_params(struct snd_soc_pcm_runtime *rtd,
> + struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> {
> - struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> struct snd_soc_dai *cpu_dai;
> struct snd_soc_dai *codec_dai;
> int i, ret = 0;
>
> - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> + snd_soc_dpcm_mutex_assert_held(rtd);
>
> ret = soc_pcm_params_symmetry(substream, params);
> if (ret)
> @@ -997,16 +1064,27 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
>
> ret = snd_soc_pcm_component_hw_params(substream, params);
> out:
> - mutex_unlock(&rtd->card->pcm_mutex);
> -
> if (ret < 0) {
> - soc_pcm_hw_clean(substream, 1);
> + soc_pcm_hw_clean(rtd, substream, 1);
> dev_err(rtd->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
> }
>
> return ret;
> }
>
> +/* hw_params PCM ops for non-DPCM streams */
> +static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> + int ret;
> +
> + snd_soc_dpcm_mutex_lock(rtd);
> + ret = __soc_pcm_hw_params(rtd, substream, params);
> + snd_soc_dpcm_mutex_unlock(rtd);
> + return ret;
> +}
> +
> static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> {
> struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> @@ -1107,7 +1185,8 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
> struct snd_pcm_substream *fe_substream;
> struct snd_pcm_substream *be_substream;
> struct snd_soc_dpcm *dpcm;
> - unsigned long flags;
> +
> + snd_soc_dpcm_mutex_assert_held(fe);
>
> /* only add new dpcms */
> for_each_dpcm_be(fe, stream, dpcm) {
> @@ -1137,10 +1216,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_stream_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_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,
> @@ -1183,8 +1262,10 @@ 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;
>
> + snd_soc_dpcm_mutex_assert_held(fe);
> +
> + snd_soc_dpcm_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",
> @@ -1202,12 +1283,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);
> }
> + snd_soc_dpcm_stream_unlock_irq(fe, stream);
> }
>
> /* get BE for DAI widget and stream */
> @@ -1431,12 +1511,9 @@ 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);
> 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);
> }
>
> void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
> @@ -1472,12 +1549,12 @@ void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
> continue;
>
> if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_HW_FREE) {
> - soc_pcm_hw_free(be_substream);
> + __soc_pcm_hw_free(be, be_substream);
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
> }
> }
>
> - soc_pcm_close(be_substream);
> + __soc_pcm_close(be, be_substream);
> be_substream->runtime = NULL;
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
> }
> @@ -1525,7 +1602,7 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
> stream ? "capture" : "playback", be->dai_link->name);
>
> be_substream->runtime = be->dpcm[stream].runtime;
> - err = soc_pcm_open(be_substream);
> + err = __soc_pcm_open(be, be_substream);
> if (err < 0) {
> be->dpcm[stream].users--;
> if (be->dpcm[stream].users < 0)
> @@ -1769,7 +1846,7 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
> dev_dbg(fe->dev, "ASoC: open FE %s\n", fe->dai_link->name);
>
> /* start the DAI frontend */
> - ret = soc_pcm_open(fe_substream);
> + ret = __soc_pcm_open(fe, fe_substream);
> if (ret < 0)
> goto unwind;
>
> @@ -1800,6 +1877,8 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
> struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
> int stream = substream->stream;
>
> + snd_soc_dpcm_mutex_assert_held(fe);
> +
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>
> /* shutdown the BEs */
> @@ -1808,7 +1887,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
> dev_dbg(fe->dev, "ASoC: close FE %s\n", fe->dai_link->name);
>
> /* now shutdown the frontend */
> - soc_pcm_close(substream);
> + __soc_pcm_close(fe, substream);
>
> /* run the stream stop event */
> dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
> @@ -1853,7 +1932,7 @@ void dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream)
> dev_dbg(be->dev, "ASoC: hw_free BE %s\n",
> be->dai_link->name);
>
> - soc_pcm_hw_free(be_substream);
> + __soc_pcm_hw_free(be, be_substream);
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
> }
> @@ -1864,13 +1943,13 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
> struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
> int stream = substream->stream;
>
> - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> + snd_soc_dpcm_mutex_lock(fe);
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>
> dev_dbg(fe->dev, "ASoC: hw_free FE %s\n", fe->dai_link->name);
>
> /* call hw_free on the frontend */
> - soc_pcm_hw_free(substream);
> + soc_pcm_hw_clean(fe, substream, 0);
>
> /* only hw_params backends that are either sinks or sources
> * to this frontend DAI */
> @@ -1879,7 +1958,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
> fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
>
> - mutex_unlock(&fe->card->mutex);
> + snd_soc_dpcm_mutex_unlock(fe);
> return 0;
> }
>
> @@ -1923,7 +2002,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
> dev_dbg(be->dev, "ASoC: hw_params BE %s\n",
> be->dai_link->name);
>
> - ret = soc_pcm_hw_params(be_substream, &dpcm->hw_params);
> + ret = __soc_pcm_hw_params(be, be_substream, &dpcm->hw_params);
> if (ret < 0)
> goto unwind;
>
> @@ -1953,7 +2032,7 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
> (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP))
> continue;
>
> - soc_pcm_hw_free(be_substream);
> + __soc_pcm_hw_free(be, be_substream);
> }
>
> return ret;
> @@ -1965,7 +2044,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
> struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
> int ret, stream = substream->stream;
>
> - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> + snd_soc_dpcm_mutex_lock(fe);
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>
> memcpy(&fe->dpcm[stream].hw_params, params,
> @@ -1979,7 +2058,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
> params_channels(params), params_format(params));
>
> /* call hw_params on the frontend */
> - ret = soc_pcm_hw_params(substream, params);
> + ret = __soc_pcm_hw_params(fe, substream, params);
> if (ret < 0)
> dpcm_be_dai_hw_free(fe, stream);
> else
> @@ -1987,7 +2066,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>
> out:
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
> - mutex_unlock(&fe->card->mutex);
> + snd_soc_dpcm_mutex_unlock(fe);
>
> if (ret < 0)
> dev_err(fe->dev, "ASoC: %s failed (%d)\n", __func__, ret);
> @@ -2258,7 +2337,7 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream)
> dev_dbg(be->dev, "ASoC: prepare BE %s\n",
> be->dai_link->name);
>
> - ret = soc_pcm_prepare(be_substream);
> + ret = __soc_pcm_prepare(be, be_substream);
> if (ret < 0)
> break;
>
> @@ -2276,7 +2355,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
> struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
> int stream = substream->stream, ret = 0;
>
> - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> + snd_soc_dpcm_mutex_lock(fe);
>
> dev_dbg(fe->dev, "ASoC: prepare FE %s\n", fe->dai_link->name);
>
> @@ -2295,7 +2374,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
> goto out;
>
> /* call prepare on the frontend */
> - ret = soc_pcm_prepare(substream);
> + ret = __soc_pcm_prepare(fe, substream);
> if (ret < 0)
> goto out;
>
> @@ -2303,7 +2382,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
>
> out:
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
> - mutex_unlock(&fe->card->mutex);
> + snd_soc_dpcm_mutex_unlock(fe);
>
> if (ret < 0)
> dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
> @@ -2354,7 +2433,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);
> @@ -2423,7 +2501,6 @@ 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);
> for_each_dpcm_be(fe, stream, dpcm) {
> struct snd_soc_pcm_runtime *be = dpcm->be;
>
> @@ -2435,7 +2512,6 @@ 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);
>
> if (ret < 0)
> dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
> @@ -2510,7 +2586,7 @@ int snd_soc_dpcm_runtime_update(struct snd_soc_card *card)
> struct snd_soc_pcm_runtime *fe;
> int ret = 0;
>
> - mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> + mutex_lock_nested(&card->pcm_mutex, card->pcm_subclass);
> /* shutdown all old paths first */
> for_each_card_rtds(card, fe) {
> ret = soc_dpcm_fe_runtime_update(fe, 0);
> @@ -2526,7 +2602,7 @@ int snd_soc_dpcm_runtime_update(struct snd_soc_card *card)
> }
>
> out:
> - mutex_unlock(&card->mutex);
> + mutex_unlock(&card->pcm_mutex);
> return ret;
> }
> EXPORT_SYMBOL_GPL(snd_soc_dpcm_runtime_update);
> @@ -2537,6 +2613,8 @@ static void dpcm_fe_dai_cleanup(struct snd_pcm_substream *fe_substream)
> struct snd_soc_dpcm *dpcm;
> int stream = fe_substream->stream;
>
> + snd_soc_dpcm_mutex_assert_held(fe);
> +
> /* mark FE's links ready to prune */
> for_each_dpcm_be(fe, stream, dpcm)
> dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
> @@ -2551,12 +2629,12 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream)
> struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(fe_substream);
> int ret;
>
> - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> + snd_soc_dpcm_mutex_lock(fe);
> ret = dpcm_fe_dai_shutdown(fe_substream);
>
> dpcm_fe_dai_cleanup(fe_substream);
>
> - mutex_unlock(&fe->card->mutex);
> + snd_soc_dpcm_mutex_unlock(fe);
> return ret;
> }
>
> @@ -2567,7 +2645,7 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
> int ret;
> int stream = fe_substream->stream;
>
> - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> + snd_soc_dpcm_mutex_lock(fe);
> fe->dpcm[stream].runtime = fe_substream->runtime;
>
> ret = dpcm_path_get(fe, stream, &list);
> @@ -2584,7 +2662,7 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream)
> dpcm_clear_pending_state(fe, stream);
> dpcm_path_put(&list);
> open_end:
> - mutex_unlock(&fe->card->mutex);
> + snd_soc_dpcm_mutex_unlock(fe);
> return ret;
> }
>
> @@ -2845,10 +2923,8 @@ 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);
> for_each_dpcm_fe(be, stream, dpcm) {
>
> if (dpcm->fe == fe)
> @@ -2862,7 +2938,6 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
> }
> }
> }
> - spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
>
> /* it's safe to do this BE DAI */
> return ret;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking
2021-12-17 14:15 ` Marek Szyprowski
@ 2021-12-17 16:54 ` Pierre-Louis Bossart
0 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2021-12-17 16:54 UTC (permalink / raw)
To: Marek Szyprowski, alsa-devel
Cc: 'Linux Samsung SOC',
Kai Vehmanen, Kuninori Morimoto, Liam Girdwood, tiwai, Bard Liao,
Sameer Pujar, Takashi Iwai, linux-kernel, Krzysztof Kozlowski,
Ranjani Sridharan, vkoul, broonie, Gyeongtaek Lee,
Peter Ujfalusi
On 12/17/21 8:15 AM, Marek Szyprowski wrote:
> Dear All,
>
> On 07.12.2021 18:37, Pierre-Louis Bossart wrote:
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> The existing locking for DPCM has several issues
>> a) a confusing mix of card->mutex and card->pcm_mutex.
>> b) a dpcm_lock spinlock added inconsistently and on paths that could
>> be recursively taken. The use of irqsave/irqrestore was also overkill.
>>
>> The suggested model is:
>>
>> 1) The pcm_mutex is the top-most protection of BE links in the FE. The
>> pcm_mutex is applied always on either the top PCM callbacks or the
>> external call from DAPM, not taken in the internal functions.
>>
>> 2) the FE stream lock is taken in higher levels before invoking
>> dpcm_be_dai_trigger()
>>
>> 3) when adding and deleting a BE, both the pcm_mutex and FE stream
>> lock are taken.
>>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> [clarification of commit message by plbossart]
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
>
> This patch recently landed in linux-next (next-20211215) as commit
> b7898396f4bb ("ASoC: soc-pcm: Fix and cleanup DPCM locking"). I found
> that after applying it, a warning is triggered on my test boards. This
> is the one from Exynos4412-based Odroid U3 board:
>
> # speaker-test -l1
>
> speaker-test 1.1.8
>
> Playback device is default
> Stream parameters are 48000Hz, S16_LE, 1 channels
> Using 16 octaves of pink noise
> Rate set to 48000Hz (requested 48000Hz)
> Buffer size range from 128 to 131072
> Period size range from 64 to 65536
> Using max buffer size 131072
> Periods = 4
> was set period_size = 32768
> was set buffer_size = 131072
> 0 - Front Left
>
> ============================================
> WARNING: possible recursive locking detected
> 5.16.0-rc1-00270-gb2ae80663008 #11109 Not tainted
> --------------------------------------------
> speaker-test/1312 is trying to acquire lock:
> c1d78ca4 (&group->lock){....}-{2:2}, at: dpcm_be_dai_trigger+0x80/0x300
>
> but task is already holding lock:
> c1d788a4 (&group->lock){....}-{2:2}, at: snd_pcm_action_lock_irq+0x68/0x7c
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(&group->lock);
> lock(&group->lock);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 1 lock held by speaker-test/1312:
> #0: c1d788a4 (&group->lock){....}-{2:2}, at:
> snd_pcm_action_lock_irq+0x68/0x7c
>
> stack backtrace:
> CPU: 0 PID: 1312 Comm: speaker-test Not tainted
> 5.16.0-rc1-00270-gb2ae80663008 #11109
> Hardware name: Samsung Exynos (Flattened Device Tree)
> [<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
> [<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
> [<c0b65d24>] (dump_stack_lvl) from [<c0193740>]
> (__lock_acquire+0x15ac/0x319c)
> [<c0193740>] (__lock_acquire) from [<c0195dd8>] (lock_acquire+0x14c/0x424)
> [<c0195dd8>] (lock_acquire) from [<c0b745b8>]
> (_raw_spin_lock_irqsave+0x44/0x60)
> [<c0b745b8>] (_raw_spin_lock_irqsave) from [<c0926b6c>]
> (dpcm_be_dai_trigger+0x80/0x300)
> [<c0926b6c>] (dpcm_be_dai_trigger) from [<c0927004>]
> (dpcm_fe_dai_do_trigger+0x124/0x1e4)
> [<c0927004>] (dpcm_fe_dai_do_trigger) from [<c090728c>]
> (snd_pcm_action+0x74/0xb0)
> [<c090728c>] (snd_pcm_action) from [<c0907eac>]
> (snd_pcm_action_lock_irq+0x3c/0x7c)
> [<c0907eac>] (snd_pcm_action_lock_irq) from [<c02f13a0>]
> (sys_ioctl+0x568/0xd44)
> [<c02f13a0>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x1c)
> Exception stack(0xc4777fa8 to 0xc4777ff0)
> 7fa0: 004f5210 b6e27394 00000004 00004142 004f5398
> 004f5398
> 7fc0: 004f5210 b6e27394 00020000 00000036 00000000 00000000 bee588e8
> 00008000
> 7fe0: b6e277c4 bee58874 b6d8e888 b6c751dc
> Time per period = 0.253397
> max98090 1-0010: PLL unlocked
> BUG: sleeping function called from invalid context at
> kernel/locking/rwsem.c:1526
> in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1312, name:
> speaker-test
> preempt_count: 1, expected: 0
> RCU nest depth: 0, expected: 0
> INFO: lockdep is turned off.
> irq event stamp: 8158
> hardirqs last enabled at (8157): [<c0b747d0>]
> _raw_spin_unlock_irqrestore+0x5c/0x60
> hardirqs last disabled at (8158): [<c0b74570>] _raw_spin_lock_irq+0x58/0x5c
> softirqs last enabled at (7854): [<c0101578>] __do_softirq+0x348/0x610
> softirqs last disabled at (7849): [<c012e7a4>] __irq_exit_rcu+0x144/0x1ec
> Preemption disabled at:
> [<00000000>] 0x0
> CPU: 0 PID: 1312 Comm: speaker-test Not tainted
> 5.16.0-rc1-00270-gb2ae80663008 #11109
> Hardware name: Samsung Exynos (Flattened Device Tree)
> [<c0110b30>] (unwind_backtrace) from [<c010c618>] (show_stack+0x10/0x14)
> [<c010c618>] (show_stack) from [<c0b65d24>] (dump_stack_lvl+0x58/0x70)
> [<c0b65d24>] (dump_stack_lvl) from [<c0158b04>]
> (__might_resched+0x1c0/0x288)
> [<c0158b04>] (__might_resched) from [<c0b71898>] (down_write+0x24/0x8c)
> [<c0b71898>] (down_write) from [<c030ed64>]
> (simple_recursive_removal+0x6c/0x370)
> [<c030ed64>] (simple_recursive_removal) from [<c04d07a4>]
> (debugfs_remove+0x38/0x4c)
> [<c04d07a4>] (debugfs_remove) from [<c0928784>]
> (dpcm_be_disconnect+0x160/0x2c4)
> [<c0928784>] (dpcm_be_disconnect) from [<c092895c>]
> (dpcm_fe_dai_cleanup+0x74/0xb0)
> [<c092895c>] (dpcm_fe_dai_cleanup) from [<c0928d90>]
> (dpcm_fe_dai_close+0xe8/0x14c)
> [<c0928d90>] (dpcm_fe_dai_close) from [<c090977c>]
> (snd_pcm_release_substream.part.0+0x3c/0xcc)
> [<c090977c>] (snd_pcm_release_substream.part.0) from [<c0909878>]
> (snd_pcm_release+0x54/0xa4)
> [<c0909878>] (snd_pcm_release) from [<c02dc400>] (__fput+0x88/0x258)
> [<c02dc400>] (__fput) from [<c014cd44>] (task_work_run+0x8c/0xc8)
> [<c014cd44>] (task_work_run) from [<c010c08c>]
> (do_work_pending+0x4a4/0x598)
> [<c010c08c>] (do_work_pending) from [<c0100088>]
> (slow_work_pending+0xc/0x20)
> Exception stack(0xc4777fb0 to 0xc4777ff8)
> 7fa0: 00000000 004f5260 004eaa9c
> 00000000
> 7fc0: 004f5260 004f536c 004f5210 00000006 004fb700 004e6e8c 004d6120
> bee58cc4
> 7fe0: b6e27e64 bee58928 b6d8eda4 b6d09ac0 60000050 00000004
>
> Let me know how I can help debugging this issue.
Thanks for testing, much appreciated.
I wasn't really able to detect a smoking gun from the stack trace, but
this seems to point to the use of the FE stream lock
"
2) the FE stream lock is taken in higher levels before invoking
dpcm_be_dai_trigger()
"
I am not sure why we would attempt taking this lock multiple times though.
One possibility (chance of red-herring) is the code in
dpcm_set_fe_update_state(). The comments do mention the possibility of a
race condition and convoluted code. Would you be able to instrument this
code to see if the lock issue happens when this function is invoked?
Takashi may have other ideas to help debug? On our side this has been
used for several weeks and tested in all kinds of configurations.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE
2023-05-11 12:08 [PATCH 0/6] Fix soc compress playback deadlock in 5.15 yixuanjiang
@ 2023-05-11 12:08 ` yixuanjiang
0 siblings, 0 replies; 13+ messages in thread
From: yixuanjiang @ 2023-05-11 12:08 UTC (permalink / raw)
To: tiwai, lgirdwood, broonie
Cc: linux-kernel, alsa-devel, Pierre-Louis Bossart, Takashi Iwai,
Kai Vehmanen, Bard Liao, Ranjani Sridharan, Yixuan Jiang, stable
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
[ Upstream commit bbf7d3b1c4f40eb02dd1dffb500ba00b0bff0303 ]
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>
[ removed FE stream lock by tiwai ]
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20211207173745.15850-3-pierre-louis.bossart@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Fixes: aa9ff6a4955f ("ASoC: soc-compress: Reposition and add pcm_mutex")
Signed-off-by: Yixuan Jiang <yixuanjiang@google.com>
Cc: stable@vger.kernel.org # 5.15+
---
sound/soc/soc-pcm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index cffae9b7c2548..373f20bd14301 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1123,6 +1123,8 @@ 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;
unsigned long flags;
@@ -1132,6 +1134,20 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
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__);
+ 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;
+ }
+
dpcm = kzalloc(sizeof(struct snd_soc_dpcm), GFP_ATOMIC);
if (!dpcm)
return -ENOMEM;
--
2.40.1.521.gf1e218fcd8-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-05-11 18:57 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-07 17:37 [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 1/6] ASoC: soc-pcm: use GFP_ATOMIC for dpcm structure Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 3/6] ASoC: soc-pcm: Fix and cleanup DPCM locking Pierre-Louis Bossart
[not found] ` <CGME20211217141541eucas1p1bdcb8b91e8a772229391b525d6adbf3b@eucas1p1.samsung.com>
2021-12-17 14:15 ` Marek Szyprowski
2021-12-17 16:54 ` Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 4/6] ASoC: soc-pcm: serialize BE triggers Pierre-Louis Bossart
[not found] ` <CGME20211217101152eucas1p14ea09622edde5118ddd832c3ebaebdde@eucas1p1.samsung.com>
2021-12-17 10:11 ` Marek Szyprowski
[not found] ` <CGME20211217115743eucas1p10e721164cd845176f9dc5c4c2ad86838@eucas1p1.samsung.com>
2021-12-17 11:57 ` Marek Szyprowski
2021-12-07 17:37 ` [PATCH 5/6] ASoC: soc-pcm: test refcount before triggering Pierre-Louis Bossart
2021-12-07 17:37 ` [PATCH 6/6] ASoC: soc-pcm: fix BE handling of PAUSE_RELEASE Pierre-Louis Bossart
2021-12-15 2:04 ` [PATCH 0/6] ASoC : soc-pcm: fix trigger race conditions with shared BE Mark Brown
2023-05-11 12:08 [PATCH 0/6] Fix soc compress playback deadlock in 5.15 yixuanjiang
2023-05-11 12:08 ` [PATCH 2/6] ASoC: soc-pcm: align BE 'atomicity' with that of the FE yixuanjiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).