Re: [PATCH 2/9] ALSA: compress: use mutex in drain

[Vinod Koul <vinod.koul@intel.com>]

On Tue, Aug 27, 2013 at 12:53:54PM +0200, Takashi Iwai wrote:
> 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:
> > > > --- 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.
Well i was going to treat drain only as exception to this. The issue here is
during the long drain other triggers are perfectly valid cases
> 
> > 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?
that part can be checked by seeing if we are already draining.
> 
> > 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 :)
The reason for blocked implementation is to treat return of the call as
notifcation that draining is completed.

For example user has written all the buffers, lets says worth 3 secs and now has
triggered drain. User needs to wait till drain is complete before closing the
device etc. So he waits on drain to compelete..

Do you have a better way to manage this?

~Vinod