[PATCH] ASoC: meson: aiu-fifo: prevent sleeping when spinlock is held

[PATCH] ASoC: meson: aiu-fifo: prevent sleeping when spinlock is held

[Valerio Setti]

dai_links in the gx-card are atomic which means that the DAI trigger
callback is called with the spinlock held. This means that usage of
potentially sleeping functions (e.g. mutexes) is not allowed here.

'snd_soc_component_write'/'snd_soc_component_read' take the component's
io_mutex on entry.

Replace usage of these functions in:
- 'aiu_fifo_i2s_trigger'
- 'fifo_spdif_trigger'
- 'aiu_fifo_pointer'

This is meant to resolve the following runtime bug found while starting I2S
playback on an OdroidC2 device where the kernel was built enabling
CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING:

  Playback device is hw:0,0
  Stream parameters are 44100Hz, S16_LE, 2 channels
  Sine wave rate is 1000.0000Hz
  Rate set to 44100Hz (requested 44100Hz)
  Buffer size range from 128 to 262144
  Period size range from 64 [   21.012074] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:623
  [   21.018009] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 267, name: speaker-test
  [   21.026184] preempt_count: 1, expected: 0
  [   21.030151] RCU nest depth: 0, expected: 0
  [   21.034205] locks held by speaker-test/267: 1, last CPU#3:
  [   21.039638]  #0: ffff000003dbd910 (&group->lock#2){....}-{3:3}, at: snd_pcm_stream_lock_irq+0x40/0x60
  [   21.048782] irq event stamp: 5274
  [   21.052059] hardirqs last  enabled at (5273): [<ffff800081494240>] _raw_spin_unlock_irq+0x38/0x80
  [   21.060856] hardirqs last disabled at (5274): [<ffff8000814935c8>] _raw_spin_lock_irq+0x80/0x90
  [   21.069481] softirqs last  enabled at (5238): [<ffff8000800f9aec>] handle_softirqs+0x584/0x5e0
  [   21.078020] softirqs last disabled at (5231): [<ffff800080010324>] __do_softirq+0x1c/0x28
  [   21.086133] CPU: 3 UID: 0 PID: 267 Comm: speaker-test Not tainted 7.3.0 #2 PREEMPT
  [   21.086141] Hardware name: Hardkernel ODROID-C2 (DT)
  [   21.086145] Call trace:
  [   21.086149]  show_stack+0x20/0x40 (C)
  [   21.086160]  dump_stack_lvl+0x6c/0x98
  [   21.086171]  dump_stack+0x18/0x24
  [   21.086179]  __might_resched+0x160/0x230
  [   21.086189]  __might_sleep+0x50/0x90
  [   21.086197]  __mutex_lock+0x64/0x8a0
  [   21.086206]  mutex_lock_nested+0x2c/0x40
  [   21.086215]  snd_soc_component_write+0x38/0x80
  [   21.086227]  aiu_fifo_i2s_trigger+0x74/0x90
  [   21.086237]  soc_dai_trigger.part.0+0x44/0x90
  [   21.086246]  snd_soc_pcm_dai_trigger+0x174/0x280
  [   21.086255]  soc_pcm_trigger+0xe4/0x1f0
  [   21.086265]  dpcm_dai_trigger_fe_be+0x30/0xb0
  [   21.086276]  dpcm_fe_dai_do_trigger+0x11c/0x1a0
  [   21.086287]  dpcm_fe_dai_trigger+0x40/0x60
  [   21.086296]  snd_pcm_do_start+0x4c/0x80
  [   21.086307]  snd_pcm_action_single+0x50/0xc0
  [   21.086316]  snd_pcm_action+0x88/0xb0
  [   21.086325]  snd_pcm_start+0x2c/0x40
  [   21.086335]  __snd_pcm_lib_xfer+0x660/0x810
  [   21.086345]  snd_pcm_common_ioctl+0xa20/0x17e0
  [   21.086356]  snd_pcm_ioctl+0x3c/0x60
  [   21.086366]  __arm64_sys_ioctl+0xac/0x110
  [   21.086376]  invoke_syscall+0x5c/0x120
  [   21.086386]  el0_svc_common.constprop.0+0x48/0xf0
  [   21.086395]  do_el0_svc+0x24/0x40
  [   21.086404]  el0_svc+0x54/0x3b0
  [   21.086414]  el0t_64_sync_handler+0xa0/0xf0
  [   21.086424]  el0t_64_sync+0x198/0x1a0

Fixes: e8712315c44d ("ASoC: soc-component: use io_mutex correctly")
Fixes: 6b59c53c8adc ("ASoC: meson: aiu: fifo-spdif: soft reset the S/PDIF datapath on start/stop")
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
I found this issue while working on [1].
This patch is independent of [1] and they can go in in any order. They
are fixing the same problem in different modules.

[1]: https://lore.kernel.org/all/20261021-gx-formatter-use-spinlock-v1-1-71ebc9224c03@baylibre.com/
---
 sound/soc/meson/aiu-fifo-i2s.c   | 15 ++++++++++++---
 sound/soc/meson/aiu-fifo-spdif.c | 14 ++++++++++----
 sound/soc/meson/aiu-fifo.c       |  6 +++++-
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/sound/soc/meson/aiu-fifo-i2s.c b/sound/soc/meson/aiu-fifo-i2s.c
index eccbc16b293ad..8f785241585df 100644
--- a/sound/soc/meson/aiu-fifo-i2s.c
+++ b/sound/soc/meson/aiu-fifo-i2s.c
@@ -5,6 +5,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/regmap.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-dai.h>
@@ -48,14 +49,22 @@ static int aiu_fifo_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 				struct snd_soc_dai *dai)
 {
 	struct snd_soc_component *component = dai->component;
+	unsigned int val;
+	int ret;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		snd_soc_component_write(component, AIU_RST_SOFT,
-					AIU_RST_SOFT_I2S_FAST);
-		snd_soc_component_read(component, AIU_I2S_SYNC);
+		ret = regmap_write(component->regmap, AIU_RST_SOFT, AIU_RST_SOFT_I2S_FAST);
+		if (ret < 0)
+			return snd_soc_ret(component->dev, ret,
+					   "Failed to write register on trigger");
+
+		ret = regmap_read(component->regmap, AIU_I2S_SYNC, &val);
+		if (ret < 0)
+			return snd_soc_ret(component->dev, ret,
+					   "Failed to read register on trigger");
 		break;
 	}
 
diff --git a/sound/soc/meson/aiu-fifo-spdif.c b/sound/soc/meson/aiu-fifo-spdif.c
index a9861c5d66374..297834bce1c7a 100644
--- a/sound/soc/meson/aiu-fifo-spdif.c
+++ b/sound/soc/meson/aiu-fifo-spdif.c
@@ -69,15 +69,21 @@ static int fifo_spdif_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		snd_soc_component_write(component, AIU_RST_SOFT,
-					AIU_RST_SOFT_958_FAST);
+		ret = regmap_write(component->regmap, AIU_RST_SOFT, AIU_RST_SOFT_958_FAST);
+		if (ret < 0)
+			return snd_soc_ret(component->dev, ret,
+					   "Failed to write register on trigger");
+
 		fifo_spdif_dcu_enable(component, true);
 		break;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_STOP:
-		snd_soc_component_write(component, AIU_RST_SOFT,
-					AIU_RST_SOFT_958_FAST);
+		ret = regmap_write(component->regmap, AIU_RST_SOFT, AIU_RST_SOFT_958_FAST);
+		if (ret < 0)
+			return snd_soc_ret(component->dev, ret,
+					   "Failed to write register on trigger");
+
 		fifo_spdif_dcu_enable(component, false);
 		break;
 	default:
diff --git a/sound/soc/meson/aiu-fifo.c b/sound/soc/meson/aiu-fifo.c
index 8b54f9681d00c..f3cdcce1ebfe3 100644
--- a/sound/soc/meson/aiu-fifo.c
+++ b/sound/soc/meson/aiu-fifo.c
@@ -5,6 +5,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/regmap.h>
 #include <linux/dma-mapping.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -37,8 +38,11 @@ snd_pcm_uframes_t aiu_fifo_pointer(struct snd_soc_component *component,
 	struct aiu_fifo *fifo = snd_soc_dai_dma_data_get_playback(dai);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	unsigned int addr;
+	int ret;
 
-	addr = snd_soc_component_read(component, fifo->mem_offset + AIU_MEM_RD);
+	ret = regmap_read(component->regmap, fifo->mem_offset + AIU_MEM_RD, &addr);
+	if (ret < 0)
+		return snd_soc_ret(component->dev, ret, "Failed to read FIFO pointer");
 
 	return bytes_to_frames(runtime, addr - (unsigned int)runtime->dma_addr);
 }

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261111-aiu-fifo-fix-sleep-on-spinlock-0ecec8d64a9b

Best regards,
--  
Valerio Setti <vsetti@baylibre.com>

Re: [PATCH] ASoC: meson: aiu-fifo: prevent sleeping when spinlock is held

[Martin Blumenstingl]

On Wed, Nov 11, 2026 at 11:09 AM Valerio Setti <vsetti@baylibre.com> wrote:
[...]
> @@ -37,8 +38,11 @@ snd_pcm_uframes_t aiu_fifo_pointer(struct snd_soc_component *component,
>         struct aiu_fifo *fifo = snd_soc_dai_dma_data_get_playback(dai);
>         struct snd_pcm_runtime *runtime = substream->runtime;
>         unsigned int addr;
> +       int ret;
>
> -       addr = snd_soc_component_read(component, fifo->mem_offset + AIU_MEM_RD);
> +       ret = regmap_read(component->regmap, fifo->mem_offset + AIU_MEM_RD, &addr);
> +       if (ret < 0)
> +               return snd_soc_ret(component->dev, ret, "Failed to read FIFO pointer");
As far as I understand:
- snd_pcm_uframes_t is "unsigned long"
- snd_soc_ret() returns a (signed) int

Doesn't that lead to a signed to unsigned conversion error (highest
bit set -> large value)?

axg-fifo simply discards regmap_read's return value.
That seems odd at first, but knowing that its regmap is MMIO backed
(same as for AIU) means there can't ever be any error.


Best regards,
Martin

Re: [PATCH] ASoC: meson: aiu-fifo: prevent sleeping when spinlock is held

[Valerio Setti]

Hi Martin

On 11/13/26 12:05 AM, Martin Blumenstingl wrote:
> On Wed, Nov 11, 2026 at 11:09 AM Valerio Setti <vsetti@baylibre.com> wrote:
> [...]
>> @@ -37,8 +38,11 @@ snd_pcm_uframes_t aiu_fifo_pointer(struct snd_soc_component *component,
>>          struct aiu_fifo *fifo = snd_soc_dai_dma_data_get_playback(dai);
>>          struct snd_pcm_runtime *runtime = substream->runtime;
>>          unsigned int addr;
>> +       int ret;
>>
>> -       addr = snd_soc_component_read(component, fifo->mem_offset + AIU_MEM_RD);
>> +       ret = regmap_read(component->regmap, fifo->mem_offset + AIU_MEM_RD, &addr);
>> +       if (ret < 0)
>> +               return snd_soc_ret(component->dev, ret, "Failed to read FIFO pointer");
> As far as I understand:
> - snd_pcm_uframes_t is "unsigned long"
> - snd_soc_ret() returns a (signed) int
> 
> Doesn't that lead to a signed to unsigned conversion error (highest
> bit set -> large value)?

That's something that made me thinking a bit as well and the reason why 
I implemented it this way is the following.
'snd_soc_component_read' internally goes into 
'soc_component_read_no_lock' and then to 'regmap_read'. If the latter 
fails then 'return soc_component_ret_reg_rw()' is called and that calls 
'return snd_soc_ret()' internally.

So my goal was to keep the same return value in case of errors.

> axg-fifo simply discards regmap_read's return value.
> That seems odd at first, but knowing that its regmap is MMIO backed
> (same as for AIU) means there can't ever be any error.

That's a good point indeed. I will follow this design then.
Thanks for pointing this out.

-- 
Valerio

Re: [PATCH] ASoC: meson: aiu-fifo: prevent sleeping when spinlock is held

[Manuel Ebner]

Hi Valerio,

I have read your patch and have a question.

On Wed, 2026-11-11 at 11:09 +0100, Valerio Setti wrote:
> dai_links in the gx-card are atomic which means that the DAI trigger
> callback is called with the spinlock held. This means that usage of
> potentially sleeping functions (e.g. mutexes) is not allowed here.
> 
> 'snd_soc_component_write'/'snd_soc_component_read' take the component's
> io_mutex on entry.
> 
> Replace usage of these functions in:
> - 'aiu_fifo_i2s_trigger'
> - 'fifo_spdif_trigger'
> - 'aiu_fifo_pointer'
> 
> This is meant to resolve the following runtime bug found while starting I2S
> playback on an OdroidC2 device where the kernel was built enabling
> CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING:
> 
> [...]
> 
> Fixes: e8712315c44d ("ASoC: soc-component: use io_mutex correctly")
> Fixes: 6b59c53c8adc ("ASoC: meson: aiu: fifo-spdif: soft reset the S/PDIF datapath on start/stop")
> Signed-off-by: Valerio Setti <vsetti@baylibre.com>
> ---
> I found this issue while working on [1].
> This patch is independent of [1] and they can go in in any order. They
> are fixing the same problem in different modules.
> 
> [1]: https://lore.kernel.org/all/20261021-gx-formatter-use-spinlock-v1-1-71ebc9224c03@baylibre.com/
> ---
>  sound/soc/meson/aiu-fifo-i2s.c   | 15 ++++++++++++---
>  sound/soc/meson/aiu-fifo-spdif.c | 14 ++++++++++----
>  sound/soc/meson/aiu-fifo.c       |  6 +++++-
>  3 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/meson/aiu-fifo-i2s.c b/sound/soc/meson/aiu-fifo-i2s.c
> index eccbc16b293ad..8f785241585df 100644
> --- a/sound/soc/meson/aiu-fifo-i2s.c
> +++ b/sound/soc/meson/aiu-fifo-i2s.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
> +#include <linux/regmap.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
>  #include <sound/soc-dai.h>
> @@ -48,14 +49,22 @@ static int aiu_fifo_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  				struct snd_soc_dai *dai)
>  {
>  	struct snd_soc_component *component = dai->component;
> +	unsigned int val;

Here is val.

> +	int ret;
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		snd_soc_component_write(component, AIU_RST_SOFT,
> -					AIU_RST_SOFT_I2S_FAST);
> -		snd_soc_component_read(component, AIU_I2S_SYNC);
> +		ret = regmap_write(component->regmap, AIU_RST_SOFT, AIU_RST_SOFT_I2S_FAST);
> +		if (ret < 0)
> +			return snd_soc_ret(component->dev, ret,
> +					   "Failed to write register on trigger");
> +
> +		ret = regmap_read(component->regmap, AIU_I2S_SYNC, &val);

So as I understand val/&val is just there to listen and forget what gets returned.
That's kinda weird, but not an issue.

> +		if (ret < 0)
> +			return snd_soc_ret(component->dev, ret,
> +					   "Failed to read register on trigger");
>  		break;
>  	}
>  
> diff --git a/sound/soc/meson/aiu-fifo-spdif.c b/sound/soc/meson/aiu-fifo-spdif.c
> index a9861c5d66374..297834bce1c7a 100644
> --- a/sound/soc/meson/aiu-fifo-spdif.c
> +++ b/sound/soc/meson/aiu-fifo-spdif.c
> @@ -69,15 +69,21 @@ static int fifo_spdif_trigger(struct snd_pcm_substream *substream, int cmd,
>  	case SNDRV_PCM_TRIGGER_START:
>  	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> -		snd_soc_component_write(component, AIU_RST_SOFT,
> -					AIU_RST_SOFT_958_FAST);
> +		ret = regmap_write(component->regmap, AIU_RST_SOFT, AIU_RST_SOFT_958_FAST);
> +		if (ret < 0)
> +			return snd_soc_ret(component->dev, ret,
> +					   "Failed to write register on trigger");
> +
>  		fifo_spdif_dcu_enable(component, true);
>  		break;
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  	case SNDRV_PCM_TRIGGER_STOP:
> -		snd_soc_component_write(component, AIU_RST_SOFT,
> -					AIU_RST_SOFT_958_FAST);
> +		ret = regmap_write(component->regmap, AIU_RST_SOFT, AIU_RST_SOFT_958_FAST);
> +		if (ret < 0)
> +			return snd_soc_ret(component->dev, ret,
> +					   "Failed to write register on trigger");
> +
>  		fifo_spdif_dcu_enable(component, false);
>  		break;
>  	default:
> diff --git a/sound/soc/meson/aiu-fifo.c b/sound/soc/meson/aiu-fifo.c
> index 8b54f9681d00c..f3cdcce1ebfe3 100644
> --- a/sound/soc/meson/aiu-fifo.c
> +++ b/sound/soc/meson/aiu-fifo.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
> +#include <linux/regmap.h>
>  #include <linux/dma-mapping.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
> @@ -37,8 +38,11 @@ snd_pcm_uframes_t aiu_fifo_pointer(struct snd_soc_component *component,
>  	struct aiu_fifo *fifo = snd_soc_dai_dma_data_get_playback(dai);
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	unsigned int addr;

Here it's addr. It's not yours, you just kept it.

> +	int ret;
>  
> -	addr = snd_soc_component_read(component, fifo->mem_offset + AIU_MEM_RD);
> +	ret = regmap_read(component->regmap, fifo->mem_offset + AIU_MEM_RD, &addr);

Similar as above. But why is addr here and val above? I think it's the same role in regmap_read().
Would it make sense to name both the same? If it does, I'm in favor of val, because that's in the
kernel-doc of regmap_read().

drivers/base/regmap/regmap.c:
> /**
>  * regmap_read() - Read a value from a single register
>  *
>  * @map: Register map to read from
>  * @reg: Register to be read from
>  * @val: Pointer to store read value


> +	if (ret < 0)
> +		return snd_soc_ret(component->dev, ret, "Failed to read FIFO pointer");
>  
>  	return bytes_to_frames(runtime, addr - (unsigned int)runtime->dma_addr);
Here addr is used, so it's not same as above.

Looking forward to your POV.
 Manuel

>  }
> 
> ---
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> change-id: 20261111-aiu-fifo-fix-sleep-on-spinlock-0ecec8d64a9b
> 
> Best regards,
> --  
> Valerio Setti <vsetti@baylibre.com>

Re: [PATCH] ASoC: meson: aiu-fifo: prevent sleeping when spinlock is held

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On Fri, Nov 13, 2026 at 03:54:29PM +0100, Manuel Ebner wrote:
> Hi Valerio,
> 
> I have read your patch and have a question.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]