Re: [PATCH v5 4/7] ASoC: meson: add AUDIN driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ASoC: meson: audin: sleeping mutex_lock() in atomic trigger and softirq timer context
--

commit f3bd154de23b0a57bb8257b40dc479a985fa6c9a
Author: Valerio Setti <vsetti@baylibre.com>

ASoC: meson: add AUDIN driver

This commit adds the AUDIN audio input driver for Amlogic GX platforms
(GXBB/GXL), including FIFO capture DAIs and an I2S data formatter widget.

> diff --git a/sound/soc/meson/audin-fifo.c b/sound/soc/meson/audin-fifo.c
> new file mode 100644
> index 0000000000000..5b254c90c7100
> --- /dev/null
> +++ b/sound/soc/meson/audin-fifo.c
[ ... ]
> +static enum hrtimer_restart audin_fifo_timer_cb(struct hrtimer *timer)
> +{
> +	struct audin_fifo_dai_data *data =
> +		container_of(timer, struct audin_fifo_dai_data, polling_timer);
> +	struct snd_pcm_substream *substream = data->substream;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	unsigned int curr_pos;
> +	unsigned int delta, elapsed_periods_bytes, extra_bytes, sleep_bytes;
> +
> +	/*
> +	 * Take the stream lock to prevent userspace from issuing a quick
> +	 * PAUSE_PUSH -> PAUSE_RELEASE transition while we're running this
> +	 * callback. Doing so would cause "hrtimer_forward_now()" to be called
> +	 * (see below) while the timer is already enqueued.
> +	 */
> +	guard(pcm_stream_lock_irqsave)(substream);
> +
> +	/* Stopped, or a trigger already re-armed the timer while we waited. */
> +	if (!READ_ONCE(data->running) || hrtimer_is_queued(timer))
> +		return HRTIMER_NORESTART;
> +
> +	curr_pos = audin_fifo_pos(data->dai, runtime);
> +	delta = (curr_pos >= data->last_pos) ?
> +		curr_pos - data->last_pos :
> +		(runtime->dma_bytes - data->last_pos) + curr_pos;
> +	extra_bytes = delta % data->period_bytes;
> +	elapsed_periods_bytes = delta - extra_bytes;
> +
> +	/* Report only when the period is completed. */
> +	if (delta >= data->period_bytes) {
> +		data->last_pos = data->last_pos + elapsed_periods_bytes;
> +		if (data->last_pos >= runtime->dma_bytes)
> +			data->last_pos %= runtime->dma_bytes;
> +		snd_pcm_period_elapsed_under_stream_lock(substream);
> +	}
[ ... ]
> diff --git a/sound/soc/meson/audin-formatter-i2s.c b/sound/soc/meson/audin-formatter-i2s.c
> new file mode 100644
> index 0000000000000..8bbb5cdb52dc8
> --- /dev/null
> +++ b/sound/soc/meson/audin-formatter-i2s.c
[ ... ]
> +static struct gx_stream *
> +audin_formatter_i2s_get_stream(struct snd_soc_dapm_widget *w)
> +{
> +	struct snd_soc_dai *be = audin_formatter_i2s_get_be(w);
> +
> +	if (!be)
> +		return NULL;
> +
> +	return snd_soc_dai_dma_data_get_capture(be);
> +}

[Severity: High]
Does binding the AUDIN formatter to gx_stream lead to sleeping in atomic and
softirq contexts during PCM trigger operations?

In audin_fifo_dai_probe(), polling_timer is set up with HRTIMER_MODE_REL_SOFT,
so audin_fifo_timer_cb() runs in softirq context and acquires
guard(pcm_stream_lock_irqsave)(substream). This requires substream->pcm->nonatomic
to be false, otherwise _snd_pcm_stream_lock_irqsave() would attempt to acquire
substream->self_group.mutex from softirq context.

However, with nonatomic set to false on the FE and BE substreams, PCM trigger
operations hold spin_lock_irqsave on the PCM stream locks (for example via
dpcm_be_dai_trigger()). This also occurs in softirq context when an XRUN is
detected:

audin_fifo_timer_cb()
  -> snd_pcm_period_elapsed_under_stream_lock()
    -> snd_pcm_update_hw_ptr0()
      -> snd_pcm_stop()
        -> dpcm_fe_dai_trigger()
          -> dpcm_be_dai_trigger()
            -> aiu_encoder_i2s_trigger()
              -> gx_stream_stop()

Because audin_formatter_i2s_get_stream() attaches the AUDIN formatter to the
backend DAI's struct gx_stream, starting or stopping the capture stream calls
aiu_encoder_i2s_trigger() -> gx_stream_start() or gx_stream_stop(), both of
which acquire the sleeping mutex ts->lock:

sound/soc/meson/gx-formatter.c:gx_stream_start() {
	mutex_lock(&ts->lock);

	/* Start all the formatters attached to the stream */
	list_for_each_entry(formatter, &ts->formatter_list, list) {
		ret = gx_formatter_enable(formatter);
...
}

sound/soc/meson/gx-formatter.c:gx_stream_stop() {
	mutex_lock(&ts->lock);
	ts->ready = false;

	/* Stop all the formatters attached to the stream */
	list_for_each_entry(formatter, &ts->formatter_list, list) {
		gx_formatter_disable(formatter);
	}

	mutex_unlock(&ts->lock);
}

Won't calling mutex_lock(&ts->lock) while holding the PCM stream spinlocks with
interrupts disabled (and from softirq context on XRUN) trigger a sleeping in
atomic context bug?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018-audin-v5-0-4b8cc13a1b22@baylibre.com?part=4

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