[PATCH 0/6] Misc PCM fixes

[PATCH 0/6] Misc PCM fixes

[Takashi Iwai]

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(-)

-- 
2.13.1

[PATCH 1/6] ALSA: pcm: Clean up SNDRV_PCM_IOCTL_PAUSE code

[Takashi Iwai]

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;
-- 
2.13.1

[PATCH 2/6] ALSA: pcm: Apply power lock globally to common ioctls

[Takashi Iwai]

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)
-- 
2.13.1

[PATCH 3/6] ALSA: pcm: Allow dropping stream directly after resume

[Takashi Iwai]

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);
-- 
2.13.1

[PATCH 4/6] ALSA: pcm: Preprocess PAUSED or SUSPENDED stream before PREPARE

[Takashi Iwai]

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);
 }
-- 
2.13.1

[PATCH 5/6] ALSA: pcm: Use common PCM_RUNTIME_CHECK() for sanity checks

[Takashi Iwai]

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;
-- 
2.13.1

[PATCH 6/6] ALSA: pcm: Skip ack callback without actual appl_ptr update

[Takashi Iwai]

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);
-- 
2.13.1

Re: [PATCH 4/6] ALSA: pcm: Preprocess PAUSED or SUSPENDED stream before PREPARE

[Takashi Sakamoto]

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

Re: [PATCH 0/6] Misc PCM fixes

[Takashi Sakamoto]

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