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

[Manuel Ebner <manuelebner@mailbox.org>]

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>