[alsa-devel] [PATCH v7 0/3] 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 v6: - Series rebased on top of my other series fixing the return values from snd_compr_poll
These are based on Takashi's tree, as all the ADSP conflicts are now straightened out, although there is a reasonable chance we will do more ADSP during this cycle.
Thanks, Charles
Charles Keepax (3): 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: wm_adsp: Treat missing compressed buffer as a fatal error
include/sound/compress_driver.h | 5 +++ sound/core/compress_offload.c | 67 +++++++++++++++++++++++++++++++++++++++-- sound/soc/codecs/wm_adsp.c | 21 ++++++------- 3 files changed, 80 insertions(+), 13 deletions(-)
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 Acked-by: Vinod Koul vinod.koul@intel.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 9b3334b..2c49848 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); @@ -399,10 +417,16 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait) stream = &data->stream;
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 = snd_compr_get_poll(stream) | POLLERR; goto out; + default: + break; } + poll_wait(f, &stream->runtime->sleep, wait);
avail = snd_compr_get_avail(stream); @@ -697,6 +721,45 @@ 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); + + stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP); + wake_up(&stream->runtime->sleep); + + 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; + + pr_debug("Changing state to: %d\n", 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;
The patch
ALSA: compress: Add function to indicate the stream has gone bad
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 a4f2d87c63571d4cd9467d369f2fbf2362646043 Mon Sep 17 00:00:00 2001
From: Charles Keepax ckeepax@opensource.wolfsonmicro.com Date: Mon, 13 Jun 2016 14:17:10 +0100 Subject: [PATCH] ALSA: compress: Add function to indicate the stream has gone bad
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 Acked-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- 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 c0abcdc11470..cee8c00f3d3e 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 9b3334be9df2..2c498488af6c 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); @@ -399,10 +417,16 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait) stream = &data->stream;
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 = snd_compr_get_poll(stream) | POLLERR; goto out; + default: + break; } + poll_wait(f, &stream->runtime->sleep, wait);
avail = snd_compr_get_avail(stream); @@ -697,6 +721,45 @@ 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); + + stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP); + wake_up(&stream->runtime->sleep); + + 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; + + pr_debug("Changing state to: %d\n", 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;
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 --- 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 a07bd7c..8ed1cde 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;
If the DSP is powered down whilst a compressed stream is being processed we should treat this as a fatal error, clearly the stream is no longer valid.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com --- sound/soc/codecs/wm_adsp.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 8ed1cde..7e42474 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -3037,12 +3037,7 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
buf = compr->buf;
- if (!compr->buf) { - ret = -ENXIO; - goto out; - } - - if (compr->buf->error) { + if (!compr->buf || compr->buf->error) { snd_compr_stop_error(stream, SNDRV_PCM_STATE_XRUN); ret = -EIO; goto out; @@ -3161,10 +3156,7 @@ static int wm_adsp_compr_read(struct wm_adsp_compr *compr,
adsp_dbg(dsp, "Requested read of %zu bytes\n", count);
- if (!compr->buf) - return -ENXIO; - - if (compr->buf->error) { + if (!compr->buf || compr->buf->error) { snd_compr_stop_error(compr->stream, SNDRV_PCM_STATE_XRUN); return -EIO; }
The patch
ASoC: wm_adsp: Treat missing compressed buffer as a fatal error
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 28ee3d73773e2d9ae922f7496723ab5c92cc16de Mon Sep 17 00:00:00 2001
From: Charles Keepax ckeepax@opensource.wolfsonmicro.com Date: Mon, 13 Jun 2016 14:17:12 +0100 Subject: [PATCH] ASoC: wm_adsp: Treat missing compressed buffer as a fatal error
If the DSP is powered down whilst a compressed stream is being processed we should treat this as a fatal error, clearly the stream is no longer valid.
Signed-off-by: Charles Keepax ckeepax@opensource.wolfsonmicro.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/wm_adsp.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 8ed1cdececf2..7e42474d7ae4 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -3037,12 +3037,7 @@ int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
buf = compr->buf;
- if (!compr->buf) { - ret = -ENXIO; - goto out; - } - - if (compr->buf->error) { + if (!compr->buf || compr->buf->error) { snd_compr_stop_error(stream, SNDRV_PCM_STATE_XRUN); ret = -EIO; goto out; @@ -3161,10 +3156,7 @@ static int wm_adsp_compr_read(struct wm_adsp_compr *compr,
adsp_dbg(dsp, "Requested read of %zu bytes\n", count);
- if (!compr->buf) - return -ENXIO; - - if (compr->buf->error) { + if (!compr->buf || compr->buf->error) { snd_compr_stop_error(compr->stream, SNDRV_PCM_STATE_XRUN); return -EIO; }
participants (2)
-
Charles Keepax -
Mark Brown