[alsa-devel] [PATCH - alsa-lib] pcm/ioplug: Add mutex in snd_pcm_ioplug_hw_ptr_update

[Vishal Agrawal]

Hi Takashi,

Thanks for the reply.
I saw the race between "delay" and "avail_update" function.
>From GStreamer stack these two function gets called from different threads.

Caller is calling two different function here it doesn't know about race
condition.
And this race condition doesn't happen with HW plug-in, Its only with
ioplug plug-in.

Thanks,
Vishal Agrawal


On Tue, Oct 29, 2013 at 4:12 PM, Takashi Iwai <tiwai at suse.de> wrote:

> At Tue, 29 Oct 2013 15:52:16 +0530,
> Vishal Agrawal wrote:
> >
> > Function snd_pcm_ioplug_hw_ptr_update can be called from many threads, we
> > need to protect the hw_ptr. I came across this issue with GStreamer and
> > Jackd. Function will be called from the thread doing snd_pcm_write and
> > another would be to get the playback position for the track bar.
> >
> > Change in itself is very minimum, but it took me a lot of time to debug.
>
> We'd like to avoid mutex in the alsa-lib code as much as possible.
> In which code paths did you get the race?  In general, the code paths
> involved with hw_ptr_update() are known to be thread-unsafe, so it's
> the responsibility of the caller side to protect the race.
>
>
> thanks,
>
> Takashi
>
> >
> > Thanks,
> > Vishal Agrawal
> > [1.2  <text/html; ISO-8859-1 (quoted-printable)>]
> >
> > [2 0001-pcm-ioplug-Add-mutex-in-snd_pcm_ioplug_hw_ptr_update.patch
> <text/x-patch; US-ASCII (base64)>]
> > From b826b1f28c50bd4a414cf50751be4ac74b4e4174 Mon Sep 17 00:00:00 2001
> > From: Vishal Agrawal <visagrawal at gmail.com>
> > Date: Mon, 28 Oct 2013 18:01:14 +0530
> > Subject: [PATCH - alsa-lib] pcm/ioplug: Add mutex in
> >  snd_pcm_ioplug_hw_ptr_update
> >
> > snd_pcm_ioplug_hw_ptr_update() can be called from different
> > threads, it needs to be protected by mutex
> >
> > Signed-off-by: Vishal Agrawal <visagrawal at gmail.com>
> >
> > diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
> > index a90c844..23dbee3 100644
> > --- a/src/pcm/pcm_ioplug.c
> > +++ b/src/pcm/pcm_ioplug.c
> > @@ -26,6 +26,7 @@
> >   *
> >   */
> >
> > +#include <pthread.h>
> >  #include "pcm_local.h"
> >  #include "pcm_ioplug.h"
> >  #include "pcm_ext_parm.h"
> > @@ -47,12 +48,15 @@ typedef struct snd_pcm_ioplug_priv {
> >       snd_htimestamp_t trigger_tstamp;
> >  } ioplug_priv_t;
> >
> > +static pthread_mutex_t hw_ptr_mutex = PTHREAD_MUTEX_INITIALIZER;
> > +
> >  /* update the hw pointer */
> >  static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
> >  {
> >       ioplug_priv_t *io = pcm->private_data;
> >       snd_pcm_sframes_t hw;
> >
> > +     pthread_mutex_lock(&hw_ptr_mutex);
> >       hw = io->data->callback->pointer(io->data);
> >       if (hw >= 0) {
> >               unsigned int delta;
> > @@ -64,6 +68,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t
> *pcm)
> >               io->last_hw = hw;
> >       } else
> >               io->data->state = SNDRV_PCM_STATE_XRUN;
> > +     pthread_mutex_unlock(&hw_ptr_mutex);
> >  }
> >
> >  static int snd_pcm_ioplug_info(snd_pcm_t *pcm, snd_pcm_info_t *info)
> > --
> > 1.7.9.5
> >
>