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

[Takashi Iwai]

On Fri, 08 Apr 2016 01:07:12 +0200,
Vinod Koul wrote:
> 
> 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?

Yes, it looks good to me.

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

PCM stops the stream from the lock context, so there is no leap.


thanks,

Takashi