[PATCH 0/4] ALSA: pcm: sync_stop fixes
Hi,
here is a patch set to cover the missing handling of the recently introduced sync_stop PCM ops at suspend and disconnect states. The last patch is a pure cleanup.
Takashi
===
Takashi Iwai (4): ALSA: pcm: Call sync_stop at disconnection ALSA: pcm: Assure sync with the pending stop operation at suspend ALSA: pcm: Don't call sync_stop if it hasn't been stopped ALSA: pcm: Use for_each_pcm_substream() macro
sound/core/init.c | 4 +++ sound/core/pcm.c | 27 +++++++++++---------- sound/core/pcm_local.h | 7 ++++++ sound/core/pcm_memory.c | 12 +++------ sound/core/pcm_native.c | 54 +++++++++++++++++++++-------------------- 5 files changed, 57 insertions(+), 47 deletions(-)
The PCM core should perform the sync for the pending stop operations at disconnection. Otherwise it may lead to unexpected access.
Currently the old user of sync_stop, USB-audio driver, has its own sync, so this isn't needed, but it's better to guarantee the sync in the PCM core level.
This patch adds the missing sync_stop call at PCM disconnection callback. It also assures the IRQ sync if it's specified in the card. snd_pcm_sync_stop() is slightly modified to be called also for any PCM substream object now.
Fixes: 1e850beea278 ("ALSA: pcm: Add the support for sync-stop operation") Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/init.c | 4 ++++ sound/core/pcm.c | 4 ++++ sound/core/pcm_local.h | 1 + sound/core/pcm_native.c | 16 ++++++++-------- 4 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/sound/core/init.c b/sound/core/init.c index 84b573e9c1f9..45f4b01de23f 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -15,6 +15,7 @@ #include <linux/pm.h> #include <linux/debugfs.h> #include <linux/completion.h> +#include <linux/interrupt.h>
#include <sound/core.h> #include <sound/control.h> @@ -426,6 +427,9 @@ int snd_card_disconnect(struct snd_card *card) /* notify all devices that we are disconnected */ snd_device_disconnect_all(card);
+ if (card->sync_irq > 0) + synchronize_irq(card->sync_irq); + snd_info_card_disconnect(card); if (card->registered) { device_del(&card->card_dev); diff --git a/sound/core/pcm.c b/sound/core/pcm.c index e5947281e5fc..50eb29fcdfe7 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -1111,6 +1111,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) } }
+ for (cidx = 0; cidx < 2; cidx++) + for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) + snd_pcm_sync_stop(substream, false); + pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { snd_unregister_device(&pcm->streams[cidx].dev); diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 17a1a5d87098..b3e8be5aeafb 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -63,6 +63,7 @@ static inline void snd_pcm_timer_done(struct snd_pcm_substream *substream) {}
void __snd_pcm_xrun(struct snd_pcm_substream *substream); void snd_pcm_group_init(struct snd_pcm_group *group); +void snd_pcm_sync_stop(struct snd_pcm_substream *substream, bool sync_irq);
#ifdef CONFIG_SND_DMA_SGBUF struct page *snd_pcm_sgbuf_ops_page(struct snd_pcm_substream *substream, diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 893c2abb2f63..1f5acf08cdcd 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -583,13 +583,13 @@ static inline void snd_pcm_timer_notify(struct snd_pcm_substream *substream, #endif }
-static void snd_pcm_sync_stop(struct snd_pcm_substream *substream) +void snd_pcm_sync_stop(struct snd_pcm_substream *substream, bool sync_irq) { - if (substream->runtime->stop_operating) { + if (substream->runtime && substream->runtime->stop_operating) { substream->runtime->stop_operating = false; - if (substream->ops->sync_stop) + if (substream->ops && substream->ops->sync_stop) substream->ops->sync_stop(substream); - else if (substream->pcm->card->sync_irq > 0) + else if (sync_irq && substream->pcm->card->sync_irq > 0) synchronize_irq(substream->pcm->card->sync_irq); } } @@ -686,7 +686,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, if (atomic_read(&substream->mmap_count)) return -EBADFD;
- snd_pcm_sync_stop(substream); + snd_pcm_sync_stop(substream, true);
params->rmask = ~0U; err = snd_pcm_hw_refine(substream, params); @@ -809,7 +809,7 @@ static int do_hw_free(struct snd_pcm_substream *substream) { int result = 0;
- snd_pcm_sync_stop(substream); + snd_pcm_sync_stop(substream, true); if (substream->ops->hw_free) result = substream->ops->hw_free(substream); if (substream->managed_buffer_alloc) @@ -1736,7 +1736,7 @@ static void snd_pcm_post_resume(struct snd_pcm_substream *substream, 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); + snd_pcm_sync_stop(substream, true); }
static const struct action_ops snd_pcm_action_resume = { @@ -1866,7 +1866,7 @@ static int snd_pcm_do_prepare(struct snd_pcm_substream *substream, snd_pcm_state_t state) { int err; - snd_pcm_sync_stop(substream); + snd_pcm_sync_stop(substream, true); err = substream->ops->prepare(substream); if (err < 0) return err;
The current PCM code calls the sync_stop at the resume action due to the analogy to the PCM prepare call pattern. But, it makes little sense, as the sync should have been done rather at the suspend time, not at the resume time.
This patch corrects the sync_stop call at suspend/resume to assure the sync before finishing the suspend.
Fixes: 1e850beea278 ("ALSA: pcm: Add the support for sync-stop operation") Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 1f5acf08cdcd..cbbdb75c66ea 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1615,6 +1615,7 @@ static int snd_pcm_do_suspend(struct snd_pcm_substream *substream, if (! snd_pcm_running(substream)) return 0; substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND); + runtime->stop_operating = true; return 0; /* suspend unconditionally */ }
@@ -1691,6 +1692,12 @@ int snd_pcm_suspend_all(struct snd_pcm *pcm) return err; } } + + for (stream = 0; stream < 2; stream++) + for (substream = pcm->streams[stream].substream; + substream; substream = substream->next) + snd_pcm_sync_stop(substream, false); + return 0; } EXPORT_SYMBOL(snd_pcm_suspend_all); @@ -1736,7 +1743,6 @@ static void snd_pcm_post_resume(struct snd_pcm_substream *substream, 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, true); }
static const struct action_ops snd_pcm_action_resume = {
The PCM stop operation sets the stop_operating flag for indicating the sync_stop post-process. This flag is, however, set unconditionally even if the PCM trigger weren't issued. This may lead to inconsistency in the driver side.
Correct the code to set stop_operating flag only after the trigger STOP is actually called.
Fixes: 1e850beea278 ("ALSA: pcm: Add the support for sync-stop operation") Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index cbbdb75c66ea..0ae2475465ab 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1421,8 +1421,10 @@ static int snd_pcm_do_stop(struct snd_pcm_substream *substream, snd_pcm_state_t state) { if (substream->runtime->trigger_master == substream && - snd_pcm_running(substream)) + snd_pcm_running(substream)) { substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP); + substream->runtime->stop_operating = true; + } return 0; /* unconditonally stop all substreams */ }
@@ -1435,7 +1437,6 @@ static void snd_pcm_post_stop(struct snd_pcm_substream *substream, 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); }
There are a few places doing the same loop iterating all PCM substreams belonging to the PCM object. Introduce a local helper macro, for_each_pcm_substream(), to simplify the code.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm.c | 29 +++++++++++++---------------- sound/core/pcm_local.h | 6 ++++++ sound/core/pcm_memory.c | 12 ++++-------- sound/core/pcm_native.c | 35 +++++++++++++++-------------------- 4 files changed, 38 insertions(+), 44 deletions(-)
diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 50eb29fcdfe7..b163164a83ec 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -1095,25 +1095,22 @@ static int snd_pcm_dev_disconnect(struct snd_device *device) mutex_lock(&pcm->open_mutex); wake_up(&pcm->open_wait); list_del_init(&pcm->list); - for (cidx = 0; cidx < 2; cidx++) { - for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) { - snd_pcm_stream_lock_irq(substream); - if (substream->runtime) { - if (snd_pcm_running(substream)) - snd_pcm_stop(substream, - SNDRV_PCM_STATE_DISCONNECTED); - /* to be sure, set the state unconditionally */ - substream->runtime->status->state = SNDRV_PCM_STATE_DISCONNECTED; - wake_up(&substream->runtime->sleep); - wake_up(&substream->runtime->tsleep); - } - snd_pcm_stream_unlock_irq(substream); + + for_each_pcm_substream(pcm, cidx, substream) { + snd_pcm_stream_lock_irq(substream); + if (substream->runtime) { + if (snd_pcm_running(substream)) + snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); + /* to be sure, set the state unconditionally */ + substream->runtime->status->state = SNDRV_PCM_STATE_DISCONNECTED; + wake_up(&substream->runtime->sleep); + wake_up(&substream->runtime->tsleep); } + snd_pcm_stream_unlock_irq(substream); }
- for (cidx = 0; cidx < 2; cidx++) - for (substream = pcm->streams[cidx].substream; substream; substream = substream->next) - snd_pcm_sync_stop(substream, false); + for_each_pcm_substream(pcm, cidx, substream) + snd_pcm_sync_stop(substream, false);
pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index b3e8be5aeafb..e3b3558aeab6 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -72,4 +72,10 @@ struct page *snd_pcm_sgbuf_ops_page(struct snd_pcm_substream *substream,
#define PCM_RUNTIME_CHECK(sub) snd_BUG_ON(!(sub) || !(sub)->runtime)
+/* loop over all PCM substreams */ +#define for_each_pcm_substream(pcm, str, subs) \ + for ((str) = 0; (str) < 2; (str)++) \ + for ((subs) = (pcm)->streams[str].substream; (subs); \ + (subs) = (subs)->next) + #endif /* __SOUND_CORE_PCM_LOCAL_H */ diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c index ee6e9c5eec45..289dd1fd8fe7 100644 --- a/sound/core/pcm_memory.c +++ b/sound/core/pcm_memory.c @@ -111,9 +111,8 @@ void snd_pcm_lib_preallocate_free_for_all(struct snd_pcm *pcm) struct snd_pcm_substream *substream; int stream;
- for (stream = 0; stream < 2; stream++) - for (substream = pcm->streams[stream].substream; substream; substream = substream->next) - snd_pcm_lib_preallocate_free(substream); + for_each_pcm_substream(pcm, stream, substream) + snd_pcm_lib_preallocate_free(substream); } EXPORT_SYMBOL(snd_pcm_lib_preallocate_free_for_all);
@@ -246,11 +245,8 @@ static void preallocate_pages_for_all(struct snd_pcm *pcm, int type, struct snd_pcm_substream *substream; int stream;
- for (stream = 0; stream < 2; stream++) - for (substream = pcm->streams[stream].substream; substream; - substream = substream->next) - preallocate_pages(substream, type, data, size, max, - managed); + for_each_pcm_substream(pcm, stream, substream) + preallocate_pages(substream, type, data, size, max, managed); }
/** diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 0ae2475465ab..17a85f4815d5 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1674,30 +1674,25 @@ int snd_pcm_suspend_all(struct snd_pcm *pcm) if (! pcm) return 0;
- for (stream = 0; stream < 2; stream++) { - for (substream = pcm->streams[stream].substream; - substream; substream = substream->next) { - /* FIXME: the open/close code should lock this as well */ - if (substream->runtime == NULL) - continue; + for_each_pcm_substream(pcm, stream, substream) { + /* FIXME: the open/close code should lock this as well */ + if (!substream->runtime) + continue;
- /* - * Skip BE dai link PCM's that are internal and may - * not have their substream ops set. - */ - if (!substream->ops) - continue; + /* + * Skip BE dai link PCM's that are internal and may + * not have their substream ops set. + */ + if (!substream->ops) + continue;
- err = snd_pcm_suspend(substream); - if (err < 0 && err != -EBUSY) - return err; - } + err = snd_pcm_suspend(substream); + if (err < 0 && err != -EBUSY) + return err; }
- for (stream = 0; stream < 2; stream++) - for (substream = pcm->streams[stream].substream; - substream; substream = substream->next) - snd_pcm_sync_stop(substream, false); + for_each_pcm_substream(pcm, stream, substream) + snd_pcm_sync_stop(substream, false);
return 0; }
participants (1)
-
Takashi Iwai