[PATCH v2 0/6] Propagate errors out from compressed streams

[PATCH v2 0/6] 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 v1:
 - Based on a recommendation from Vinod, rather than handling
   errors from pointer requests, add a new call that allows us to
   inform the compress core that the stream has gone bad. Then
   return errors from the avail and poll callbacks if the stream is
   bad.

Thanks,
Charles

Charles Keepax (6):
  ASoC: compress: Pass error out of soc_compr_pointer
  ALSA: compress: Add function to indicate the stream has gone bad
  ALSA: compress: Replace complex if statement with switch
  ASoC: wm_adsp: Factor out fetching of stream errors from the DSP
  ASoC: wm_adsp: Improve DSP error handling
  ASoC: wm_adsp: Use new snd_compr_stop_xrun to signal stream failure

 include/sound/compress_driver.h |  3 ++
 sound/core/compress_offload.c   | 67 ++++++++++++++++++++++++++++++++++++++---
 sound/soc/codecs/wm_adsp.c      | 43 +++++++++++++++++++-------
 sound/soc/soc-compress.c        |  5 +--
 4 files changed, 101 insertions(+), 17 deletions(-)

-- 
2.1.4

[PATCH v2 1/6] 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

[PATCH v2 2/6] 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_xrun 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 |  3 +++
 sound/core/compress_offload.c   | 58 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index c0abcdc..968fe2e 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 xrun_work;
 	enum snd_compr_direction direction;
 	bool metadata_set;
 	bool next_track;
@@ -187,4 +188,6 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+int snd_compr_stop_xrun(struct snd_compr_stream *stream);
+
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index a9933c0..0b93121 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 xrun_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.xrun_work, xrun_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.xrun_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;
@@ -397,10 +412,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);
@@ -420,6 +441,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;
@@ -698,6 +722,38 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	return retval;
 }
 
+static void xrun_delayed_work(struct work_struct *work)
+{
+	struct snd_compr_stream *stream;
+
+	stream = container_of(work, struct snd_compr_stream, xrun_work.work);
+
+	mutex_lock(&stream->device->lock);
+	snd_compr_stop(stream);
+	mutex_unlock(&stream->device->lock);
+}
+
+/*
+ * snd_compr_stop_xrun: Report a fatal error on a stream
+ * @stream: pointer to stream
+ *
+ * Stop the stream and set its state to XRUN.
+ *
+ * Should be called with compressed device lock held.
+ */
+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);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_compr_stop_xrun);
+
 static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 {
 	int ret;
-- 
2.1.4

[PATCH v2 3/6] 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 0b93121..65d94d6 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -303,9 +303,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 v2 4/6] ASoC: wm_adsp: Factor out fetching of stream errors from the DSP

[Charles Keepax]

Factor out the reading of the DSP error flag into its own function to
support further improvements to the code.

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

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 953c427..f70c609 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2816,6 +2816,23 @@ static int wm_adsp_buffer_update_avail(struct wm_adsp_compr_buf *buf)
 	return 0;
 }
 
+static int wm_adsp_buffer_get_error(struct wm_adsp_compr_buf *buf)
+{
+	int ret;
+
+	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(error), &buf->error);
+	if (ret < 0) {
+		adsp_err(buf->dsp, "Failed to check buffer error: %d\n", ret);
+		return ret;
+	}
+	if (buf->error != 0) {
+		adsp_err(buf->dsp, "Buffer error occurred: %d\n", buf->error);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 {
 	struct wm_adsp_compr_buf *buf;
@@ -2834,16 +2851,9 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 
 	adsp_dbg(dsp, "Handling buffer IRQ\n");
 
-	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(error), &buf->error);
-	if (ret < 0) {
-		adsp_err(dsp, "Failed to check buffer error: %d\n", ret);
-		goto out;
-	}
-	if (buf->error != 0) {
-		adsp_err(dsp, "Buffer error occurred: %d\n", buf->error);
-		ret = -EIO;
+	ret = wm_adsp_buffer_get_error(buf);
+	if (ret < 0)
 		goto out;
-	}
 
 	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(irq_count),
 				  &buf->irq_count);
-- 
2.1.4

[PATCH v2 5/6] ASoC: wm_adsp: Improve DSP error handling

[Charles Keepax]

If we encounter an error on the DSP side whilst user-space is
waiting on the poll we should call snd_compr_fragment_elapsed,
although data is not actually available we want to wake
user-space such that the error can be propagated out
quickly. Additionally some versions of the DSP firmware are
not super consistent about actually generating an IRQ if they
encounter an error, as such we will check the DSP error status
every time we run out of available data as well, to ensure we
catch it.

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

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index f70c609..3ac2e1f 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2853,7 +2853,7 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 
 	ret = wm_adsp_buffer_get_error(buf);
 	if (ret < 0)
-		goto out;
+		goto out_notify; /* Wake poll to report error */
 
 	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(irq_count),
 				  &buf->irq_count);
@@ -2868,6 +2868,7 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 		goto out;
 	}
 
+out_notify:
 	if (compr && compr->stream)
 		snd_compr_fragment_elapsed(compr->stream);
 
@@ -2928,6 +2929,10 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 		 * DSP to inform us once a whole fragment is available.
 		 */
 		if (buf->avail < wm_adsp_compr_frag_words(compr)) {
+			ret = wm_adsp_buffer_get_error(buf);
+			if (ret < 0)
+				goto out;
+
 			ret = wm_adsp_buffer_reenable_irq(buf);
 			if (ret < 0) {
 				adsp_err(dsp,
-- 
2.1.4

[PATCH v2 6/6] ASoC: wm_adsp: Use new snd_compr_stop_xrun to signal stream failure

[Charles Keepax]

If we encounter a fatal error on the compressed stream call the new
snd_compr_stop_xrun 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 3ac2e1f..4e06776 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_xrun(stream);
 		ret = -EIO;
 		goto out;
 	}
@@ -2930,8 +2931,11 @@ 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_xrun(stream);
 				goto out;
+			}
 
 			ret = wm_adsp_buffer_reenable_irq(buf);
 			if (ret < 0) {
@@ -3029,8 +3033,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_xrun(compr->stream);
 		return -EIO;
+	}
 
 	count /= WM_ADSP_DATA_WORD_SIZE;
 
-- 
2.1.4

Applied "ASoC: wm_adsp: Improve DSP error handling" to the asoc tree

[Mark Brown]

The patch

   ASoC: wm_adsp: Improve DSP error handling

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 5847609edb3c80be07e897e449a9bb579a0fe9d8 Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date: Wed, 6 Apr 2016 11:21:54 +0100
Subject: [PATCH] ASoC: wm_adsp: Improve DSP error handling

If we encounter an error on the DSP side whilst user-space is
waiting on the poll we should call snd_compr_fragment_elapsed,
although data is not actually available we want to wake
user-space such that the error can be propagated out
quickly. Additionally some versions of the DSP firmware are
not super consistent about actually generating an IRQ if they
encounter an error, as such we will check the DSP error status
every time we run out of available data as well, to ensure we
catch it.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/wm_adsp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index f70c60914042..3ac2e1f06ad3 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2853,7 +2853,7 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 
 	ret = wm_adsp_buffer_get_error(buf);
 	if (ret < 0)
-		goto out;
+		goto out_notify; /* Wake poll to report error */
 
 	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(irq_count),
 				  &buf->irq_count);
@@ -2868,6 +2868,7 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 		goto out;
 	}
 
+out_notify:
 	if (compr && compr->stream)
 		snd_compr_fragment_elapsed(compr->stream);
 
@@ -2928,6 +2929,10 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 		 * DSP to inform us once a whole fragment is available.
 		 */
 		if (buf->avail < wm_adsp_compr_frag_words(compr)) {
+			ret = wm_adsp_buffer_get_error(buf);
+			if (ret < 0)
+				goto out;
+
 			ret = wm_adsp_buffer_reenable_irq(buf);
 			if (ret < 0) {
 				adsp_err(dsp,
-- 
2.8.0.rc3

Applied "ASoC: wm_adsp: Factor out fetching of stream errors from the DSP" to the asoc tree

[Mark Brown]

The patch

   ASoC: wm_adsp: Factor out fetching of stream errors from the DSP

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 9771b18a0b374b6e6ecfa84c8b59d5ef79e969b1 Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date: Wed, 6 Apr 2016 11:21:53 +0100
Subject: [PATCH] ASoC: wm_adsp: Factor out fetching of stream errors from the
 DSP

Factor out the reading of the DSP error flag into its own function to
support further improvements to the code.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/wm_adsp.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 953c4278b75e..f70c60914042 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2816,6 +2816,23 @@ static int wm_adsp_buffer_update_avail(struct wm_adsp_compr_buf *buf)
 	return 0;
 }
 
+static int wm_adsp_buffer_get_error(struct wm_adsp_compr_buf *buf)
+{
+	int ret;
+
+	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(error), &buf->error);
+	if (ret < 0) {
+		adsp_err(buf->dsp, "Failed to check buffer error: %d\n", ret);
+		return ret;
+	}
+	if (buf->error != 0) {
+		adsp_err(buf->dsp, "Buffer error occurred: %d\n", buf->error);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 {
 	struct wm_adsp_compr_buf *buf;
@@ -2834,16 +2851,9 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 
 	adsp_dbg(dsp, "Handling buffer IRQ\n");
 
-	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(error), &buf->error);
-	if (ret < 0) {
-		adsp_err(dsp, "Failed to check buffer error: %d\n", ret);
-		goto out;
-	}
-	if (buf->error != 0) {
-		adsp_err(dsp, "Buffer error occurred: %d\n", buf->error);
-		ret = -EIO;
+	ret = wm_adsp_buffer_get_error(buf);
+	if (ret < 0)
 		goto out;
-	}
 
 	ret = wm_adsp_buffer_read(buf, HOST_BUFFER_FIELD(irq_count),
 				  &buf->irq_count);
-- 
2.8.0.rc3

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

[Koul, Vinod]

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.

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


-- 
~Vinod

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

[Charles Keepax]

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

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

[Koul, Vinod]

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

Re: [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

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

[Charles Keepax]

On Thu, Apr 07, 2016 at 11:07:12PM +0000, Koul, Vinod 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:
> > > 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?

So if I make pointer return void, the two possible error paths
are either return zero available data or call xrun. Xrun will
stop the stream so I only want to do that if I think the stream
is dead. But that likely leaves various catagories of errors I
don't want to do that for. Returning zero on the other hand hides
the error so no one will ever know an error occurs (well except by
inspecting the kernel log). It feels to me like someone is going
to hit a case where they need to do that at some point. So
removing the return type seems wrong, but at the moment you have
two functions that return ints but don't propogate their values
though which just looks odd in the code.

Thanks,
Charles