[alsa-devel] [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Wed, 30 Oct 2013 21:03:54 +0530,
Vinod Koul wrote:
> 
> 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.

It's a long-standing limitation of POSIX.
Basically a signal handler can accept only very few functions that are
asynchronous-safe.  That's the reason why signal is practically
useless in most use cases.

>  Btw
> closing the fd should have same effect as free would trigger the stop internally

Well, better to test it before jumping into the conclusion :)

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

OK.


thanks,

Takashi