[alsa-devel] [PATCH v4 0/4] Propagate errors out from compressed streams
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 v3: - Add missing comment for new field in snd_compr_stream - Return -EPIPE on XRUN to match what the PCM framework does
It might be easiest to put the ASoC changes through Mark's tree as there are some merge conflicts otherwise, as such I have based this series on Mark's tree.
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 | 5 +++ sound/core/compress_offload.c | 76 ++++++++++++++++++++++++++++++++++++++--- sound/soc/codecs/wm_adsp.c | 11 ++++-- sound/soc/soc-compress.c | 5 +-- 4 files changed, 88 insertions(+), 9 deletions(-)
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; }
On Tue, Apr 12, 2016 at 11:32:06AM +0100, Charles Keepax wrote:
A switch statement looks a bit cleaner than an if statement spread over 3 lines, as such update this to a switch.
Acked-by: Vinod Koul vinod.koul@intel.com
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 | 5 +++ sound/core/compress_offload.c | 67 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h index c0abcdc..cee8c00 100644 --- a/include/sound/compress_driver.h +++ b/include/sound/compress_driver.h @@ -68,6 +68,7 @@ struct snd_compr_runtime { * @ops: pointer to DSP callbacks * @runtime: pointer to runtime structure * @device: device pointer + * @error_work: delayed work used when closing the stream due to an error * @direction: stream direction, playback/recording * @metadata_set: metadata set flag, true when set * @next_track: has userspace signal next track transition, true when set @@ -78,6 +79,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 +189,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..81d267e 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,15 @@ 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: + return -EBADFD; + case SNDRV_PCM_STATE_XRUN: + return -EPIPE; + default: + break; + } + if (copy_to_user((__u64 __user *)arg, &ioctl_avail, sizeof(ioctl_avail))) return -EFAULT; @@ -346,11 +362,13 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf, switch (stream->runtime->state) { case SNDRV_PCM_STATE_OPEN: case SNDRV_PCM_STATE_PREPARED: - case SNDRV_PCM_STATE_XRUN: case SNDRV_PCM_STATE_SUSPENDED: case SNDRV_PCM_STATE_DISCONNECTED: retval = -EBADFD; goto out; + case SNDRV_PCM_STATE_XRUN: + retval = -EPIPE; + goto out; }
avail = snd_compr_get_avail(stream); @@ -400,10 +418,18 @@ 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: retval = -EBADFD; goto out; + case SNDRV_PCM_STATE_XRUN: + retval = -EPIPE; + goto out; + default: + break; } + poll_wait(f, &stream->runtime->sleep, wait);
avail = snd_compr_get_avail(stream); @@ -423,6 +449,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 = -EPIPE; + break; default: if (stream->direction == SND_COMPRESS_PLAYBACK) retval = POLLOUT | POLLWRNORM | POLLERR; @@ -701,6 +730,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;
On Tue, Apr 12, 2016 at 11:32:07AM +0100, Charles Keepax wrote:
- @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)
Do we want the state as an agument here, since we are invoking stop, it should transistion to the stopped 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);
On Tue, Apr 19, 2016 at 11:55:52AM +0530, Vinod Koul wrote:
On Tue, Apr 12, 2016 at 11:32:07AM +0100, Charles Keepax wrote:
- @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)Do we want the state as an agument here, since we are invoking stop, it should transistion to the stopped state
This was my interpretation of this comment of yours:
"Also should this be made a generic stop rather than xrun. Perhaps the reason can be specified as an argument."
If you clarify what you are looking for I am happy to respin. We should set the state to something that indicates there has been an error, such that things are distinct from a normal stop operation. Personally I liked simply calling the function snd_compr_stop_xrun and always setting it to xrun and it matches what the PCM framework does. So I am happy to go back to that if you prefer?
Thanks, Charles
On Tue, Apr 19, 2016 at 09:31:40AM +0100, Charles Keepax wrote:
On Tue, Apr 19, 2016 at 11:55:52AM +0530, Vinod Koul wrote:
On Tue, Apr 12, 2016 at 11:32:07AM +0100, Charles Keepax wrote:
- @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)Do we want the state as an agument here, since we are invoking stop, it should transistion to the stopped state
This was my interpretation of this comment of yours:
"Also should this be made a generic stop rather than xrun. Perhaps the reason can be specified as an argument."
If you clarify what you are looking for I am happy to respin. We should set the state to something that indicates there has been an error, such that things are distinct from a normal stop operation. Personally I liked simply calling the function snd_compr_stop_xrun and always setting it to xrun and it matches what the PCM framework does. So I am happy to go back to that if you prefer?
Your interpretation was right, but there is no point in setting xrun as this will be essentially overwritten when you invoke the snd_compr_stop()
It does set it back to setup state..
Thanks
On Tue, Apr 19, 2016 at 06:34:03PM +0530, Vinod Koul wrote:
On Tue, Apr 19, 2016 at 09:31:40AM +0100, Charles Keepax wrote:
On Tue, Apr 19, 2016 at 11:55:52AM +0530, Vinod Koul wrote:
On Tue, Apr 12, 2016 at 11:32:07AM +0100, Charles Keepax wrote:
- @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)Do we want the state as an agument here, since we are invoking stop, it should transistion to the stopped state
This was my interpretation of this comment of yours:
"Also should this be made a generic stop rather than xrun. Perhaps the reason can be specified as an argument."
If you clarify what you are looking for I am happy to respin. We should set the state to something that indicates there has been an error, such that things are distinct from a normal stop operation. Personally I liked simply calling the function snd_compr_stop_xrun and always setting it to xrun and it matches what the PCM framework does. So I am happy to go back to that if you prefer?
Your interpretation was right, but there is no point in setting xrun as this will be essentially overwritten when you invoke the snd_compr_stop()
It does set it back to setup state..
Ah oops thanks missed that one, I will have a look at fixing that and doing another spin.
Thanks, Charles
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;
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,
On Tue, Apr 12, 2016 at 11:32:09AM +0100, Charles Keepax wrote:
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.
Acked-by: Vinod Koul vinod.koul@intel.com
participants (2)
-
Charles Keepax -
Vinod Koul