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 <[email protected]> > > > Cc: <[email protected]> > > > --- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
