Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>, alsa-devel@alsa-project.org
Subject: Re: [alsa-devel] [PATCH 6/8] ALSA: pcm: Add the support for sync-stop operation
Date: Mon, 18 Nov 2019 10:33:51 -0600
Message-ID: <bef3322e-5c5e-cc88-05ae-c6d13edf8bb5@linux.intel.com> (raw)
In-Reply-To: <20191117085308.23915-7-tiwai@suse.de>



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@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;
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-17  8:53 [alsa-devel] [PATCH 0/8] ALSA: pcm: API cleanups and extensions Takashi Iwai
2019-11-17  8:53 ` [alsa-devel] [PATCH 1/8] ALSA: pcm: Introduce managed buffer allocation mode Takashi Iwai
2019-11-18 16:24   ` Pierre-Louis Bossart
2019-11-18 18:46     ` Takashi Iwai
2019-11-17  8:53 ` [alsa-devel] [PATCH 2/8] ALSA: docs: Update for " Takashi Iwai
2019-11-17  8:53 ` [alsa-devel] [PATCH 3/8] ALSA: pcm: Allow NULL ioctl ops Takashi Iwai
2019-11-17  8:53 ` [alsa-devel] [PATCH 4/8] ALSA: docs: Update document about the default PCM " Takashi Iwai
2019-11-17  8:53 ` [alsa-devel] [PATCH 5/8] ALSA: pcm: Move PCM_RUNTIME_CHECK() macro into local header Takashi Iwai
2019-11-17  9:42   ` kbuild test robot
2019-11-17 10:05     ` Takashi Iwai
2019-11-17 10:28   ` kbuild test robot
2019-11-17  8:53 ` [alsa-devel] [PATCH 6/8] ALSA: pcm: Add the support for sync-stop operation Takashi Iwai
2019-11-18 16:33   ` Pierre-Louis Bossart [this message]
2019-11-18 18:47     ` Takashi Iwai
2019-11-17  8:53 ` [alsa-devel] [PATCH 7/8] ALSA: pcm: Add card sync_irq field Takashi Iwai
2019-11-18 16:38   ` Pierre-Louis Bossart
2019-11-18 18:52     ` Takashi Iwai
2019-11-18 19:20       ` Sridharan, Ranjani
2019-11-18 19:49         ` Takashi Iwai
2019-11-18 19:55           ` Sridharan, Ranjani
2019-11-18 20:40             ` Takashi Iwai
2019-11-18 23:47               ` Ranjani Sridharan
2019-11-19  6:44                 ` Takashi Iwai
2019-11-19  7:40                   ` Ranjani Sridharan
2019-11-19  8:24                     ` Takashi Iwai
2019-11-19  9:39                       ` Takashi Iwai
2019-11-19 16:36                       ` Ranjani Sridharan
2019-11-19 21:27                         ` Takashi Iwai
2019-11-19 21:43                           ` Sridharan, Ranjani
2019-11-21 19:22                             ` Sridharan, Ranjani
2019-11-21 20:34                               ` Takashi Iwai
2019-11-21 20:46                                 ` Sridharan, Ranjani
2019-11-21 21:13                                   ` Takashi Iwai
2019-11-21 21:17                                     ` Sridharan, Ranjani
2019-11-21 21:28                                       ` Takashi Iwai
2019-11-21 21:45                                         ` Sridharan, Ranjani
2019-11-22  4:08                                     ` Jie, Yang
2019-11-17  8:53 ` [alsa-devel] [PATCH 8/8] ALSA: docs: Update about the new PCM sync_stop ops Takashi Iwai

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bef3322e-5c5e-cc88-05ae-c6d13edf8bb5@linux.intel.com \
    --to=pierre-louis.bossart@linux.intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git