[alsa-devel] [PATCH 2/9] ALSA: compress: use mutex in drain

[Takashi Iwai]

At Tue, 27 Aug 2013 15:17:41 +0530,
Vinod Koul wrote:
> 
> On Tue, Aug 27, 2013 at 12:25:23PM +0200, Takashi Iwai wrote:
> > At Tue, 27 Aug 2013 12:10:32 +0530,
> > Vinod Koul wrote:
> > > 
> > > Since we dont have lock over the function, we need to aquire mutex when checking
> > > and modfying states in drain and partial_drain handlers
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> > > Cc: <stable at vger.kernel.org>
> > > ---
> > >  sound/core/compress_offload.c |   22 +++++++++++++++++++---
> > >  1 files changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index 868aefd..e640f8c 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -676,18 +676,29 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > >  	return retval;
> > >  }
> > >  
> > > +/* this fn is called without lock being held and we change stream states here
> > > + * so while using the stream state auquire the lock but relase before invoking
> > > + * DSP as the call will possibly take a while
> > > + */
> > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > >  {
> > >  	int retval;
> > >  
> > > +	mutex_lock(&stream->device->lock);
> > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > -			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > -		return -EPERM;
> > > +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> > > +		retval = -EPERM;
> > > +		goto ret;
> > > +	}
> > > +	mutex_unlock(&stream->device->lock);
> > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > 
> > Why release the lock here?  The trigger callback is called within this
> > mutex lock in other places.
> This is the main part :)
> 
> Since the drain states will take a while (order of few seconds) to execute so we
> will be blocked. Thats why we cant have lock here.

What's about other places calling the trigger ops within lock?
You can't mix things.

> The point of lock is to sync
> the stream states here.

Without the lock, it's racy.  What if another thread calls the same
function at the same time?

> We are not modfying anything. During drain and partial
> drain we need to allow other trigger ops like pause, stop tog o thru so drop the
> lock here for these two ops only!

Well, the biggest problem is that there is no proper design for which
ops take a lock and which not.  The trigger callback is basically to
trigger the action.  There should be no long-time blocking there.
(Otherwise you'll definitely loose a gunfight :)


Takashi