From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH 2/9] ALSA: compress: use mutex in drain Date: Tue, 27 Aug 2013 15:46:14 +0530 Message-ID: <20130827101614.GY2748@intel.com> References: <1377585639-29516-1-git-send-email-vinod.koul@intel.com> <1377585639-29516-3-git-send-email-vinod.koul@intel.com> <20130827094741.GV2748@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by alsa0.perex.cz (Postfix) with ESMTP id 7CC21265133 for ; Tue, 27 Aug 2013 13:01:10 +0200 (CEST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org, broonie@kernel.org, lgirdwood@gmail.com List-Id: alsa-devel@alsa-project.org 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