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

[Takashi Iwai]

At Tue, 27 Aug 2013 15:46:14 +0530,
Vinod Koul wrote:
> 
> 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

An ops callback must be defined either to be locked or unlocked.
Calling in the unlocked context only for some case doesn't sound
right.

> > > 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.

But how?  The place you're calling trigger is unlocked.
Suppose another thread calling trigger_stop just between
mutex_unlock() and stream->ops->trigger(DRAIN) call in the above.
The state check doesn't work there.

> > > 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?

Split the drain action in two parts, trigger and synchronization:

	lock();
	...
	trigger(pause);
	while (!pause_finished) {
		unlock();
		schedule_or_sleep_or_whatever();
		lock();
	}
	...
	unlock();


Takashi