[alsa-devel] Bug in snd_pcm_ioplug_avail_update()?

[Takashi Iwai]

On Tue, 17 Jul 2018 01:53:36 +0200,
Rob Duncan wrote:
> 
> 
> At 11:38 on Mon, Jul 16 2018, Takashi wrote:
> 
> > Yeah, this looks like a bug.  The code there supposed to copy the
> > whole data that is available, while snd_pcm_mmap_begin() gives the
> > range only for a single copy action, so it won't fill if the region to
> > fill is split across the buffer boundary.
> >
> > I guess we need some open-code there to achieve the whole data copy.
> > The avail value from snd_pcm_mmap_avail() can be moved before the
> > action, so that we can avoid to calculate this twice.
> >
> > If you have already some fix patch, it'd be helpful.  Otherwise I'll
> > fix it up some time later.
> 
> How about this?
> 
> Rob.
> 
> >From 467766fc0bd2fb0981d699d018114fa348ddc6ac Mon Sep 17 00:00:00 2001
> From: Rob Duncan <rduncan at teslamotors.com>
> Date: Mon, 16 Jul 2018 16:35:23 -0700
> Subject: [PATCH] pcm: ioplug: Transfer all available data
> 
> The snd_pcm_mmap_begin() call returns the amount of contiguous data,
> which is less than the total available if it wraps around the buffer
> boundary.
> 
> If we don't handle this split we leave stale data in the buffer that
> should have been overwritten, as well as unread data in the io_plugin
> that gets transferred on a subsequent call at the wrong offset.
> 
> Signed-off-by: Rob Duncan <rduncan at teslamotors.com>

The code change is similar as what I had in my mind, but...


> ---
>  src/pcm/pcm_ioplug.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
> index 6d52c27b..90e55d0a 100644
> --- a/src/pcm/pcm_ioplug.c
> +++ b/src/pcm/pcm_ioplug.c
> @@ -713,6 +713,8 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
>  	ioplug_priv_t *io = pcm->private_data;
>  	snd_pcm_uframes_t avail;
>  
> +	avail = snd_pcm_mmap_avail(pcm);

This call should be put after the hwptr update.  That is...


> +
>  	snd_pcm_ioplug_hw_ptr_update(pcm);
>  	if (io->data->state == SND_PCM_STATE_XRUN)
>  		return -EPIPE;

... put here.

In addition:

> @@ -728,9 +730,15 @@ static snd_pcm_sframes_t snd_pcm_ioplug_avail_update(snd_pcm_t *pcm)
>  			result = io->data->callback->transfer(io->data, areas, offset, size);
>  			if (result < 0)
>  				return result;
> +
> +			if (size < avail) {
> +				result = io->data->callback->transfer(io->data, areas,
> +								      0, avail - size);
> +				if (result < 0)
> +					return result;
> +			}

Please give a short comment about the buffer boundary wrap for
readers.


Thanks!

Takashi