[PATCH v3 0/4] Propagate errors out from compressed streams

[PATCH v3 0/4] Propagate errors out from compressed streams

[Charles Keepax]

If the DSP suffers an unrecoverable error, the driver likely
knows about this, however the framework may not get informed
because errors returned from pointer requests are ignored within
the framework.

Things work out fine if user-space is doing a read as reads
return error status back to user-space so the user can find out
that things have gone bad. However, if user-space is doing an
avail request there is no path for the error to come back up to
user-space. The pointer request returns zero available data, so a
read never happens and we basically just end up sitting waiting
for data on a stream that we know full well has died.

This patch set attempts to address this and ensure that errors
are fully propagated to user-space and we don't ever end up wait
for data that will never come.

Changes since v2:
 - Make naming of stop function more generic and allow it to
   specify target state

Thanks,
Charles

Charles Keepax (4):
  ALSA: compress: Replace complex if statement with switch
  ALSA: compress: Add function to indicate the stream has gone bad
  ASoC: wm_adsp: Use new snd_compr_stop_error to signal stream failure
  ASoC: compress: Pass error out of soc_compr_pointer

 include/sound/compress_driver.h |  4 +++
 sound/core/compress_offload.c   | 69 ++++++++++++++++++++++++++++++++++++++---
 sound/soc/codecs/wm_adsp.c      | 11 +++++--
 sound/soc/soc-compress.c        |  5 +--
 4 files changed, 81 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH v3 1/4] ALSA: compress: Replace complex if statement with switch

[Charles Keepax]

A switch statement looks a bit cleaner than an if statement
spread over 3 lines, as such update this to a switch.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index a9933c0..507071d 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -288,9 +288,12 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 	stream = &data->stream;
 	mutex_lock(&stream->device->lock);
 	/* write is allowed when stream is running or has been steup */
-	if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
-	    stream->runtime->state != SNDRV_PCM_STATE_PREPARED &&
-			stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_SETUP:
+	case SNDRV_PCM_STATE_PREPARED:
+	case SNDRV_PCM_STATE_RUNNING:
+		break;
+	default:
 		mutex_unlock(&stream->device->lock);
 		return -EBADFD;
 	}
-- 
2.1.4

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

[Charles Keepax]

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;
+	default:
+		break;
+	}
+
 	if (copy_to_user((__u64 __user *)arg,
 				&ioctl_avail, sizeof(ioctl_avail)))
 		return -EFAULT;
@@ -400,10 +415,16 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 		return -EFAULT;
 
 	mutex_lock(&stream->device->lock);
-	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
+
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_OPEN:
+	case SNDRV_PCM_STATE_XRUN:
 		retval = -EBADFD;
 		goto out;
+	default:
+		break;
 	}
+
 	poll_wait(f, &stream->runtime->sleep, wait);
 
 	avail = snd_compr_get_avail(stream);
@@ -423,6 +444,9 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 		if (avail >= stream->runtime->fragment_size)
 			retval = snd_compr_get_poll(stream);
 		break;
+	case SNDRV_PCM_STATE_XRUN:
+		retval = -EBADFD;
+		break;
 	default:
 		if (stream->direction == SND_COMPRESS_PLAYBACK)
 			retval = POLLOUT | POLLWRNORM | POLLERR;
@@ -701,6 +725,40 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	return retval;
 }
 
+static void error_delayed_work(struct work_struct *work)
+{
+	struct snd_compr_stream *stream;
+
+	stream = container_of(work, struct snd_compr_stream, error_work.work);
+
+	mutex_lock(&stream->device->lock);
+	snd_compr_stop(stream);
+	mutex_unlock(&stream->device->lock);
+}
+
+/*
+ * snd_compr_stop_error: Report a fatal error on a stream
+ * @stream: pointer to stream
+ * @state: state to transition the stream to
+ *
+ * Stop the stream and set its state.
+ *
+ * Should be called with compressed device lock held.
+ */
+int snd_compr_stop_error(struct snd_compr_stream *stream,
+			 snd_pcm_state_t state)
+{
+	if (stream->runtime->state == state)
+		return 0;
+
+	stream->runtime->state = state;
+
+	queue_delayed_work(system_power_efficient_wq, &stream->error_work, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_compr_stop_error);
+
 static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 {
 	int ret;
-- 
2.1.4

[PATCH v3 3/4] ASoC: wm_adsp: Use new snd_compr_stop_error to signal stream failure

[Charles Keepax]

If we encounter a fatal error on the compressed stream call the new
snd_compr_stop_error to shutdown the stream and allow the core to
inform user-space that the stream is no longer valid.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm_adsp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 3ac2e1f..d221ac9 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2913,6 +2913,7 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 	}
 
 	if (compr->buf->error) {
+		snd_compr_stop_error(stream, SNDRV_PCM_STATE_XRUN);
 		ret = -EIO;
 		goto out;
 	}
@@ -2930,8 +2931,12 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 		 */
 		if (buf->avail < wm_adsp_compr_frag_words(compr)) {
 			ret = wm_adsp_buffer_get_error(buf);
-			if (ret < 0)
+			if (ret < 0) {
+				if (compr->buf->error)
+					snd_compr_stop_error(stream,
+							SNDRV_PCM_STATE_XRUN);
 				goto out;
+			}
 
 			ret = wm_adsp_buffer_reenable_irq(buf);
 			if (ret < 0) {
@@ -3029,8 +3034,10 @@ static int wm_adsp_compr_read(struct wm_adsp_compr *compr,
 	if (!compr->buf)
 		return -ENXIO;
 
-	if (compr->buf->error)
+	if (compr->buf->error) {
+		snd_compr_stop_error(compr->stream, SNDRV_PCM_STATE_XRUN);
 		return -EIO;
+	}
 
 	count /= WM_ADSP_DATA_WORD_SIZE;
 
-- 
2.1.4

[PATCH v3 4/4] ASoC: compress: Pass error out of soc_compr_pointer

[Charles Keepax]

Both soc_compr_pointer and the platform driver pointer callback return
ints but current soc_compr_pointer always returns 0. Update this so we
return the actual value from the platform driver callback. This doesn't
fix any issues simply makes the code more consistent.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-compress.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 875733c..d2df46c 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -530,14 +530,15 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_platform *platform = rtd->platform;
+	int ret = 0;
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
 	if (platform->driver->compr_ops && platform->driver->compr_ops->pointer)
-		 platform->driver->compr_ops->pointer(cstream, tstamp);
+		ret = platform->driver->compr_ops->pointer(cstream, tstamp);
 
 	mutex_unlock(&rtd->pcm_mutex);
-	return 0;
+	return ret;
 }
 
 static int soc_compr_copy(struct snd_compr_stream *cstream,
-- 
2.1.4

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

[Takashi Iwai]

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

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

[Charles Keepax]

On Mon, Apr 11, 2016 at 04:41:23PM +0200, Takashi Iwai wrote:
> On Mon, 11 Apr 2016 16:27:33 +0200,
> Charles Keepax wrote:
> > +	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.

I think it probably makes sense to copy what the PCM framework
does here, I will respin and use EPIPE.

Thanks,
Charles

Applied "ASoC: wm_adsp: Use new snd_compr_stop_error to signal stream failure" to the asoc tree

[Mark Brown]

The patch

   ASoC: wm_adsp: Use new snd_compr_stop_error to signal stream failure

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 8d280664d26538cd37e7c08b1c2b58fe006cc482 Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date: Mon, 13 Jun 2016 14:17:11 +0100
Subject: [PATCH] ASoC: wm_adsp: Use new snd_compr_stop_error to signal stream
 failure

If we encounter a fatal error on the compressed stream call the new
snd_compr_stop_error to shutdown the stream and allow the core to
inform user-space that the stream is no longer valid.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Reviewed-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/wm_adsp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index a07bd7c2c587..8ed1cdececf2 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -3043,6 +3043,7 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 	}
 
 	if (compr->buf->error) {
+		snd_compr_stop_error(stream, SNDRV_PCM_STATE_XRUN);
 		ret = -EIO;
 		goto out;
 	}
@@ -3060,8 +3061,12 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 		 */
 		if (buf->avail < wm_adsp_compr_frag_words(compr)) {
 			ret = wm_adsp_buffer_get_error(buf);
-			if (ret < 0)
+			if (ret < 0) {
+				if (compr->buf->error)
+					snd_compr_stop_error(stream,
+							SNDRV_PCM_STATE_XRUN);
 				goto out;
+			}
 
 			ret = wm_adsp_buffer_reenable_irq(buf);
 			if (ret < 0) {
@@ -3159,8 +3164,10 @@ static int wm_adsp_compr_read(struct wm_adsp_compr *compr,
 	if (!compr->buf)
 		return -ENXIO;
 
-	if (compr->buf->error)
+	if (compr->buf->error) {
+		snd_compr_stop_error(compr->stream, SNDRV_PCM_STATE_XRUN);
 		return -EIO;
+	}
 
 	count /= WM_ADSP_DATA_WORD_SIZE;
 
-- 
2.8.1