Re: [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Double free / Use-After-Free memory corruption on DAI probe failure path.
- [High] Sleeping in atomic context within the PCM trigger callback.
- [High] Custom interface rate symmetry logic permanently locks the sample rate.
- [High] Incomplete parameter symmetry allows concurrent streams to corrupt the shared hardware clock divider.
- [High] Unsafe hardware soft-resets in `hw_params` corrupt active streams.
- [Medium] Data race in manual rate symmetry enforcement.
- [Medium] Integer underflow when programming the clock divider due to uninitialized sysclk.
--

commit ed5bb252934aea5e2205d3b589d2556dcdbed81f
Author: Valerio Setti <vsetti@baylibre.com>
Subject: ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures

This commit transitions the AIU I2S encoder to use gx_iface and gx_stream
structures, moving clock handling and rate symmetry into the hw_params
and trigger callbacks.

> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 3b4061508c180..39accd396affb 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c
[ ... ]
> @@ -142,24 +144,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
>  }
>  
>  static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
> -				      struct snd_pcm_hw_params *params)
> +				      struct gx_stream *ts)
>  {
>  	struct aiu *aiu = snd_soc_component_get_drvdata(component);
> -	unsigned int srate = params_rate(params);
>  	unsigned int fs, bs;
>  	int ret;
>  
>  	/* Get the oversampling factor */
> -	fs = DIV_ROUND_CLOSEST(clk_get_rate(aiu->i2s.clks[MCLK].clk), srate);
> +	fs = DIV_ROUND_CLOSEST(ts->iface->mclk_rate, ts->iface->rate);
>  
>  	if (fs % 64)
>  		return -EINVAL;

If the machine driver does not call aiu_encoder_i2s_set_sysclk(),
iface->mclk_rate remains 0. This causes fs to evaluate to 0, which passes
the modulo check since 0 % 64 is 0.

When bs = fs / 64 (which is 0) is passed into aiu_encoder_i2s_set_more_div(),
does bs - 1 underflow to 0xffffffff?

If so, FIELD_PREP(AIU_CLK_CTRL_MORE_I2S_DIV, bs - 1) might mask the
underflowed value and write a corrupted state to the clock control register.

[ ... ]
> @@ -188,25 +184,55 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
>  				     struct snd_pcm_hw_params *params,
>  				     struct snd_soc_dai *dai)
>  {
> +	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> +	struct gx_iface *iface = ts->iface;
>  	struct snd_soc_component *component = dai->component;
>  	int ret;
>  
> -	/* Disable the clock while changing the settings */
> -	aiu_encoder_i2s_divider_enable(component, false);

The removal of aiu_encoder_i2s_divider_enable(component, false) means
this function now performs pipeline soft-resets (AIU_RST_SOFT_I2S_FAST)
and modifies clock dividers while the hardware might be actively running a
concurrent stream.

Can this corrupt the bit-clock or cause pipeline lockups on the active
stream?

> +	/* Enforce interface wide rate symmetry. */
> +	if (iface->rate && (iface->rate != params_rate(params))) {
> +		dev_err(dai->dev, "can't set iface rate (%d != %d)\n",
> +			iface->rate, params_rate(params));
> +		return -EINVAL;
> +	}
> +
> +	iface->rate = params_rate(params);
> +	ts->physical_width = params_physical_width(params);
> +	ts->width = params_width(params);
> +	ts->channels = params_channels(params);

ALSA permits concurrent hw_params ioctls on independent substreams, such
as playback and capture. Since the rate check and assignment are performed
without mutual exclusion, could two threads simultaneously see iface->rate
as 0 and overwrite each other's configuration?

Additionally, this logic enforces symmetry for the sample rate, but ignores
channel count and physical width.

If a playback stream is actively running at 16-bit 2-channel, and a capture
stream is concurrently configured to 16-bit 8-channel, it appears
aiu_encoder_i2s_set_more_div() will recalculate and overwrite the shared
hardware clock divider AIU_CLK_CTRL_MORE_I2S_DIV.

Could this silently corrupt the audio data of the actively playing stream?

[ ... ]
>  static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
>  				     struct snd_soc_dai *dai)
>  {
>  	struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
> +	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> +	struct gx_iface *iface = ts->iface;
> +
> +	if (!snd_soc_dai_active(dai))
> +		iface->rate = 0;

The ASoC core invokes the shutdown callback before decrementing the stream's
active count.

Because of this, does snd_soc_dai_active(dai) always evaluate to at least 1
during the shutdown of the final stream?

If so, iface->rate will never be reset to 0, which would cause subsequent
streams with different sample rates to permanently fail with -EINVAL until
the driver is reloaded.

[ ... ]
> +static int aiu_encoder_i2s_trigger(struct snd_pcm_substream *substream,
> +				   int cmd,
> +				   struct snd_soc_dai *dai)
> +{
> +	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> +	int ret;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = gx_stream_start(ts);
> +		break;
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +	case SNDRV_PCM_TRIGGER_STOP:
> +		gx_stream_stop(ts);
> +		ret = 0;
> +		break;

The PCM trigger callback typically executes in an atomic context with the
PCM stream spinlock held and hardware interrupts disabled, unless the DAI
is explicitly marked with nonatomic = 1.

Looking at gx_stream_start() and gx_stream_stop(), they both unconditionally
acquire a sleeping lock via mutex_lock(&ts->lock).

Does acquiring a mutex in this atomic context trigger a scheduling while
atomic kernel panic?

[ ... ]
> +static int aiu_encoder_i2s_remove_dai(struct snd_soc_dai *dai)
> +{
> +	int stream;
> +
> +	for_each_pcm_streams(stream) {
> +		struct gx_stream *ts = snd_soc_dai_dma_data_get(dai, stream);
> +
> +		if (ts)
> +			gx_stream_free(ts);
> +	}
> +
> +	return 0;
> +}
> +
> +static int aiu_encoder_i2s_probe_dai(struct snd_soc_dai *dai)
> +{
> +	struct aiu *aiu = snd_soc_dai_get_drvdata(dai);
> +	struct gx_iface *iface = &aiu->i2s.iface;
> +	int stream;
> +
> +	for_each_pcm_streams(stream) {
> +		struct gx_stream *ts;
> +
> +		if (!snd_soc_dai_get_widget(dai, stream))
> +			continue;
> +
> +		ts = gx_stream_alloc(iface);
> +		if (!ts) {
> +			aiu_encoder_i2s_remove_dai(dai);
> +			return -ENOMEM;
> +		}
> +		snd_soc_dai_dma_data_set(dai, stream, ts);
> +	}

If gx_stream_alloc() fails for the second stream in
aiu_encoder_i2s_probe_dai(), it calls aiu_encoder_i2s_remove_dai() to clean
up the already allocated streams.

Since aiu_encoder_i2s_remove_dai() retrieves the gx_stream and frees it, but
does not clear the pointer via snd_soc_dai_dma_data_set(dai, stream, NULL),
the snd_soc_dai structure will still contain a dangling pointer.

If a subsequent deferred probe retry fails allocation again, will
remove_dai() fetch the same dangling pointer and pass it to gx_stream_free()
a second time, resulting in a double-free memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic