| Message ID | 20191117085308.23915-7-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: pcm: API cleanups and extensions | expand |
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; >
On Mon, 18 Nov 2019 17:33:51 +0100, Pierre-Louis Bossart wrote: > > > > 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" The new PCM ops is exactly for resolving such a thing. Basically snd_pcm_period_elapsed() is handled asynchronously, so we need the synchronization. For a simpler driver, it's merely synchronize_irq() call (no matter whether it's a hard-irq or a threaded-irq), but for drivers that deal with another async mechanism (e.g. USB-audio), another sync procedure is required. It's possible that the workaround you've done in SOF can be simplified by this mechanism, but I didn't take a deeper look at it yet. thanks, Takashi
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;
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. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- include/sound/pcm.h | 2 ++ sound/core/pcm_native.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+)