ASoC: meson: gx-formatter: change stream lock from mutex to spinlock
v2 cover ASoC: meson: gx-formatter: change stream lock from mutex to spinlock
Patch 1 fixes a pre-existing bugs, present since v7.3, reported by Sashiko [1] while reviewing the series adding I2S input support for Amlogic GX SoCs [2].
[2] depends on patch 1 to address Sashiko's comment.
Patch 2 adds a hardening check suggested by Martin while reviewing v1.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
---
Changes in v2:
- Moved 'gx_formatter_disable()' under the spinlock guard in
'gx_formatter_detach'. [Martin]
- Added a new commit to introduce the 'lockdep_assert_held' check before
calling formatter's prepare/enable/disable callbacks. [Martin]
- Link to v1: https://patch.msgid.link/20261021-gx-formatter-use-spinlock-v1-1-71ebc9224c03@baylibre.com
To: Jerome Brunet <[email protected]>
To: Liam Girdwood <[email protected]>
To: Mark Brown <[email protected]>
To: Jaroslav Kysela <[email protected]>
To: Takashi Iwai <[email protected]>
To: Neil Armstrong <[email protected]>
To: Kevin Hilman <[email protected]>
To: Martin Blumenstingl <[email protected]>
To: Valerio Setti <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Valerio Setti (2):
ASoC: meson: gx-formatter: change stream lock from mutex to spinlock
ASoC: meson: gx-formatter: add lockdep_assert_held check
sound/soc/meson/gx-formatter.c | 26 +++++++++++++++----------- sound/soc/meson/gx-formatter.h | 4 ++++ sound/soc/meson/gx-interface.h | 3 ++- 3 files changed, 21 insertions(+), 12 deletions(-)
---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261021-gx-formatter-use-spinlock-289b3e2e46df
Best regards,
Signature · 1 line
Valerio Setti <[email protected]>
PATCH v2 1/2 ASoC: meson: gx-formatter: change stream lock from mutex to spinlock
Currently a mutex is being used in 'struct gx_stream' to serialize access to structure data. This was inherited from the AXG design. An important difference though is that in axg-card dai_links are nonatomic so callback functions like 'trigger' are allowed to sleep. In GX instead dai_links are atomic.
For example, starting playback from userspace with 'CONFIG_DEBUG_ATOMIC_SLEEP=y' results in a warning message like:
[ ] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:316 [ ] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 272, name: speaker-test [ ] preempt_count: 1, expected: 0
due to the following call stack:
snd_pcm_do_start (called with spinlock taken)
dpcm_fe_dai_trigger
dpcm_fe_dai_do_trigger
dpcm_dai_trigger_fe_be
dpcm_be_dai_trigger
soc_pcm_trigger
snd_soc_pcm_dai_trigger
soc_dai_trigger
aiu_encoder_i2s_trigger
gx_stream_start (takes 'ts->lock')Fix this by changing the lock type from mutex to spinlock. This is safe on GX platform because all formatters' functions only perform MMIO regmap calls that do not sleep.
'spin_lock_irqsave' is used in 'gx_stream_start'/'gx_stream_stop' because trigger runs with IRQs already disabled and stop can also run in IRQ context (e.g. in case of XRUN while executing 'aiu_fifo_isr'). 'spin_lock_irq' is instead used in 'gx_formatter_attach'/ 'gx_formatter_detach' in order to prevent IRQs on the same CPU that might end up calling 'gx_stream_stop' and therefore deadlock.
While at this also improve 'gx_formatter_detach()' by moving 'gx_formatter_disable()' under the spinlock guard. This makes it symmetric to 'gx_formatter_attach()' and ensures that the prepare/enable/disable formatter callbacks are called with spinlock held.
---
Changes in v2:
- Moved 'gx_formatter_disable()' under the spinlock guard in 'gx_formatter_detach'. [Martin]
--- sound/soc/meson/gx-formatter.c | 22 +++++++++++----------- sound/soc/meson/gx-formatter.h | 4 ++++ sound/soc/meson/gx-interface.h | 3 ++- 3 files changed, 17 insertions(+), 12 deletions(-)
sound/soc/meson/gx-formatter.c +11 -11
@@ -61,7 +61,7 @@ static int gx_formatter_attach(struct gx_formatter *formatter)struct gx_stream *ts = formatter->stream;int ret =0 ; -mutex_lock (&ts->lock); +spin_lock_irq (&ts->lock);/* Catch up if the stream is already running when we attach */ if (ts->ready) {
@@ -74,7 +74,7 @@ static int gx_formatter_attach(struct gx_formatter *formatter)list_add_tail (&formatter->list, &ts->formatter_list); out: -mutex_unlock (&ts->lock); +spin_unlock_irq (&ts->lock);return ret; }
@@ -85,11 +85,10 @@ static void gx_formatter_detach(struct gx_formatter *formatter)if (!ts)return ; -mutex_lock (&ts->lock); +spin_lock_irq (&ts->lock);list_del (&formatter->list); -mutex_unlock (&ts->lock); -gx_formatter_disable (formatter); +spin_unlock_irq (&ts->lock); } static int gx_formatter_power_up(struct gx_formatter *formatter,
@@ -212,9 +211,10 @@ EXPORT_SYMBOL_GPL(gx_formatter_create);int gx_stream_start (struct gx_stream *ts) {struct gx_formatter *formatter; +unsigned long irq_flags;int ret =0 ; -mutex_lock (&ts->lock); +spin_lock_irqsave (&ts->lock, irq_flags);/* Start all the formatters attached to the stream */ list_for_each_entry (formatter, &ts->formatter_list, list) {
@@ -228,7 +228,7 @@ int gx_stream_start(struct gx_stream *ts) ts->ready = true; out: -mutex_unlock (&ts->lock); +spin_unlock_irqrestore (&ts->lock, irq_flags);return ret; }EXPORT_SYMBOL_GPL (gx_stream_start);
@@ -236,8 +236,9 @@ EXPORT_SYMBOL_GPL(gx_stream_start);void gx_stream_stop (struct gx_stream *ts) {struct gx_formatter *formatter; +unsigned long irq_flags; -mutex_lock (&ts->lock); +spin_lock_irqsave (&ts->lock, irq_flags); ts->ready = false;/* Stop all the formatters attached to the stream */
@@ -245,7 +246,7 @@ void gx_stream_stop(struct gx_stream *ts)gx_formatter_disable (formatter); } -mutex_unlock (&ts->lock); +spin_unlock_irqrestore (&ts->lock, irq_flags); }EXPORT_SYMBOL_GPL (gx_stream_stop);
@@ -256,7 +257,7 @@ struct gx_stream *gx_stream_alloc(struct gx_iface *iface) ts =kzalloc_obj (*ts);if (ts) {INIT_LIST_HEAD (&ts->formatter_list); -mutex_init (&ts->lock); +spin_lock_init (&ts->lock); ts->iface = iface; }
@@ -272,7 +273,6 @@ void gx_stream_free(struct gx_stream *ts)* are removing the TDM DAI. It should not be possible */ WARN_ON (!list_empty (&ts->formatter_list)); -mutex_destroy (&ts->lock);kfree (ts); }EXPORT_SYMBOL_GPL (gx_stream_free);
sound/soc/meson/gx-formatter.h +4 -0
@@ -20,6 +20,10 @@ struct gx_formatter_hw {struct gx_formatter_ops {struct gx_stream *(*get_stream)(struct snd_soc_dapm_widget *w); +/* +* Following functions are called with gx_stream->lock held and local +* IRQs disabled so they must not sleep. +*/ void (*enable)(struct regmap *map);void (*disable)(struct regmap *map); int (*prepare)(struct regmap *map,
sound/soc/meson/gx-interface.h +2 -1
@@ -9,6 +9,7 @@#include <linux/clk.h> #include <linux/regmap.h> +#include <linux/spinlock.h> #include <sound/pcm.h> #include <sound/soc.h> #include <sound/soc-dai.h>
@@ -24,7 +25,7 @@ struct gx_iface {struct gx_stream {struct gx_iface *iface;struct list_head formatter_list; -struct mutex lock; +spinlock_t lock;unsigned int channels;unsigned int width;unsigned int physical_width;
Signature · 1 line
2.47.3
PATCH v2 2/2 ASoC: meson: gx-formatter: add lockdep_assert_held check
In reply to Valerio SettiThe documentation of 'struct gx_formatter_ops' states that formatter's callbacks for enable/disable/prepare are called with the spinlock held. Validate this by adding 'lockdep_assert_held()' in 'gx_formatter_enable'/'gx_formatter_disable'.
--- sound/soc/meson/gx-formatter.c | 4 ++++ 1 file changed, 4 insertions(+)
sound/soc/meson/gx-formatter.c +4 -0
@@ -22,6 +22,8 @@ static int gx_formatter_enable(struct gx_formatter *formatter) {int ret; +lockdep_assert_held (&formatter->stream->lock); +/* Do nothing if the formatter is already enabled */ if (formatter->enabled)return 0 ;
@@ -46,6 +48,8 @@ static int gx_formatter_enable(struct gx_formatter *formatter)static void gx_formatter_disable (struct gx_formatter *formatter) { +lockdep_assert_held (&formatter->stream->lock); +/* Do nothing if the formatter is already disabled */ if (!formatter->enabled)return ;
Signature · 1 line
2.47.3