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

[Valerio Setti <vsetti@baylibre.com>]

>> +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?
> 

The analysis is technically correct, but let me share my analysis first
and then gather some feedback.

The issue is not introduced by this series. gx_stream_start() and
gx_stream_stop() have been called from aiu_encoder_i2s_trigger() since
commit 9335117a221a ("ASoC: meson: aiu-encoder-i2s: prepare for
multiple streams"), which is already upstream. GX links are atomic, so
the playback path already takes the 'gx_stream->lock' mutex with the PCM
stream spinlock held. This patch only extends the same pattern to the
capture path.

Now back to the issue. Taking 'gx_stream->lock' mutex in trigger means
taking a sleeping lock in atomic context which, of course, cannot sleep.
For the sleep to happen it requires the mutex to be contended:
- trigger vs. trigger cannot contend, since dpcm_be_dai_trigger() holds
   the BE substream lock around the BE trigger, which serializes all
   calls on the same gx_stream (even with multiple AUDIN FIFOs sharing
   the BE);
- the formatter attach/detach from the DAPM widget events also takes
   the mutex. This normally happens in prepare/hw_free, when the stream
   is not running, but a mixer change (e.g. "FIFOx Input Source" moved
   away from I2S) powers the formatter down under the DAPM mutex only,
   while the BE is still running. A trigger on another CPU at that
   moment (userspace or XRUN) would find the mutex held.
   However since AUDIN FIFO only supports I2S for now, I don't see a real
   reason for this switch to happen in practice.

Now the solution. Moving the gx-card links to be nonatomic seems a bit too
much intrusive to me. I think the best solution is to change
'gx_stream->lock' to be a spinlock. This is fine for GX formatter because
all ops are pure MMIO regmap accesses, so they shouldn't sleep.

Finally the question. As I said, since the issue described by Sashiko
seems a bit hard to hit, I think the fix can be postponed to a quick
follow-up series. However if reviewers deem that it's absolutely mandatory
to fix the issue before this series lands, I will either prepare a small
pre-requisite series or add a new commit to this one (I would like to
know what's the reviewers preference on this case).

Thanks in advance for the replies! :)

-- 
Valerio


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