Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 29 Jun 2020 10:46:59 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     Takashi Iwai <tiwai@suse.com>, Jaroslav Kysela <perex@perex.cz>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state
Message-ID: <20200629051659.GB376808@vkoul-mobl>
References: <20200625154651.99758-1-vkoul@kernel.org>
 <20200625154651.99758-4-vkoul@kernel.org>
 <20200626103549.GB71940@ediswmail.ad.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200626103549.GB71940@ediswmail.ad.cirrus.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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