[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[Takashi Iwai]

Hi,

this is a series of patches to fix the potential bugs in PCM ack
callback using the pcm-indirect helper functions, and also to enhance
the ack callback to be called properly in all appl_ptr updates, as
similarly done by Pierre and Subhransu's patches.

The changes in the pcm-indirect helper code and its users are fairly
trivial, just passing the error code back.

ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.


Takashi

===

Takashi Iwai (7):
  ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers
  ALSA: mips: Deliver indirect-PCM transfer error
  ALSA: cs46xx: Deliver indirect-PCM transfer error
  ALSA: emu10k1: Deliver indirect-PCM transfer error
  ALSA: rme32: Deliver indirect-PCM transfer error
  staging: bcm2835-audio: Deliver indirect-PCM transfer error
  ALSA: pcm: Call ack() whenever appl_ptr is updated

 .../vc04_services/bcm2835-audio/bcm2835-pcm.c      |  5 +--
 include/sound/pcm-indirect.h                       | 10 ++++-
 sound/core/pcm_native.c                            | 46 +++++++++++++++++-----
 sound/mips/hal2.c                                  | 14 +++----
 sound/pci/cs46xx/cs46xx_lib.c                      |  8 ++--
 sound/pci/emu10k1/emupcm.c                         |  4 +-
 sound/pci/rme32.c                                  | 10 ++---
 7 files changed, 63 insertions(+), 34 deletions(-)

-- 
2.13.0

[PATCH 1/7] ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers

[Takashi Iwai]

The indirect-PCM helper codes have an implicit assumption that the
appl_ptr always increases.  But the PCM core may deal with the
decrement of appl_ptr via rewind ioctls, and it may screw up the
buffer pointer management.

This patch adds the negative appl_ptr diff in transfer functions and
let returning an error instead of always accepting the appl_ptr
updates.  The callers are usually PCM ack callbacks, and they pass the
error to the upper layer accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm-indirect.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
index 1df7acaaa535..7ade285328cf 100644
--- a/include/sound/pcm-indirect.h
+++ b/include/sound/pcm-indirect.h
@@ -43,7 +43,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
 /*
  * helper function for playback ack callback
  */
-static inline void
+static inline int
 snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 				   struct snd_pcm_indirect *rec,
 				   snd_pcm_indirect_copy_t copy)
@@ -56,6 +56,8 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if (diff < 0)
+			return -EINVAL;
 		rec->sw_ready += (int)frames_to_bytes(runtime, diff);
 		rec->appl_ptr = appl_ptr;
 	}
@@ -82,6 +84,7 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 		rec->hw_ready += bytes;
 		rec->sw_ready -= bytes;
 	}
+	return 0;
 }
 
 /*
@@ -109,7 +112,7 @@ snd_pcm_indirect_playback_pointer(struct snd_pcm_substream *substream,
 /*
  * helper function for capture ack callback
  */
-static inline void
+static inline int
 snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 				  struct snd_pcm_indirect *rec,
 				  snd_pcm_indirect_copy_t copy)
@@ -121,6 +124,8 @@ snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if (diff < 0)
+			return -EINVAL;
 		rec->sw_ready -= frames_to_bytes(runtime, diff);
 		rec->appl_ptr = appl_ptr;
 	}
@@ -147,6 +152,7 @@ snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 		rec->hw_ready -= bytes;
 		rec->sw_ready += bytes;
 	}
+	return 0;
 }
 
 /*
-- 
2.13.0

[PATCH 2/7] ALSA: mips: Deliver indirect-PCM transfer error

[Takashi Iwai]

Now that the indirect-PCM transfer helper gives back an error, we
should return the error from ack callbacks.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/mips/hal2.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
index 00fc9241d266..684dc4ddef41 100644
--- a/sound/mips/hal2.c
+++ b/sound/mips/hal2.c
@@ -616,10 +616,9 @@ static int hal2_playback_ack(struct snd_pcm_substream *substream)
 	struct hal2_codec *dac = &hal2->dac;
 
 	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
-	snd_pcm_indirect_playback_transfer(substream,
-					   &dac->pcm_indirect,
-					   hal2_playback_transfer);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream,
+						  &dac->pcm_indirect,
+						  hal2_playback_transfer);
 }
 
 static int hal2_capture_open(struct snd_pcm_substream *substream)
@@ -707,10 +706,9 @@ static int hal2_capture_ack(struct snd_pcm_substream *substream)
 	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
 	struct hal2_codec *adc = &hal2->adc;
 
-	snd_pcm_indirect_capture_transfer(substream,
-					  &adc->pcm_indirect,
-					  hal2_capture_transfer);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream,
+						 &adc->pcm_indirect,
+						 hal2_capture_transfer);
 }
 
 static struct snd_pcm_ops hal2_playback_ops = {
-- 
2.13.0

[PATCH 3/7] ALSA: cs46xx: Deliver indirect-PCM transfer error

[Takashi Iwai]

Now that the indirect-PCM transfer helper gives back an error, we
should return the error from ack callbacks.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/cs46xx/cs46xx_lib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
index 00fa52e9a2f2..ae2aad52ea6f 100644
--- a/sound/pci/cs46xx/cs46xx_lib.c
+++ b/sound/pci/cs46xx/cs46xx_lib.c
@@ -887,8 +887,8 @@ static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
-	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec,
+						  snd_cs46xx_pb_trans_copy);
 }
 
 static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
@@ -903,8 +903,8 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
 static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
 {
 	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
-	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec,
+						 snd_cs46xx_cp_trans_copy);
 }
 
 static snd_pcm_uframes_t snd_cs46xx_playback_direct_pointer(struct snd_pcm_substream *substream)
-- 
2.13.0

[PATCH 4/7] ALSA: emu10k1: Deliver indirect-PCM transfer error

[Takashi Iwai]

Now that the indirect-PCM transfer helper gives back an error, we
should return the error from ack callbacks.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/emu10k1/emupcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index ef1cf530c929..bdda29f335f6 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -1632,8 +1632,8 @@ static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substr
 	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
 	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
 
-	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec,
+						  fx8010_pb_trans_copy);
 }
 
 static int snd_emu10k1_fx8010_playback_hw_params(struct snd_pcm_substream *substream,
-- 
2.13.0

[PATCH 5/7] ALSA: rme32: Deliver indirect-PCM transfer error

[Takashi Iwai]

Now that the indirect-PCM transfer helper gives back an error, we
should return the error from ack callbacks.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/rme32.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
index 96d15db65dfd..f9b424056d0f 100644
--- a/sound/pci/rme32.c
+++ b/sound/pci/rme32.c
@@ -1157,9 +1157,8 @@ static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
 	if (rme32->running & (1 << SNDRV_PCM_STREAM_CAPTURE))
 		rec->hw_queue_size -= cprec->hw_ready;
 	spin_unlock(&rme32->lock);
-	snd_pcm_indirect_playback_transfer(substream, rec,
-					   snd_rme32_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, rec,
+						  snd_rme32_pb_trans_copy);
 }
 
 static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
@@ -1174,9 +1173,8 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
 static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
 {
 	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
-	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
-					  snd_rme32_cp_trans_copy);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
+						 snd_rme32_cp_trans_copy);
 }
 
 static snd_pcm_uframes_t
-- 
2.13.0

[PATCH 6/7] staging: bcm2835-audio: Deliver indirect-PCM transfer error

[Takashi Iwai]

Now that the indirect-PCM transfer helper gives back an error, we
should return the error from ack callbacks.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index e8cf0b97bf02..3637ddf909a4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -353,9 +353,8 @@ static int snd_bcm2835_pcm_ack(struct snd_pcm_substream *substream)
 	struct snd_pcm_indirect *pcm_indirect = &alsa_stream->pcm_indirect;
 
 	pcm_indirect->hw_queue_size = runtime->hw.buffer_bytes_max;
-	snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
-					   snd_bcm2835_pcm_transfer);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
+						  snd_bcm2835_pcm_transfer);
 }
 
 /* trigger callback */
-- 
2.13.0

[PATCH 6/7] staging: bcm2835-audio: Deliver indirect-PCM transfer error

[Eric Anholt]

Takashi Iwai <tiwai@suse.de> writes:

> Now that the indirect-PCM transfer helper gives back an error, we
> should return the error from ack callbacks.

As far as bcm2835 goes:

Acked-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170522/e51eb81a/attachment.sig>

[PATCH 7/7] ALSA: pcm: Call ack() whenever appl_ptr is updated

[Takashi Iwai]

Although the ack callback is supposed to be called at each appl_ptr or
hw_ptr update, we missed a few opportunities: namely, forward, rewind
and sync_ptr.

Formerly calling ack at rewind may have leaded to unexpected results
due to the forgotten negative appl_ptr update in indirect-PCM helper,
which is the major user of the PCM ack callback.  But now we fixed
this oversights, thus we can call ack callback safely even at rewind
callback -- of course with the proper handling of the error from the
callback.

This patch adds the calls of ack callback in the places mentioned in
the above.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ecde57afa45a..7bc4a0bbad6f 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2455,13 +2455,35 @@ static int do_pcm_hwsync(struct snd_pcm_substream *substream)
 	}
 }
 
-/* increase the appl_ptr; returns the processed frames */
+/* update to the given appl_ptr and call ack callback if needed;
+ * when an error is returned, take back to the original value
+ */
+static int apply_appl_ptr(struct snd_pcm_substream *substream,
+			  snd_pcm_uframes_t appl_ptr)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
+	int ret;
+
+	runtime->control->appl_ptr = appl_ptr;
+	if (substream->ops->ack) {
+		ret = substream->ops->ack(substream);
+		if (ret < 0) {
+			runtime->control->appl_ptr = old_appl_ptr;
+			return ret;
+		}
+	}
+	return 0;
+}
+
+/* increase the appl_ptr; returns the processed frames or a negative error */
 static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream,
 					  snd_pcm_uframes_t frames,
 					   snd_pcm_sframes_t avail)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	snd_pcm_sframes_t appl_ptr;
+	int ret;
 
 	if (avail <= 0)
 		return 0;
@@ -2470,17 +2492,18 @@ static snd_pcm_sframes_t forward_appl_ptr(struct snd_pcm_substream *substream,
 	appl_ptr = runtime->control->appl_ptr + frames;
 	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
 		appl_ptr -= runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	return frames;
+	ret = apply_appl_ptr(substream, appl_ptr);
+	return ret < 0 ? ret : frames;
 }
 
-/* decrease the appl_ptr; returns the processed frames */
+/* decrease the appl_ptr; returns the processed frames or a negative error */
 static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream,
 					 snd_pcm_uframes_t frames,
 					 snd_pcm_sframes_t avail)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	snd_pcm_sframes_t appl_ptr;
+	int ret;
 
 	if (avail <= 0)
 		return 0;
@@ -2489,8 +2512,8 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream,
 	appl_ptr = runtime->control->appl_ptr - frames;
 	if (appl_ptr < 0)
 		appl_ptr += runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	return frames;
+	ret = apply_appl_ptr(substream, appl_ptr);
+	return ret < 0 ? ret : frames;
 }
 
 static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream,
@@ -2620,10 +2643,15 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 			return err;
 	}
 	snd_pcm_stream_lock_irq(substream);
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
-		control->appl_ptr = sync_ptr.c.control.appl_ptr;
-	else
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
+		if (err < 0) {
+			snd_pcm_stream_unlock_irq(substream);
+			return err;
+		}
+	} else {
 		sync_ptr.c.control.appl_ptr = control->appl_ptr;
+	}
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
 		control->avail_min = sync_ptr.c.control.avail_min;
 	else
-- 
2.13.0

[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[Takashi Sakamoto]

Hi,

On May 22 2017 04:02, Takashi Iwai wrote:
> Hi,
> 
> this is a series of patches to fix the potential bugs in PCM ack
> callback using the pcm-indirect helper functions, and also to enhance
> the ack callback to be called properly in all appl_ptr updates, as
> similarly done by Pierre and Subhransu's patches.
> 
> The changes in the pcm-indirect helper code and its users are fairly
> trivial, just passing the error code back.
> 
> ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.
> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (7):
>    ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers
>    ALSA: mips: Deliver indirect-PCM transfer error
>    ALSA: cs46xx: Deliver indirect-PCM transfer error
>    ALSA: emu10k1: Deliver indirect-PCM transfer error
>    ALSA: rme32: Deliver indirect-PCM transfer error
>    staging: bcm2835-audio: Deliver indirect-PCM transfer error
>    ALSA: pcm: Call ack() whenever appl_ptr is updated
> 
>   .../vc04_services/bcm2835-audio/bcm2835-pcm.c      |  5 +--
>   include/sound/pcm-indirect.h                       | 10 ++++-
>   sound/core/pcm_native.c                            | 46 +++++++++++++++++-----
>   sound/mips/hal2.c                                  | 14 +++----
>   sound/pci/cs46xx/cs46xx_lib.c                      |  8 ++--
>   sound/pci/emu10k1/emupcm.c                         |  4 +-
>   sound/pci/rme32.c                                  | 10 ++---
>   7 files changed, 63 insertions(+), 34 deletions(-)

I mostly agree with this patchset, except for one point.

The added helper function, apply_appl_ptr(), copies given data to the 
runtime for application-side pointer, then call 'struct 
snd_pcm_ops.ack'. We have similar code block in 'sound/core/pcm_lib.c', 
as well. See 'snd_pcm_lib_write1()' and 'snd_pcm_lib_read1()'.

In a point of code maintenance, it's better to share the function 
between two objects; pcm_native.o and pcm_lib.o.

For instance, I can propose a below patch. I expect you to squash it up 
within one of your patches.

========== 8< ----------

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index ab4b1d1e44ee..9d048f91bd3f 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2010,6 +2010,12 @@ typedef int (*transfer_f)(struct 
snd_pcm_substream *substream, unsigned int hwof
  			  unsigned long data, unsigned int off,
  			  snd_pcm_uframes_t size);

+/* This symbol is shared by several relocatable objects for the same shared
+ * object.
+ */
+int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
+			  snd_pcm_uframes_t appl_ptr);
+
  static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream 
*substream,
  					    unsigned long data,
  					    snd_pcm_uframes_t size,
@@ -2087,11 +2093,9 @@ static snd_pcm_sframes_t 
snd_pcm_lib_write1(struct snd_pcm_substream *substream,
  			break;
  		}
  		appl_ptr += frames;
-		if (appl_ptr >= runtime->boundary)
-			appl_ptr -= runtime->boundary;
-		runtime->control->appl_ptr = appl_ptr;
-		if (substream->ops->ack)
-			substream->ops->ack(substream);
+		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
+		if (err < 0)
+			goto _end_unlock;

  		offset += frames;
  		size -= frames;
@@ -2321,9 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct 
snd_pcm_substream *substream,
  		appl_ptr += frames;
  		if (appl_ptr >= runtime->boundary)
  			appl_ptr -= runtime->boundary;
-		runtime->control->appl_ptr = appl_ptr;
-		if (substream->ops->ack)
-			substream->ops->ack(substream);
+		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
+		if (err < 0)
+			goto _end_unlock;

  		offset += frames;
  		size -= frames;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 7bc4a0bbad6f..0b176b35ade1 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2456,9 +2456,11 @@ static int do_pcm_hwsync(struct snd_pcm_substream 
*substream)
  }

  /* update to the given appl_ptr and call ack callback if needed;
- * when an error is returned, take back to the original value
+ * when an error is returned, take back to the original value.
+ * The given argument should have valid value as application pointer on PCM
+ * buffer.
   */
-static int apply_appl_ptr(struct snd_pcm_substream *substream,
+int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
  			  snd_pcm_uframes_t appl_ptr)
  {
  	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -2492,7 +2494,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct 
snd_pcm_substream *substream,
  	appl_ptr = runtime->control->appl_ptr + frames;
  	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
  		appl_ptr -= runtime->boundary;
-	ret = apply_appl_ptr(substream, appl_ptr);
+	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
  	return ret < 0 ? ret : frames;
  }

@@ -2512,7 +2514,7 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct 
snd_pcm_substream *substream,
  	appl_ptr = runtime->control->appl_ptr - frames;
  	if (appl_ptr < 0)
  		appl_ptr += runtime->boundary;
-	ret = apply_appl_ptr(substream, appl_ptr);
+	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
  	return ret < 0 ? ret : frames;
  }

@@ -2644,7 +2646,8 @@ static int snd_pcm_sync_ptr(struct 
snd_pcm_substream *substream,
  	}
  	snd_pcm_stream_lock_irq(substream);
  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
-		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
+		err = snd_pcm_apply_appl_ptr(substream,
+					     sync_ptr.c.control.appl_ptr);
  		if (err < 0) {
  			snd_pcm_stream_unlock_irq(substream);
  			return err;


Regards

Takashi Sakamoto

[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[Takashi Iwai]

On Mon, 22 May 2017 05:49:00 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On May 22 2017 04:02, Takashi Iwai wrote:
> > Hi,
> >
> > this is a series of patches to fix the potential bugs in PCM ack
> > callback using the pcm-indirect helper functions, and also to enhance
> > the ack callback to be called properly in all appl_ptr updates, as
> > similarly done by Pierre and Subhransu's patches.
> >
> > The changes in the pcm-indirect helper code and its users are fairly
> > trivial, just passing the error code back.
> >
> > ARM/R-Pi people are Cc'ed because of bcm2835-audio driver.
> >
> >
> > Takashi
> >
> > ===
> >
> > Takashi Iwai (7):
> >    ALSA: pcm: Fix negative appl_ptr handling in pcm-indirect helpers
> >    ALSA: mips: Deliver indirect-PCM transfer error
> >    ALSA: cs46xx: Deliver indirect-PCM transfer error
> >    ALSA: emu10k1: Deliver indirect-PCM transfer error
> >    ALSA: rme32: Deliver indirect-PCM transfer error
> >    staging: bcm2835-audio: Deliver indirect-PCM transfer error
> >    ALSA: pcm: Call ack() whenever appl_ptr is updated
> >
> >   .../vc04_services/bcm2835-audio/bcm2835-pcm.c      |  5 +--
> >   include/sound/pcm-indirect.h                       | 10 ++++-
> >   sound/core/pcm_native.c                            | 46 +++++++++++++++++-----
> >   sound/mips/hal2.c                                  | 14 +++----
> >   sound/pci/cs46xx/cs46xx_lib.c                      |  8 ++--
> >   sound/pci/emu10k1/emupcm.c                         |  4 +-
> >   sound/pci/rme32.c                                  | 10 ++---
> >   7 files changed, 63 insertions(+), 34 deletions(-)
> 
> I mostly agree with this patchset, except for one point.
> 
> The added helper function, apply_appl_ptr(), copies given data to the
> runtime for application-side pointer, then call 'struct
> snd_pcm_ops.ack'. We have similar code block in
> 'sound/core/pcm_lib.c', as well. See 'snd_pcm_lib_write1()' and
> 'snd_pcm_lib_read1()'.
> 
> In a point of code maintenance, it's better to share the function
> between two objects; pcm_native.o and pcm_lib.o.
> 
> For instance, I can propose a below patch. I expect you to squash it
> up within one of your patches.
> 
> ========== 8< ----------
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index ab4b1d1e44ee..9d048f91bd3f 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2010,6 +2010,12 @@ typedef int (*transfer_f)(struct
> snd_pcm_substream *substream, unsigned int hwof
>  			  unsigned long data, unsigned int off,
>  			  snd_pcm_uframes_t size);
> 
> +/* This symbol is shared by several relocatable objects for the same shared
> + * object.
> + */
> +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
> +			  snd_pcm_uframes_t appl_ptr);
> +
>  static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream
> *substream,
>  					    unsigned long data,
>  					    snd_pcm_uframes_t size,
> @@ -2087,11 +2093,9 @@ static snd_pcm_sframes_t
> snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  			break;
>  		}
>  		appl_ptr += frames;
> -		if (appl_ptr >= runtime->boundary)
> -			appl_ptr -= runtime->boundary;
> -		runtime->control->appl_ptr = appl_ptr;
> -		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
> +		if (err < 0)
> +			goto _end_unlock;
> 
>  		offset += frames;
>  		size -= frames;
> @@ -2321,9 +2325,9 @@ static snd_pcm_sframes_t
> snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  		appl_ptr += frames;
>  		if (appl_ptr >= runtime->boundary)
>  			appl_ptr -= runtime->boundary;
> -		runtime->control->appl_ptr = appl_ptr;
> -		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +		err = snd_pcm_apply_appl_ptr(substream, appl_ptr);
> +		if (err < 0)
> +			goto _end_unlock;
> 
>  		offset += frames;
>  		size -= frames;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 7bc4a0bbad6f..0b176b35ade1 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2456,9 +2456,11 @@ static int do_pcm_hwsync(struct
> snd_pcm_substream *substream)
>  }
> 
>  /* update to the given appl_ptr and call ack callback if needed;
> - * when an error is returned, take back to the original value
> + * when an error is returned, take back to the original value.
> + * The given argument should have valid value as application pointer on PCM
> + * buffer.
>   */
> -static int apply_appl_ptr(struct snd_pcm_substream *substream,
> +int snd_pcm_apply_appl_ptr(struct snd_pcm_substream *substream,
>  			  snd_pcm_uframes_t appl_ptr)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
> @@ -2492,7 +2494,7 @@ static snd_pcm_sframes_t forward_appl_ptr(struct
> snd_pcm_substream *substream,
>  	appl_ptr = runtime->control->appl_ptr + frames;
>  	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
>  		appl_ptr -= runtime->boundary;
> -	ret = apply_appl_ptr(substream, appl_ptr);
> +	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
>  	return ret < 0 ? ret : frames;
>  }
> 
> @@ -2512,7 +2514,7 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct
> snd_pcm_substream *substream,
>  	appl_ptr = runtime->control->appl_ptr - frames;
>  	if (appl_ptr < 0)
>  		appl_ptr += runtime->boundary;
> -	ret = apply_appl_ptr(substream, appl_ptr);
> +	ret = snd_pcm_apply_appl_ptr(substream, appl_ptr);
>  	return ret < 0 ? ret : frames;
>  }
> 
> @@ -2644,7 +2646,8 @@ static int snd_pcm_sync_ptr(struct
> snd_pcm_substream *substream,
>  	}
>  	snd_pcm_stream_lock_irq(substream);
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> -		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
> +		err = snd_pcm_apply_appl_ptr(substream,
> +					     sync_ptr.c.control.appl_ptr);
>  		if (err < 0) {
>  			snd_pcm_stream_unlock_irq(substream);
>  			return err;

These functions will be modified again by other upcoming patchsets, so
I'd like not to touch so much for minor optimization at this stage.
Let's postpone that after all settled down.


thanks,

Takashi

[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[Takashi Sakamoto]

On May 22 2017 14:27, Takashi Iwai wrote:
> These functions will be modified again by other upcoming patchsets, so
> I'd like not to touch so much for minor optimization at this stage.

If you address to your latest patchset[1], it might not be applied 
promptly because I have some comments (not posted yet, in this night). 
It's not good to avoid better bahaviour in a reason of patchset 
dependency, at least you seems to be too hasty.


[1] [alsa-devel] [PATCH 00/16] ALSA: Convert to new copy_silence PCM ops

Regards

Takashi Sakamoto

[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[Takashi Iwai]

On Mon, 22 May 2017 08:31:13 +0200,
Takashi Sakamoto wrote:
> 
> On May 22 2017 14:27, Takashi Iwai wrote:
> > These functions will be modified again by other upcoming patchsets, so
> > I'd like not to touch so much for minor optimization at this stage.
> 
> If you address to your latest patchset[1], it might not be applied
> promptly because I have some comments (not posted yet, in this
> night). It's not good to avoid better bahaviour in a reason of
> patchset dependency, at least you seems to be too hasty.

Come on, your suggestion is an optimization change that can be applied
anytime later easily.  If it were a mandatory fix, we should apply or
fold it immediately, but that's not the case.  The only necessary
thing is not to forget it.


Takashi

> 
> 
> [1] [alsa-devel] [PATCH 00/16] ALSA: Convert to new copy_silence PCM ops
> 
> Regards
> 
> Takashi Sakamoto
> 

[PATCH 0/7] ALSA: Fix/improve PCM ack callback

[Takashi Iwai]

On Mon, 22 May 2017 08:46:56 +0200,
Takashi Iwai wrote:
> 
> On Mon, 22 May 2017 08:31:13 +0200,
> Takashi Sakamoto wrote:
> > 
> > On May 22 2017 14:27, Takashi Iwai wrote:
> > > These functions will be modified again by other upcoming patchsets, so
> > > I'd like not to touch so much for minor optimization at this stage.
> > 
> > If you address to your latest patchset[1], it might not be applied
> > promptly because I have some comments (not posted yet, in this
> > night). It's not good to avoid better bahaviour in a reason of
> > patchset dependency, at least you seems to be too hasty.
> 
> Come on, your suggestion is an optimization change that can be applied
> anytime later easily.  If it were a mandatory fix, we should apply or
> fold it immediately, but that's not the case.  The only necessary
> thing is not to forget it.

That implies: just submit your proposed change as an incremental patch
on top of mine.  Then I'll queue it.


thanks,

Takashi