Re: [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul <vinod.koul@intel.com>]

On Wed, Oct 30, 2013 at 05:24:12PM +0100, Takashi Iwai wrote:
> At Wed, 30 Oct 2013 20:48:40 +0530,
> Vinod Koul wrote:
> > 
> > On Wed, Oct 30, 2013 at 05:01:38PM +0100, Takashi Iwai wrote:
> >  
> > > > > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > > > > +{
> > > > > > +	/*
> > > > > > +	 * We are called with lock held. So drop the lock while we wait for
> > > > > > +	 * drain complete notfication from the driver
> > > > > > +	 *
> > > > > > +	 * It is expected that driver will notify the drain completion and then
> > > > > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > > > > +	 * error. We can trigger next track after this.
> > > > > > +	 */
> > > > > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > > > +	mutex_unlock(&stream->device->lock);
> > > > > > +
> > > > > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > > > > 
> > > > > Don't you really want the interruption?
> > > > > Or use wait_event_interruptible() and return error appropriately.
> > > > any interruption from usermode should trigger the compress_stop/compress_free
> > > 
> > > ... but how?
> > > 
> > > > which will unblock this. I dont see the need to have interruptible here
> > > 
> > > Suppose you're running a single thread program.  Then who triggers the
> > > stop?
> > Your signal catcher which should be registered for signals intrested and then
> > calls stop or free. Assuming this should be considered single threaded. IIRC,
> > this is how you do in aplay, right?
> 
> No, issuing an ioctl in a signal isn't guaranteed to work at all.
Okay thanks for pointing out, do you know why we have that limitation. Btw
closing the fd should have same effect as free would trigger the stop internally

> You shouldn't code the kernel driver relying on that userspace
> behavior.
Ok, will update it

--
~Vinod