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

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

On Thu, 2016-04-07 at 09:28 +0100, Charles Keepax wrote:
> 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

This is what I suspected as well :D so this should be fine. I will take
a detailed look at the changes once am back home.

Also should this be made a generic stop rather than xrun. Perhaps the
reason can be specified as an argument.

Btw Takashi you okay with this approach?


> 
> > 
> > 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.

The point here is that we are anyway propagating error by invoking the
new API so why return error here.
Btw can you please explain how this makes code worse?

I think on PCM we don't do that.


-- 
~Vinod