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

[Takashi Iwai <tiwai@suse.de>]

On Mon, 11 Apr 2016 16:27:33 +0200,
Charles Keepax wrote:
> 
> Currently, the avail IOCTL doesn't pass any error status, which
> means typically on error it simply shows no data available. This
> can lead to situations where user-space is waiting indefinitely
> for data that will never come as the DSP has suffered an
> unrecoverable error.
> 
> Add snd_compr_stop_error which end drivers can call to indicate
> the stream has suffered an unrecoverable error and stop it. The
> avail and poll IOCTLs are then updated to report if the stream is
> in an error state to user-space. Allowing the error to propagate
> out. Processing of the actual snd_compr_stop needs to be deferred
> to a worker thread as the end driver may detect the errors during
> an existing operation callback.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  include/sound/compress_driver.h |  4 +++
>  sound/core/compress_offload.c   | 60 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index c0abcdc..a057038 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -78,6 +78,7 @@ struct snd_compr_stream {
>  	struct snd_compr_ops *ops;
>  	struct snd_compr_runtime *runtime;
>  	struct snd_compr *device;
> +	struct delayed_work error_work;
>  	enum snd_compr_direction direction;
>  	bool metadata_set;
>  	bool next_track;
> @@ -187,4 +188,7 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
>  	wake_up(&stream->runtime->sleep);
>  }
>  
> +int snd_compr_stop_error(struct snd_compr_stream *stream,
> +			 snd_pcm_state_t state);
> +
>  #endif
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 507071d..86d1d85 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -67,6 +67,8 @@ struct snd_compr_file {
>  	struct snd_compr_stream stream;
>  };
>  
> +static void error_delayed_work(struct work_struct *work);
> +
>  /*
>   * a note on stream states used:
>   * we use following states in the compressed core
> @@ -123,6 +125,9 @@ static int snd_compr_open(struct inode *inode, struct file *f)
>  		snd_card_unref(compr->card);
>  		return -ENOMEM;
>  	}
> +
> +	INIT_DELAYED_WORK(&data->stream.error_work, error_delayed_work);
> +
>  	data->stream.ops = compr->ops;
>  	data->stream.direction = dirn;
>  	data->stream.private_data = compr->private_data;
> @@ -153,6 +158,8 @@ static int snd_compr_free(struct inode *inode, struct file *f)
>  	struct snd_compr_file *data = f->private_data;
>  	struct snd_compr_runtime *runtime = data->stream.runtime;
>  
> +	cancel_delayed_work_sync(&data->stream.error_work);
> +
>  	switch (runtime->state) {
>  	case SNDRV_PCM_STATE_RUNNING:
>  	case SNDRV_PCM_STATE_DRAINING:
> @@ -237,6 +244,14 @@ snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg)
>  	avail = snd_compr_calc_avail(stream, &ioctl_avail);
>  	ioctl_avail.avail = avail;
>  
> +	switch (stream->runtime->state) {
> +	case SNDRV_PCM_STATE_OPEN:
> +	case SNDRV_PCM_STATE_XRUN:
> +		return -EBADFD;

One question is whether we want a dedicated error code for XRUN or
such a DSP error.  On PCM, for example, we return -EPIPE traditionally
for XRUN state.  This is a clear indicator for user what to do at
next.

Other than that, the patch series looks good to me.


thanks,

Takashi