Re: [PATCH 1/3] ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The bs-quirk incompatibility check evaluates a non-existent capture stream and is unconditionally bypassed.

Pre-existing issues:
- [High] The AIU encoder I2S trigger callback unconditionally acquires a mutex while holding an IRQ-disabled spinlock.
--

--- Patch [1]: [PATCH 1/3] ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check ---
commit dc420735065ef5215776c2cbf7d64821269c3f60
Author: Valerio Setti <[email protected]>

ASoC: meson: aiu-encoder-i2s: fix bs quirk incompatibility check

This patch modifies the bs-quirk incompatibility check to evaluate the quirk
requirement of incoming parameters against the committed configuration of the
opposite stream at hw_params() time. It removes the interface-wide flag and
clears committed channels/width in hw_free().

> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 83b579e98f1c7..c2a280bfdfe2e 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c
[ ... ]
> @@ -62,13 +62,36 @@ static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component,
[ ... ]
> +static int aiu_encoder_check_bs_quirk(struct snd_pcm_substream *substream,
> +				      struct snd_pcm_hw_params *params,
> +				      struct snd_soc_dai *dai)
> +{
> +	struct gx_stream *other_stream = snd_soc_dai_dma_data_get(dai, !substream->stream);
> +
> +	/* Nothing to do if the other stream doesn't exist or it's not configured yet. */
> +	if (!other_stream || !other_stream->channels)
> +		return 0;

[Severity: High]
Will other_stream always be NULL here?

Looking at aiu_cpu_dai_drv in sound/soc/meson/aiu.c, CPU_I2S_ENCODER is
defined as a playback-only DAI:

[CPU_I2S_ENCODER] = {
    .name = "I2S Encoder",
    .playback = {
    ...

Since there is no capture stream, won't !substream->stream always resolve to a
non-existent stream?

If so, other_stream will be unconditionally NULL, which would cause this
function to immediately return 0 and bypass the incompatibility check entirely.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the trigger callback acquire a mutex
while in atomic context?

The ALSA .trigger operation calls aiu_encoder_i2s_trigger(), which invokes
gx_stream_start(). This then acquires a mutex:

aiu_encoder_i2s_trigger()
    gx_stream_start(ts)
        mutex_lock(&ts->lock);

Since the Meson GX machine driver does not set nonatomic = true on its DAI
links, the PCM stream lock is an IRQ-disabled spinlock when .trigger is
invoked.

Would this result in a scheduling while atomic bug when the sleepable mutex is
acquired while holding the IRQ-disabled spinlock?

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

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