Re: [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state

[Vinod Koul]

On 26-06-20, 10:35, Charles Keepax wrote:

> > -	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > +	mutex_lock(&stream->device->lock);
> > +	/* for partial_drain case we are back to running state on success */
> > +	if (stream->partial_drain) {
> > +		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> > +		stream->partial_drain = false; /* clear this flag as well */
> > +	} else {
> > +		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > +	}
> > +	mutex_unlock(&stream->device->lock);
> 
> You have added locking here in snd_compr_drain_notify but....
> >  
> >  	wake_up(&stream->runtime->sleep);
> >  }
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index e618580feac4..1c4b2cf450a0 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -803,6 +803,9 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> >  
> >  	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
> >  	if (!retval) {
> > +		/* clear flags and stop any drain wait */
> > +		stream->partial_drain = false;
> > +		stream->metadata_set = false;
> >  		snd_compr_drain_notify(stream);
> 
> that can be called from snd_compr_stop here which is already
> holding the lock resulting in deadlock.

Thanks Charles, right somehow my testing missed this, have verified that
it is the case.

I will remove the locks here, and we should add a comment to note this..

Thanks

-- 
~Vinod