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 [thread overview]
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
next prev parent reply other threads:[~2019-11-18 18:21 UTC|newest]
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 publicly 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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).