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

Takashi Sakamoto
2017-05-22T03:49:00Z

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