Re: [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asymmetric `bs_quirk` validation allows a new stream to corrupt the BCLK divider of an already active non-quirk stream.
- [Medium] Improper state tracking using `snd_soc_dai_active(dai)` in `hw_free` leaks shared interface state, locking the DAI's parameters.
--

commit b82d5ce50c8fb1a7cfc960367388f0f096fbaa05
Author: Valerio Setti <[email protected]>

ASoC: meson: aiu-encoder-i2s: prepare for multiple streams

This commit prepares the aiu-encoder-i2s interface to handle both playback
and capture streams. It introduces per-stream data allocation, enforces
interface-wide rate symmetry, and modifies clock handling to enable
internal clocks on startup while deferring MCLK and I2S clock dividers to
prepare/hw_free.

> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 3b4061508c180..f50b03824ad28 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c

[ ... ]

> @@ -126,6 +131,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
>  			return -EINVAL;
>  		}
>  		bs += bs / 2;
> +		iface->bs_quirk = true;
> +	} else {

[Severity: High]
Does this branch allow a new stream to corrupt the clock divider of an
already active non-quirk stream?

The else branch correctly prevents a new non-quirk stream from configuring
if iface->bs_quirk is already true. However, the if branch seems to miss
the reverse check.

If a normal stream (like 2-channel audio) is already active, iface->bs_quirk
remains false. If a second stream is then configured with 16-bit 8-channels,
it appears this code will calculate the 50% increase, set
iface->bs_quirk = true, and unconditionally overwrite the shared register,
which might corrupt the bitclock frequency of the already running first
stream.

> +		/*
> +		 * If the bs quirk is currently applied for one stream and another
> +		 * ones tries to setup a configuration for which the quirk is
> +		 * not required, then fail.
> +		 */
> +		if (iface->bs_quirk) {
> +			dev_err(component->dev,
> +				"bclk requirements are incompatible with active stream\n");
> +			return -EINVAL;
> +		}
>  	}

[ ... ]

> @@ -188,24 +206,59 @@ 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);
> +	/*
> +	 * Enforce interface wide rate symmetry only if there is more than
> +	 * 1 stream active.
> +	 */
> +	if (snd_soc_dai_active(dai) > 1) {
> +		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;
> +		}
> +	}

[ ... ]

> @@ -214,9 +267,24 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
>  static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
>  				   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;
>  
> -	aiu_encoder_i2s_divider_enable(component, false);
> +	/*
> +	 * If this is the last substream being closed then disable the i2s
> +	 * clock divider and clear 'iface->rate'.
> +	 */
> +	if (snd_soc_dai_active(dai) <= 1) {
> +		aiu_encoder_i2s_divider_enable(component, 0);
> +		iface->rate = 0;
> +		iface->bs_quirk = false;
> +	}

[Severity: Medium]
Can this leak the shared interface state and lock the DAI's parameters?

snd_soc_dai_active() tracks the number of open file descriptors, not the
number of streams with active hardware parameters.

If an application opens both playback and capture (making active == 2),
configures the playback stream, and then frees the hardware parameters via
SNDRV_PCM_IOCTL_HW_FREE without closing the file descriptors, this hw_free
callback will see active == 2.

Could this cause iface->rate to be leaked into a zombie state, preventing
valid format reconfigurations without fully closing the devices, and causing
subsequent hw_params attempts to fail the symmetry check above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic