Re: [PATCH v2 2/6] ALSA: compress: Add function to indicate the stream has gone bad

[Charles Keepax <ckeepax@opensource.wolfsonmicro.com>]

On Thu, Apr 07, 2016 at 12:40:03AM +0000, Koul, Vinod wrote:
> On Wed, 2016-04-06 at 11:21 +0100, Charles Keepax wrote:
> 
> > +int snd_compr_stop_xrun(struct snd_compr_stream *stream)
> > +{
> > +	if (stream->runtime->state == SNDRV_PCM_STATE_XRUN)
> > +		return 0;
> > +
> > +	stream->runtime->state = SNDRV_PCM_STATE_XRUN;
> > +
> > +	queue_delayed_work(system_power_efficient_wq, &stream
> > ->xrun_work, 0);
> 
> why do we want to do this in workqueue and not stop the compress stream
> immediately.

We need to defer this to a work queue because it is very likely
we will detect the errors whilst already in a callback in the
driver. For example we notice the stream is bad whilst processing
a read or a pointer callback in the driver. Because this call by
definition goes right back to the top of the stack rather than
unwinding the stack nicely as returning an error would do, we
need to be careful of all the locks that are likely to be held in
between.

snd_compr_ioctl - takes stream->device->lock
snd_compr_tstamp
soc_compr_pointer - takes rtd->pcm_mutex
wm_adsp_compr_pointer - takes dsp->pwr_lock
snd_compr_stop_xrun
snd_compr_stop
soc_compr_trigger - Deadlock as we take rtd->pcm_mutex again

> 
> Also if we do this, then why should pointer return error?

The first patch in the chain could indeed be changed to have
pointer calls not return an error status. But I feel that would
be making the code worse. Ok the situation I am most interested
here indicates a failure of the stream, but its a very small leap
to imagine situations where pointer fails temporarily and the
stream is still good.

Thanks,
Charles