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

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

[Charles Keepax]

The soc_compr_pointer does not correctly pass any errors returned by the
driver callback back up the stack. This patch corrects this issue.

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 2/4] ALSA: compress: Handle errors during avail requests

[Charles Keepax]

No errors are currently returned whilst checking the amount of available
data. If user-space does an avail request which encounters an error it
will generally return zero, causing user-space to poll and if each
subsequence avail request also returns zero it will wait indefinitely for
data that is never coming. This patch updates the code so that errors
during avail requests will be correctly propagated to user-space.

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

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 7fac3ca..c9c0849 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -170,9 +170,15 @@ static int snd_compr_free(struct inode *inode, struct file *f)
 static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
 		struct snd_compr_tstamp *tstamp)
 {
+	int ret;
+
 	if (!stream->ops->pointer)
 		return -ENOTSUPP;
-	stream->ops->pointer(stream, tstamp);
+
+	ret = stream->ops->pointer(stream, tstamp);
+	if (ret < 0)
+		return ret;
+
 	pr_debug("dsp consumed till %d total %d bytes\n",
 		tstamp->byte_offset, tstamp->copied_total);
 	if (stream->direction == SND_COMPRESS_PLAYBACK)
@@ -182,18 +188,24 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
 	return 0;
 }
 
-static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
+static int snd_compr_calc_avail(struct snd_compr_stream *stream,
 		struct snd_compr_avail *avail)
 {
+	int ret;
+
 	memset(avail, 0, sizeof(*avail));
-	snd_compr_update_tstamp(stream, &avail->tstamp);
+	ret = snd_compr_update_tstamp(stream, &avail->tstamp);
+	if (ret < 0)
+		return ret;
+
 	/* Still need to return avail even if tstamp can't be filled in */
 
 	if (stream->runtime->total_bytes_available == 0 &&
 			stream->runtime->state == SNDRV_PCM_STATE_SETUP &&
 			stream->direction == SND_COMPRESS_PLAYBACK) {
 		pr_debug("detected init and someone forgot to do a write\n");
-		return stream->runtime->buffer_size;
+		avail->avail = stream->runtime->buffer_size;
+		return 0;
 	}
 	pr_debug("app wrote %lld, DSP consumed %lld\n",
 			stream->runtime->total_bytes_available,
@@ -202,9 +214,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 				stream->runtime->total_bytes_transferred) {
 		if (stream->direction == SND_COMPRESS_PLAYBACK) {
 			pr_debug("both pointers are same, returning full avail\n");
-			return stream->runtime->buffer_size;
+			avail->avail = stream->runtime->buffer_size;
+			return 0;
 		} else {
 			pr_debug("both pointers are same, returning no avail\n");
+			avail->avail = 0;
 			return 0;
 		}
 	}
@@ -215,24 +229,30 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 		avail->avail = stream->runtime->buffer_size - avail->avail;
 
 	pr_debug("ret avail as %lld\n", avail->avail);
-	return avail->avail;
+	return 0;
 }
 
-static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
+static inline int snd_compr_get_avail(struct snd_compr_stream *stream,
+				      size_t *avail)
 {
-	struct snd_compr_avail avail;
+	struct snd_compr_avail full_avail;
+	int ret;
+
+	ret = snd_compr_calc_avail(stream, &full_avail);
+	*avail = full_avail.avail;
 
-	return snd_compr_calc_avail(stream, &avail);
+	return ret;
 }
 
 static int
 snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg)
 {
 	struct snd_compr_avail ioctl_avail;
-	size_t avail;
+	int ret;
 
-	avail = snd_compr_calc_avail(stream, &ioctl_avail);
-	ioctl_avail.avail = avail;
+	ret = snd_compr_calc_avail(stream, &ioctl_avail);
+	if (ret < 0)
+		return ret;
 
 	if (copy_to_user((__u64 __user *)arg,
 				&ioctl_avail, sizeof(ioctl_avail)))
@@ -287,11 +307,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 	/* 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_RUNNING) {
-		mutex_unlock(&stream->device->lock);
-		return -EBADFD;
+		retval = -EBADFD;
+		goto out;
 	}
 
-	avail = snd_compr_get_avail(stream);
+	retval = snd_compr_get_avail(stream, &avail);
+	if (retval < 0)
+		goto out;
+
 	pr_debug("avail returned %ld\n", (unsigned long)avail);
 	/* calculate how much we can write to buffer */
 	if (avail > count)
@@ -313,6 +336,7 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 		pr_debug("stream prepared, Houston we are good to go\n");
 	}
 
+out:
 	mutex_unlock(&stream->device->lock);
 	return retval;
 }
@@ -346,7 +370,10 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
 		goto out;
 	}
 
-	avail = snd_compr_get_avail(stream);
+	retval = snd_compr_get_avail(stream, &avail);
+	if (retval < 0)
+		goto out;
+
 	pr_debug("avail returned %ld\n", (unsigned long)avail);
 	/* calculate how much we can read from buffer */
 	if (avail > count)
@@ -399,7 +426,10 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 	}
 	poll_wait(f, &stream->runtime->sleep, wait);
 
-	avail = snd_compr_get_avail(stream);
+	retval = snd_compr_get_avail(stream, &avail);
+	if (retval < 0)
+		goto out;
+
 	pr_debug("avail is %ld\n", (unsigned long)avail);
 	/* check if we have at least one fragment to fill */
 	switch (stream->runtime->state) {
-- 
2.1.4

[PATCH 3/4] ASoC: wm_adsp: Factor out checking 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 e2e0d81..f28c244 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2812,6 +2812,23 @@ static int wm_adsp_buffer_update_avail(struct wm_adsp_compr_buf *buf)
 	return 0;
 }
 
+static int wm_adsp_buffer_check_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 = dsp->buffer;
@@ -2827,16 +2844,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_check_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 4/4] 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index f28c244..3ee1c72 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -2861,10 +2861,10 @@ int wm_adsp_compr_handle_irq(struct wm_adsp *dsp)
 		goto out;
 	}
 
+out:
 	if (compr && compr->stream)
 		snd_compr_fragment_elapsed(compr->stream);
 
-out:
 	mutex_unlock(&dsp->pwr_lock);
 
 	return ret;
@@ -2926,6 +2926,10 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 					 ret);
 				goto out;
 			}
+
+			ret = wm_adsp_buffer_check_error(buf);
+			if (ret < 0)
+				goto out;
 		}
 	}
 
-- 
2.1.4

Re: [PATCH 4/4] ASoC: wm_adsp: Improve DSP error handling

[Charles Keepax]

On Thu, Mar 10, 2016 at 10:44:54AM +0000, Charles Keepax wrote:
> 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>
> ---

Apologies please drop this last change I need to do a respin.
This will ping the poll for spurious interrupts which is not very
desirable. I will fix that and send a rev 2 but the rest of the
chain is good to merge as is.

Thanks,
Charles

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

[Vinod Koul]

On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> The soc_compr_pointer does not correctly pass any errors returned by the
> driver callback back up the stack. This patch corrects this issue.

Should we do that :) I am not too sure. Pointer query is supposed to read
the current value and return. You are trying to indicate that stream has
gone bad which is not the same as read faced an error...

Also please use cover letter for these things to describe problem you are
trying to solve.

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

-- 
~Vinod

Re: [PATCH 2/4] ALSA: compress: Handle errors during avail requests

[Vinod Koul]

On Thu, Mar 10, 2016 at 10:44:52AM +0000, Charles Keepax wrote:
> No errors are currently returned whilst checking the amount of available
> data. If user-space does an avail request which encounters an error it

How do you detect an error? Is DSP sending you error or just fails to
respond? ALso note that in compress formats, DSP may still be decoding the
data and rendering while we thing pointers are not moving

Also while at it, I am thinking exporting snd_compr_stop() might be a better
idea, if you can reliably detect error then stop the stream...

> will generally return zero, causing user-space to poll and if each
> subsequence avail request also returns zero it will wait indefinitely for
> data that is never coming. This patch updates the code so that errors
> during avail requests will be correctly propagated to user-space.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  sound/core/compress_offload.c | 64 +++++++++++++++++++++++++++++++------------
>  1 file changed, 47 insertions(+), 17 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 7fac3ca..c9c0849 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -170,9 +170,15 @@ static int snd_compr_free(struct inode *inode, struct file *f)
>  static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
>  		struct snd_compr_tstamp *tstamp)
>  {
> +	int ret;
> +
>  	if (!stream->ops->pointer)
>  		return -ENOTSUPP;
> -	stream->ops->pointer(stream, tstamp);
> +
> +	ret = stream->ops->pointer(stream, tstamp);
> +	if (ret < 0)
> +		return ret;
> +
>  	pr_debug("dsp consumed till %d total %d bytes\n",
>  		tstamp->byte_offset, tstamp->copied_total);
>  	if (stream->direction == SND_COMPRESS_PLAYBACK)
> @@ -182,18 +188,24 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
>  	return 0;
>  }
>  
> -static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
> +static int snd_compr_calc_avail(struct snd_compr_stream *stream,
>  		struct snd_compr_avail *avail)
>  {
> +	int ret;
> +
>  	memset(avail, 0, sizeof(*avail));
> -	snd_compr_update_tstamp(stream, &avail->tstamp);
> +	ret = snd_compr_update_tstamp(stream, &avail->tstamp);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Still need to return avail even if tstamp can't be filled in */
>  
>  	if (stream->runtime->total_bytes_available == 0 &&
>  			stream->runtime->state == SNDRV_PCM_STATE_SETUP &&
>  			stream->direction == SND_COMPRESS_PLAYBACK) {
>  		pr_debug("detected init and someone forgot to do a write\n");
> -		return stream->runtime->buffer_size;
> +		avail->avail = stream->runtime->buffer_size;
> +		return 0;
>  	}
>  	pr_debug("app wrote %lld, DSP consumed %lld\n",
>  			stream->runtime->total_bytes_available,
> @@ -202,9 +214,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
>  				stream->runtime->total_bytes_transferred) {
>  		if (stream->direction == SND_COMPRESS_PLAYBACK) {
>  			pr_debug("both pointers are same, returning full avail\n");
> -			return stream->runtime->buffer_size;
> +			avail->avail = stream->runtime->buffer_size;
> +			return 0;
>  		} else {
>  			pr_debug("both pointers are same, returning no avail\n");
> +			avail->avail = 0;
>  			return 0;
>  		}
>  	}
> @@ -215,24 +229,30 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
>  		avail->avail = stream->runtime->buffer_size - avail->avail;
>  
>  	pr_debug("ret avail as %lld\n", avail->avail);
> -	return avail->avail;
> +	return 0;
>  }
>  
> -static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
> +static inline int snd_compr_get_avail(struct snd_compr_stream *stream,
> +				      size_t *avail)
>  {
> -	struct snd_compr_avail avail;
> +	struct snd_compr_avail full_avail;
> +	int ret;
> +
> +	ret = snd_compr_calc_avail(stream, &full_avail);
> +	*avail = full_avail.avail;
>  
> -	return snd_compr_calc_avail(stream, &avail);
> +	return ret;
>  }
>  
>  static int
>  snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg)
>  {
>  	struct snd_compr_avail ioctl_avail;
> -	size_t avail;
> +	int ret;
>  
> -	avail = snd_compr_calc_avail(stream, &ioctl_avail);
> -	ioctl_avail.avail = avail;
> +	ret = snd_compr_calc_avail(stream, &ioctl_avail);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (copy_to_user((__u64 __user *)arg,
>  				&ioctl_avail, sizeof(ioctl_avail)))
> @@ -287,11 +307,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
>  	/* 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_RUNNING) {
> -		mutex_unlock(&stream->device->lock);
> -		return -EBADFD;
> +		retval = -EBADFD;
> +		goto out;
>  	}
>  
> -	avail = snd_compr_get_avail(stream);
> +	retval = snd_compr_get_avail(stream, &avail);
> +	if (retval < 0)
> +		goto out;
> +
>  	pr_debug("avail returned %ld\n", (unsigned long)avail);
>  	/* calculate how much we can write to buffer */
>  	if (avail > count)
> @@ -313,6 +336,7 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
>  		pr_debug("stream prepared, Houston we are good to go\n");
>  	}
>  
> +out:
>  	mutex_unlock(&stream->device->lock);
>  	return retval;
>  }
> @@ -346,7 +370,10 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
>  		goto out;
>  	}
>  
> -	avail = snd_compr_get_avail(stream);
> +	retval = snd_compr_get_avail(stream, &avail);
> +	if (retval < 0)
> +		goto out;
> +
>  	pr_debug("avail returned %ld\n", (unsigned long)avail);
>  	/* calculate how much we can read from buffer */
>  	if (avail > count)
> @@ -399,7 +426,10 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
>  	}
>  	poll_wait(f, &stream->runtime->sleep, wait);
>  
> -	avail = snd_compr_get_avail(stream);
> +	retval = snd_compr_get_avail(stream, &avail);
> +	if (retval < 0)
> +		goto out;
> +
>  	pr_debug("avail is %ld\n", (unsigned long)avail);
>  	/* check if we have at least one fragment to fill */
>  	switch (stream->runtime->state) {
> -- 
> 2.1.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

-- 
~Vinod

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

[Charles Keepax]

On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > The soc_compr_pointer does not correctly pass any errors returned by the
> > driver callback back up the stack. This patch corrects this issue.
> 
> Should we do that :) I am not too sure. Pointer query is supposed to read
> the current value and return. You are trying to indicate that stream has
> gone bad which is not the same as read faced an error...
> 
> Also please use cover letter for these things to describe problem you are
> trying to solve.

Apologies for not doing so, I had been viewing this as more of a
simple oversight in the framework rather than a design choice.

The problem I am looking at is the DSP suffers an unrecoverable
error. We can find out about this error in our driver because the
DSP returns some error status to us.  This is 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.

I will have a look at other options for propagating this error,
I guess it should also be possible to push it through the poll so
I can look at why that isn't happening.

I guess my return question would be I can imagine many reasons
why a pointer query might fail, especially for off chip DSPs.
What is the reasoning for wanting to hide those errors from the
rest of the system? It seems to me it would be best to handle an
error as soon as it is noticed, and if a particular system has a
pointer request that never fails then it can just not return an
error.

Thanks,
Charles

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

[Takashi Iwai]

On Fri, 11 Mar 2016 11:04:25 +0100,
Charles Keepax wrote:
> 
> On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > > The soc_compr_pointer does not correctly pass any errors returned by the
> > > driver callback back up the stack. This patch corrects this issue.
> > 
> > Should we do that :) I am not too sure. Pointer query is supposed to read
> > the current value and return. You are trying to indicate that stream has
> > gone bad which is not the same as read faced an error...
> > 
> > Also please use cover letter for these things to describe problem you are
> > trying to solve.
> 
> Apologies for not doing so, I had been viewing this as more of a
> simple oversight in the framework rather than a design choice.
> 
> The problem I am looking at is the DSP suffers an unrecoverable
> error. We can find out about this error in our driver because the
> DSP returns some error status to us.  This is 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.
> 
> I will have a look at other options for propagating this error,
> I guess it should also be possible to push it through the poll so
> I can look at why that isn't happening.
>
> I guess my return question would be I can imagine many reasons
> why a pointer query might fail, especially for off chip DSPs.
> What is the reasoning for wanting to hide those errors from the
> rest of the system? It seems to me it would be best to handle an
> error as soon as it is noticed, and if a particular system has a
> pointer request that never fails then it can just not return an
> error.

IMO, propagating the error immediately is a good thing.  I guess it
wasn't checked in the pointer callback just because the pointer
callback was supposed to be a simple state copy without involving the
state change.

OTOH, another question is whether it's enough just to tell the error
there as is.  When such an error is detected, it essentially means
that the whole DSP got wrong.  What we can do at best is to prepare
for recovery.  And this requires the state change.

Actually, PCM pointer callback may return a special value indicating
an XRUN error.  The PCM core reacts for it, stops the stream and
changes the stream state, so that further accesses get the error
consistently.  Similar mechanism would be needed for compress API, I
suppose.


Takashi

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

[Vinod Koul]

On Fri, Mar 11, 2016 at 10:04:25AM +0000, Charles Keepax wrote:
> On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > > The soc_compr_pointer does not correctly pass any errors returned by the
> > > driver callback back up the stack. This patch corrects this issue.
> > 
> > Should we do that :) I am not too sure. Pointer query is supposed to read
> > the current value and return. You are trying to indicate that stream has
> > gone bad which is not the same as read faced an error...
> > 
> > Also please use cover letter for these things to describe problem you are
> > trying to solve.
> 
> Apologies for not doing so, I had been viewing this as more of a
> simple oversight in the framework rather than a design choice.
> 
> The problem I am looking at is the DSP suffers an unrecoverable
> error. We can find out about this error in our driver because the
> DSP returns some error status to us.  This is 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.

So this confirms my hunch and we should then notify core of error by stopping
the stream properly and then return error on poll/pointer query.

So cna try this untested patch, whcih includes a hack for stopped state. We
don't seem to have a stopped state in ALSA, that bit would need refinement

-- >8 --

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index c0abcdc11470..a42c64248ad7 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -187,4 +187,5 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+int snd_compr_stop(struct snd_compr_stream *stream);
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 18b8dc45bb8f..5b451a3af1a3 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -218,12 +218,22 @@ static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
 	return snd_compr_calc_avail(stream, &avail);
 }
 
+#define SNDRV_PCM_STATE_STOP 8
+
 static int
 snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg)
 {
 	struct snd_compr_avail ioctl_avail;
 	size_t avail;
 
+	mutex_lock(&stream->device->lock);
+	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN ||
+			stream->runtime->state == SNDRV_PCM_STATE_STOP) {
+		mutex_unlock(&stream->device->lock);
+		return -EBADFD;
+	}
+	mutex_unlock(&stream->device->lock);
+
 	avail = snd_compr_calc_avail(stream, &ioctl_avail);
 	ioctl_avail.avail = avail;
 
@@ -386,7 +396,8 @@ 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) {
+	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN ||
+			stream->runtime->state == SNDRV_PCM_STATE_STOP) {
 		retval = -EBADFD;
 		goto out;
 	}
@@ -669,7 +680,7 @@ static int snd_compr_start(struct snd_compr_stream *stream)
 	return retval;
 }
 
-static int snd_compr_stop(struct snd_compr_stream *stream)
+int snd_compr_stop(struct snd_compr_stream *stream)
 {
 	int retval;
 
@@ -684,6 +695,7 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	}
 	return retval;
 }
+EXPORT_SYMBOL_GPL(snd_compr_stop);
 
 static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 {


-- 
~Vinod

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

[Takashi Iwai]

On Fri, 11 Mar 2016 11:37:47 +0100,
Vinod Koul wrote:
> 
> On Fri, Mar 11, 2016 at 10:04:25AM +0000, Charles Keepax wrote:
> > On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> > > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > > > The soc_compr_pointer does not correctly pass any errors returned by the
> > > > driver callback back up the stack. This patch corrects this issue.
> > > 
> > > Should we do that :) I am not too sure. Pointer query is supposed to read
> > > the current value and return. You are trying to indicate that stream has
> > > gone bad which is not the same as read faced an error...
> > > 
> > > Also please use cover letter for these things to describe problem you are
> > > trying to solve.
> > 
> > Apologies for not doing so, I had been viewing this as more of a
> > simple oversight in the framework rather than a design choice.
> > 
> > The problem I am looking at is the DSP suffers an unrecoverable
> > error. We can find out about this error in our driver because the
> > DSP returns some error status to us.  This is 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.
> 
> So this confirms my hunch and we should then notify core of error by stopping
> the stream properly and then return error on poll/pointer query.
> 
> So cna try this untested patch, whcih includes a hack for stopped state. We
> don't seem to have a stopped state in ALSA, that bit would need refinement

In PCM, the stopped state is either SETUP/PREPARE or XRUN.


Takashi

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

[Vinod Koul]

On Fri, Mar 11, 2016 at 11:25:22AM +0100, Takashi Iwai wrote:
> > I guess my return question would be I can imagine many reasons
> > why a pointer query might fail, especially for off chip DSPs.
> > What is the reasoning for wanting to hide those errors from the
> > rest of the system? It seems to me it would be best to handle an
> > error as soon as it is noticed, and if a particular system has a
> > pointer request that never fails then it can just not return an
> > error.
> 
> IMO, propagating the error immediately is a good thing.  I guess it
> wasn't checked in the pointer callback just because the pointer
> callback was supposed to be a simple state copy without involving the
> state change.
> 
> OTOH, another question is whether it's enough just to tell the error
> there as is.  When such an error is detected, it essentially means
> that the whole DSP got wrong.  What we can do at best is to prepare
> for recovery.  And this requires the state change.

Yes, for recovery on our DSP we do inoke snd_pcm_stop() for all the open
streams and then get these restarted. I think we need to do same for
compress too..

> Actually, PCM pointer callback may return a special value indicating
> an XRUN error.  The PCM core reacts for it, stops the stream and
> changes the stream state, so that further accesses get the error
> consistently.  Similar mechanism would be needed for compress API, I
> suppose.

I didnt know that :)

-- 
~Vinod

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

[Charles Keepax]

On Fri, Mar 11, 2016 at 11:39:11AM +0100, Takashi Iwai wrote:
> On Fri, 11 Mar 2016 11:37:47 +0100,
> Vinod Koul wrote:
> > 
> > On Fri, Mar 11, 2016 at 10:04:25AM +0000, Charles Keepax wrote:
> > > On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> > > > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > > > > The soc_compr_pointer does not correctly pass any errors returned by the
> > > > > driver callback back up the stack. This patch corrects this issue.
> > > > 
> > > > Should we do that :) I am not too sure. Pointer query is supposed to read
> > > > the current value and return. You are trying to indicate that stream has
> > > > gone bad which is not the same as read faced an error...
> > > > 
> > > > Also please use cover letter for these things to describe problem you are
> > > > trying to solve.
> > > 
> > > Apologies for not doing so, I had been viewing this as more of a
> > > simple oversight in the framework rather than a design choice.
> > > 
> > > The problem I am looking at is the DSP suffers an unrecoverable
> > > error. We can find out about this error in our driver because the
> > > DSP returns some error status to us.  This is 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.
> > 
> > So this confirms my hunch and we should then notify core of error by stopping
> > the stream properly and then return error on poll/pointer query.
> > 
> > So cna try this untested patch, whcih includes a hack for stopped state. We
> > don't seem to have a stopped state in ALSA, that bit would need refinement
> 
> In PCM, the stopped state is either SETUP/PREPARE or XRUN.
> 
> 
> Takashi

Thanks guys, I will take a look at all this and look at
respinning the series.

Thanks,
Charles

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

[Vinod Koul]

On Fri, Mar 11, 2016 at 11:39:11AM +0100, Takashi Iwai wrote:
> On Fri, 11 Mar 2016 11:37:47 +0100,
> Vinod Koul wrote:
> > 
> > On Fri, Mar 11, 2016 at 10:04:25AM +0000, Charles Keepax wrote:
> > > On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> > > > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > > > > The soc_compr_pointer does not correctly pass any errors returned by the
> > > > > driver callback back up the stack. This patch corrects this issue.
> > > > 
> > > > Should we do that :) I am not too sure. Pointer query is supposed to read
> > > > the current value and return. You are trying to indicate that stream has
> > > > gone bad which is not the same as read faced an error...
> > > > 
> > > > Also please use cover letter for these things to describe problem you are
> > > > trying to solve.
> > > 
> > > Apologies for not doing so, I had been viewing this as more of a
> > > simple oversight in the framework rather than a design choice.
> > > 
> > > The problem I am looking at is the DSP suffers an unrecoverable
> > > error. We can find out about this error in our driver because the
> > > DSP returns some error status to us.  This is 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.
> > 
> > So this confirms my hunch and we should then notify core of error by stopping
> > the stream properly and then return error on poll/pointer query.
> > 
> > So cna try this untested patch, whcih includes a hack for stopped state. We
> > don't seem to have a stopped state in ALSA, that bit would need refinement
> 
> In PCM, the stopped state is either SETUP/PREPARE or XRUN.

We didn't use PREPARE so we can set to XRUN. So here is alternate patch for
this, whcih looks much better :)

-- >8 --

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index c0abcdc11470..a42c64248ad7 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -187,4 +187,5 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
 	wake_up(&stream->runtime->sleep);
 }
 
+int snd_compr_stop(struct snd_compr_stream *stream);
 #endif
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 18b8dc45bb8f..5d2a4d30eb6e 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -224,6 +224,14 @@ snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg)
 	struct snd_compr_avail ioctl_avail;
 	size_t avail;
 
+	mutex_lock(&stream->device->lock);
+	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN ||
+			stream->runtime->state == SNDRV_PCM_STATE_XRUN) {
+		mutex_unlock(&stream->device->lock);
+		return -EBADFD;
+	}
+	mutex_unlock(&stream->device->lock);
+
 	avail = snd_compr_calc_avail(stream, &ioctl_avail);
 	ioctl_avail.avail = avail;
 
@@ -386,7 +394,8 @@ 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) {
+	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN ||
+			stream->runtime->state == SNDRV_PCM_STATE_XRUN) {
 		retval = -EBADFD;
 		goto out;
 	}
@@ -669,7 +678,7 @@ static int snd_compr_start(struct snd_compr_stream *stream)
 	return retval;
 }
 
-static int snd_compr_stop(struct snd_compr_stream *stream)
+static int _snd_compr_stop(struct snd_compr_stream *stream)
 {
 	int retval;
 
@@ -685,6 +694,20 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	return retval;
 }
 
+int snd_compr_stop(struct snd_compr_stream *stream)
+{
+	int ret = _snd_compr_stop(stream);
+
+	if (ret) {
+		mutex_lock(&stream->device->lock);
+		stream->runtime->state = SNDRV_PCM_STATE_XRUN;
+		mutex_unlock(&stream->device->lock);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_compr_stop);
+
 static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 {
 	int ret;
@@ -832,7 +855,7 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 		retval = snd_compr_start(stream);
 		break;
 	case _IOC_NR(SNDRV_COMPRESS_STOP):
-		retval = snd_compr_stop(stream);
+		retval = _snd_compr_stop(stream);
 		break;
 	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
 		retval = snd_compr_drain(stream);


-- 
~Vinod

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

[Charles Keepax]

On Fri, Mar 11, 2016 at 04:44:28PM +0530, Vinod Koul wrote:
> On Fri, Mar 11, 2016 at 11:39:11AM +0100, Takashi Iwai wrote:
> > On Fri, 11 Mar 2016 11:37:47 +0100,
> > Vinod Koul wrote:
> > > 
> > > On Fri, Mar 11, 2016 at 10:04:25AM +0000, Charles Keepax wrote:
> > > > On Fri, Mar 11, 2016 at 01:18:43PM +0530, Vinod Koul wrote:
> > > > > On Thu, Mar 10, 2016 at 10:44:51AM +0000, Charles Keepax wrote:
> > > > > > The soc_compr_pointer does not correctly pass any errors returned by the
> > > > > > driver callback back up the stack. This patch corrects this issue.
> > > > > 
> > > > > Should we do that :) I am not too sure. Pointer query is supposed to read
> > > > > the current value and return. You are trying to indicate that stream has
> > > > > gone bad which is not the same as read faced an error...
> > > > > 
> > > > > Also please use cover letter for these things to describe problem you are
> > > > > trying to solve.
> > > > 
> > > > Apologies for not doing so, I had been viewing this as more of a
> > > > simple oversight in the framework rather than a design choice.
> > > > 
> > > > The problem I am looking at is the DSP suffers an unrecoverable
> > > > error. We can find out about this error in our driver because the
> > > > DSP returns some error status to us.  This is 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.
> > > 
> > > So this confirms my hunch and we should then notify core of error by stopping
> > > the stream properly and then return error on poll/pointer query.
> > > 
> > > So cna try this untested patch, whcih includes a hack for stopped state. We
> > > don't seem to have a stopped state in ALSA, that bit would need refinement
> > 
> > In PCM, the stopped state is either SETUP/PREPARE or XRUN.
> 
> We didn't use PREPARE so we can set to XRUN. So here is alternate patch for
> this, whcih looks much better :)

Sorry about the delay in responding here. Are we really
completely opposed to the idea of passing error state through
the AVAIL? Basically I see there as being two problems with the
approach being suggested here.

1) It only covers errors the DSP can detect. For example if
the SPI transactions start failing whilst we are doing avail
callbacks we get into the same situation. Although we could call
this snd_compr_stop on any error from within our driver, but then
how was that better than just passing the errors?

2) The locking is exceptionally awkward, the trouble is you are
calling right from the bottom of the stack upto the top, and
then going back down again. So for example we have a lock in
our driver that syncs between the IRQ, the DSP powering up/down
through DAPM, and the compressed callbacks. We will want to call
snd_compr_stop from within the IRQ handler as the DSP should give
us an IRQ on an error. But then to ensure that the stream we are
going to pass to snd_compr_stop is valid we need to hold that
lock whilst we make the call. But this leads to a potential mutex
inversion. As we can do driver lock, stream->device_lock on the
IRQ path and stream->device->lock, driver lock on all the normal
callback paths. Not to mention the fact that snd_compr_stop is
then going to want to call trigger which will give us a double
lock on the driver lock.

How about allowing errors to propogate through the pointer
callback but then shutting down the stream from the core code
instead? We could perhaps say that a pointer call failing is
indicative of a fatal error?

Thanks,
Charles

Applied "ASoC: compress: Pass error out of soc_compr_pointer" to the asoc tree

[Mark Brown]

The patch

   ASoC: compress: Pass error out of soc_compr_pointer

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 7c9190f7e7428487cd67839f9a547efcc9ec3b9b Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date: Mon, 20 Jun 2016 09:51:32 +0100
Subject: [PATCH] ASoC: compress: Pass error out of soc_compr_pointer

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>
Acked-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 875733c52953..d2df46c14c68 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.8.1