[alsa-devel] [PATCH 0/6] Misc PCM fixes
Hi,
this is a kind of gleaning work, a collection of small cleanups of ALSA PCM implementations.
Takashi
===
Takashi Iwai (6): ALSA: pcm: Clean up SNDRV_PCM_IOCTL_PAUSE code ALSA: pcm: Apply power lock globally to common ioctls ALSA: pcm: Allow dropping stream directly after resume ALSA: pcm: Preprocess PAUSED or SUSPENDED stream before PREPARE ALSA: pcm: Use common PCM_RUNTIME_CHECK() for sanity checks ALSA: pcm: Skip ack callback without actual appl_ptr update
sound/core/pcm_lib.c | 3 ++ sound/core/pcm_native.c | 84 +++++++++++++++++++++---------------------------- 2 files changed, 39 insertions(+), 48 deletions(-)
Use snd_pcm_action_lock_irq() helper instead of open coding. No functional change.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 07995e645327..798bca967c0e 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2865,13 +2865,9 @@ static int snd_pcm_common_ioctl1(struct file *file, case SNDRV_PCM_IOCTL_DROP: return snd_pcm_drop(substream); case SNDRV_PCM_IOCTL_PAUSE: - { - int res; - snd_pcm_stream_lock_irq(substream); - res = snd_pcm_pause(substream, (int)(unsigned long)arg); - snd_pcm_stream_unlock_irq(substream); - return res; - } + return snd_pcm_action_lock_irq(&snd_pcm_action_pause, + substream, + (int)(unsigned long)arg); } pcm_dbg(substream->pcm, "unknown ioctl = 0x%x\n", cmd); return -ENOTTY;
All PCM common ioctls should run only in the powered up state, but currently only a few ioctls do the proper snd_power_lock() and snd_power_wait() invocations. Instead of adding to each place, do it commonly in the caller side, so that all these ioctls are assured to be operated at the power up state.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 56 +++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 37 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 798bca967c0e..bd1b74aa2068 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1540,14 +1540,7 @@ static const struct action_ops snd_pcm_action_resume = {
static int snd_pcm_resume(struct snd_pcm_substream *substream) { - struct snd_card *card = substream->pcm->card; - int res; - - snd_power_lock(card); - if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0)) >= 0) - res = snd_pcm_action_lock_irq(&snd_pcm_action_resume, substream, 0); - snd_power_unlock(card); - return res; + return snd_pcm_action_lock_irq(&snd_pcm_action_resume, substream, 0); }
#else @@ -1566,17 +1559,9 @@ static int snd_pcm_resume(struct snd_pcm_substream *substream) */ static int snd_pcm_xrun(struct snd_pcm_substream *substream) { - struct snd_card *card = substream->pcm->card; struct snd_pcm_runtime *runtime = substream->runtime; int result;
- snd_power_lock(card); - if (runtime->status->state == SNDRV_PCM_STATE_SUSPENDED) { - result = snd_power_wait(card, SNDRV_CTL_POWER_D0); - if (result < 0) - goto _unlock; - } - snd_pcm_stream_lock_irq(substream); switch (runtime->status->state) { case SNDRV_PCM_STATE_XRUN: @@ -1589,8 +1574,6 @@ static int snd_pcm_xrun(struct snd_pcm_substream *substream) result = -EBADFD; } snd_pcm_stream_unlock_irq(substream); - _unlock: - snd_power_unlock(card); return result; }
@@ -1694,8 +1677,6 @@ static const struct action_ops snd_pcm_action_prepare = { static int snd_pcm_prepare(struct snd_pcm_substream *substream, struct file *file) { - int res; - struct snd_card *card = substream->pcm->card; int f_flags;
if (file) @@ -1703,12 +1684,8 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream, else f_flags = substream->f_flags;
- snd_power_lock(card); - if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0)) >= 0) - res = snd_pcm_action_nonatomic(&snd_pcm_action_prepare, - substream, f_flags); - snd_power_unlock(card); - return res; + return snd_pcm_action_nonatomic(&snd_pcm_action_prepare, + substream, f_flags); }
/* @@ -1805,15 +1782,6 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream, if (runtime->status->state == SNDRV_PCM_STATE_OPEN) return -EBADFD;
- snd_power_lock(card); - if (runtime->status->state == SNDRV_PCM_STATE_SUSPENDED) { - result = snd_power_wait(card, SNDRV_CTL_POWER_D0); - if (result < 0) { - snd_power_unlock(card); - return result; - } - } - if (file) { if (file->f_flags & O_NONBLOCK) nonblock = 1; @@ -1896,7 +1864,6 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream, unlock: snd_pcm_stream_unlock_irq(substream); up_read(&snd_pcm_link_rwsem); - snd_power_unlock(card);
return result; } @@ -2798,7 +2765,7 @@ static int snd_pcm_tstamp(struct snd_pcm_substream *substream, int __user *_arg) return 0; } -static int snd_pcm_common_ioctl1(struct file *file, +static int snd_pcm_common_ioctl(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg) { @@ -2873,6 +2840,21 @@ static int snd_pcm_common_ioctl1(struct file *file, return -ENOTTY; }
+static int snd_pcm_common_ioctl1(struct file *file, + struct snd_pcm_substream *substream, + unsigned int cmd, void __user *arg) +{ + struct snd_card *card = substream->pcm->card; + int res; + + snd_power_lock(card); + res = snd_power_wait(card, SNDRV_CTL_POWER_D0); + if (res >= 0) + res = snd_pcm_common_ioctl(file, substream, cmd, arg); + snd_power_unlock(card); + return res; +} + static int snd_pcm_playback_ioctl1(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg)
So far, the PCM core refuses DROP ioctl when the stream in the suspended state. This was basically to avoid the invalid state change *during* the suspend. But since we protect the power change globally in the common PCM ioctl caller side, it's guaranteed that snd_pcm_drop() is called at the right power state. So we can assume that the drop of stream is safe immediately after SUSPENDED state.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index bd1b74aa2068..69cf9b02ac70 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1883,8 +1883,7 @@ static int snd_pcm_drop(struct snd_pcm_substream *substream) runtime = substream->runtime;
if (runtime->status->state == SNDRV_PCM_STATE_OPEN || - runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED || - runtime->status->state == SNDRV_PCM_STATE_SUSPENDED) + runtime->status->state == SNDRV_PCM_STATE_DISCONNECTED) return -EBADFD;
snd_pcm_stream_lock_irq(substream);
Calling PREPARE ioctl to the stream in either PAUSED or SUSPENDED state may confuse some drivers that don't handle the state properly. Instead of fixing each driver, PCM core should take care of the proper state change before actually trying to (re-)prepare the stream. Namely, when the stream is in PAUSED state, it triggers PAUSH_RELEASE, and when in SUSPENDED state, it triggers STOP, before calling prepare callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 69cf9b02ac70..0941b9c92b3f 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1684,6 +1684,17 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream, else f_flags = substream->f_flags;
+ snd_pcm_stream_lock_irq(substream); + switch (substream->runtime->status->state) { + case SNDRV_PCM_STATE_PAUSED: + snd_pcm_pause(substream, 0); + /* fallthru */ + case SNDRV_PCM_STATE_SUSPENDED: + snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP); + break; + } + snd_pcm_stream_unlock_irq(substream); + return snd_pcm_action_nonatomic(&snd_pcm_action_prepare, substream, f_flags); }
On Jun 13 2017 23:18, Takashi Iwai wrote:
Calling PREPARE ioctl to the stream in either PAUSED or SUSPENDED state may confuse some drivers that don't handle the state properly. Instead of fixing each driver, PCM core should take care of the proper state change before actually trying to (re-)prepare the stream. Namely, when the stream is in PAUSED state, it triggers PAUSH_RELEASE,
s/PAUSH_RELEASE/PAUSE_RELEASE/
and when in SUSPENDED state, it triggers STOP, before calling prepare callbacks.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_native.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 69cf9b02ac70..0941b9c92b3f 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1684,6 +1684,17 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream, else f_flags = substream->f_flags;
- snd_pcm_stream_lock_irq(substream);
- switch (substream->runtime->status->state) {
- case SNDRV_PCM_STATE_PAUSED:
snd_pcm_pause(substream, 0);/* fallthru */- case SNDRV_PCM_STATE_SUSPENDED:
snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);break;- }
- snd_pcm_stream_unlock_irq(substream);
- return snd_pcm_action_nonatomic(&snd_pcm_action_prepare, substream, f_flags); }
Regards
Takashi Sakamoto
Just a code cleanup.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 0941b9c92b3f..05858c91c0ea 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2869,7 +2869,7 @@ static int snd_pcm_playback_ioctl1(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg) { - if (snd_BUG_ON(!substream)) + if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; if (snd_BUG_ON(substream->stream != SNDRV_PCM_STREAM_PLAYBACK)) return -EINVAL; @@ -2949,7 +2949,7 @@ static int snd_pcm_capture_ioctl1(struct file *file, struct snd_pcm_substream *substream, unsigned int cmd, void __user *arg) { - if (snd_BUG_ON(!substream)) + if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; if (snd_BUG_ON(substream->stream != SNDRV_PCM_STREAM_CAPTURE)) return -EINVAL;
We call ack callback whenever appl_ptr gets updated via pcm_lib_apply_appl_ptr(). There are various code paths to call this function. A part of them are for read/write/forward/rewind, where the appl_ptr is always changed and thus the call of ack is mandatory. OTOH, another part of code paths are from the explicit user call, e.g. via SYNC_PTR ioctl. There, we may receive the same appl_ptr value, and in such a case, calling ack is obviously superfluous.
This patch adds the check of the given appl_ptr value, and returns immediately if it's no real update.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_lib.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index e73b6e4135f6..75308ddc54ca 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2112,6 +2112,9 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream, snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr; int ret;
+ if (old_appl_ptr == appl_ptr) + return 0; + runtime->control->appl_ptr = appl_ptr; if (substream->ops->ack) { ret = substream->ops->ack(substream);
Hi,
On Jun 13 2017 23:18, Takashi Iwai wrote:
Hi,
this is a kind of gleaning work, a collection of small cleanups of ALSA PCM implementations.
Takashi
===
Takashi Iwai (6): ALSA: pcm: Clean up SNDRV_PCM_IOCTL_PAUSE code ALSA: pcm: Apply power lock globally to common ioctls ALSA: pcm: Allow dropping stream directly after resume ALSA: pcm: Preprocess PAUSED or SUSPENDED stream before PREPARE ALSA: pcm: Use common PCM_RUNTIME_CHECK() for sanity checks ALSA: pcm: Skip ack callback without actual appl_ptr update
sound/core/pcm_lib.c | 3 ++ sound/core/pcm_native.c | 84 +++++++++++++++++++++---------------------------- 2 files changed, 39 insertions(+), 48 deletions(-)
I reviewed all of changes in this patchset:
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Regards
Takashi Sakamoto
participants (2)
-
Takashi Iwai -
Takashi Sakamoto