[alsa-devel] [PATCH 6/8] ALSA: pcm: Add the support for sync-stop operation

[Pierre-Louis Bossart]

On 11/17/19 2:53 AM, Takashi Iwai wrote:
> The standard programming model of a PCM sound driver is to process
> snd_pcm_period_elapsed() from an interrupt handler.  When a running
> stream is stopped, PCM core calls the trigger-STOP PCM ops, sets the
> stream state to SETUP, and moves on to the next step.  This is
> performed in an atomic manner -- this could be called from the interrupt
> context, after all.
> 
> The problem is that, if the stream goes further and reaches to the
> CLOSE state immediately, the stream might be still being processed in
> snd_pcm_period_elapsed() in the interrupt context, and hits a NULL
> dereference.  Such a crash happens because of the atomic operation,
> and we can't wait until the stream-stop finishes.
> 
> For addressing such a problem, this commit adds a new PCM ops,
> sync_stop.  This gets called at the appropriate places that need a
> sync with the stream-stop, i.e. at hw_params, prepare and hw_free.
> 
> Some drivers already have a similar mechanism implemented locally, and
> we'll refactor the code later.

This rings a bell, for the SOF driver Keyon added a workqueue to avoid 
doing the call to snd_pcm_period_elapsed() while handling IPC 
interrupts. I believe the reason what that the IPC needs to be 
serialized, and the call to snd_pcm_period_elapsed could initiate a 
trigger stop IPC while we were dealing with an IPC, which broke the 
notion of serialization.

See "ASoC: SOF: PCM: add period_elapsed work to fix race condition in 
interrupt context"
and "ASoC: SOF: ipc: use snd_sof_pcm_period_elapsed"

> 
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>   include/sound/pcm.h     |  2 ++
>   sound/core/pcm_native.c | 15 +++++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 25563317782c..8a89fa6fdd5e 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -59,6 +59,7 @@ struct snd_pcm_ops {
>   	int (*hw_free)(struct snd_pcm_substream *substream);
>   	int (*prepare)(struct snd_pcm_substream *substream);
>   	int (*trigger)(struct snd_pcm_substream *substream, int cmd);
> +	int (*sync_stop)(struct snd_pcm_substream *substream);
>   	snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
>   	int (*get_time_info)(struct snd_pcm_substream *substream,
>   			struct timespec *system_ts, struct timespec *audio_ts,
> @@ -395,6 +396,7 @@ struct snd_pcm_runtime {
>   	wait_queue_head_t sleep;	/* poll sleep */
>   	wait_queue_head_t tsleep;	/* transfer sleep */
>   	struct fasync_struct *fasync;
> +	bool stop_operating;		/* sync_stop will be called */
>   
>   	/* -- private section -- */
>   	void *private_data;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 704ea75199e4..163d621ff238 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -568,6 +568,15 @@ static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream,
>   #endif
>   }
>   
> +static void snd_pcm_sync_stop(struct snd_pcm_substream *substream)
> +{
> +	if (substream->runtime->stop_operating) {
> +		substream->runtime->stop_operating = false;
> +		if (substream->ops->sync_stop)
> +			substream->ops->sync_stop(substream);
> +	}
> +}
> +
>   /**
>    * snd_pcm_hw_param_choose - choose a configuration defined by @params
>    * @pcm: PCM instance
> @@ -660,6 +669,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>   		if (atomic_read(&substream->mmap_count))
>   			return -EBADFD;
>   
> +	snd_pcm_sync_stop(substream);
> +
>   	params->rmask = ~0U;
>   	err = snd_pcm_hw_refine(substream, params);
>   	if (err < 0)
> @@ -788,6 +799,7 @@ static int snd_pcm_hw_free(struct snd_pcm_substream *substream)
>   	snd_pcm_stream_unlock_irq(substream);
>   	if (atomic_read(&substream->mmap_count))
>   		return -EBADFD;
> +	snd_pcm_sync_stop(substream);
>   	if (substream->ops->hw_free)
>   		result = substream->ops->hw_free(substream);
>   	if (substream->managed_buffer_alloc)
> @@ -1313,6 +1325,7 @@ static void snd_pcm_post_stop(struct snd_pcm_substream *substream, int state)
>   		runtime->status->state = state;
>   		snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTOP);
>   	}
> +	runtime->stop_operating = true;
>   	wake_up(&runtime->sleep);
>   	wake_up(&runtime->tsleep);
>   }
> @@ -1589,6 +1602,7 @@ static void snd_pcm_post_resume(struct snd_pcm_substream *substream, int state)
>   	snd_pcm_trigger_tstamp(substream);
>   	runtime->status->state = runtime->status->suspended_state;
>   	snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MRESUME);
> +	snd_pcm_sync_stop(substream);
>   }
>   
>   static const struct action_ops snd_pcm_action_resume = {
> @@ -1709,6 +1723,7 @@ static int snd_pcm_pre_prepare(struct snd_pcm_substream *substream,
>   static int snd_pcm_do_prepare(struct snd_pcm_substream *substream, int state)
>   {
>   	int err;
> +	snd_pcm_sync_stop(substream);
>   	err = substream->ops->prepare(substream);
>   	if (err < 0)
>   		return err;
>