[alsa-devel] [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
If the stream state is running or paused when device file is closed snd_compr_free() will send a SNDRV_PCM_TRIGGER_STOP to the stream.
Signed-off-by: Richard Fitzgerald rf@opensource.wolfsonmicro.com --- sound/core/compress_offload.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 91e6bcb..a81c5e4 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -132,6 +132,11 @@ static int snd_compr_open(struct inode *inode, struct file *f) static int snd_compr_free(struct inode *inode, struct file *f) { struct snd_compr_file *data = f->private_data; + + if ((data->stream.runtime->state == SNDRV_PCM_STATE_RUNNING) || + (data->stream.runtime->state == SNDRV_PCM_STATE_PAUSED)) + data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP); + data->stream.ops->free(&data->stream); kfree(data->stream.runtime->buffer); kfree(data->stream.runtime);
On Fri, Apr 26, 2013 at 02:47:06PM +0100, Richard Fitzgerald wrote:
- if ((data->stream.runtime->state == SNDRV_PCM_STATE_RUNNING) ||
(data->stream.runtime->state == SNDRV_PCM_STATE_PAUSED))data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
A switch statement would be more idiomatic for this sort of thing.
if ((data->stream.runtime->state == SNDRV_PCM_STATE_RUNNING) ||(data->stream.runtime->state == SNDRV_PCM_STATE_PAUSED))data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);A switch statement would be more idiomatic for this sort of thing.
This follows the general style used in other functions in the source file. I'm happy to upload a version that uses a switch instead if that's preferred.
On Fri, Apr 26, 2013 at 02:56:27PM +0100, Richard Fitzgerald wrote:
if ((data->stream.runtime->state == SNDRV_PCM_STATE_RUNNING) ||(data->stream.runtime->state == SNDRV_PCM_STATE_PAUSED))data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
A switch statement would be more idiomatic for this sort of thing.
This follows the general style used in other functions in the source file. I'm happy to upload a version that uses a switch instead if that's preferred.
Well, it doesn't quite follow the same style as the code generally does:
if (stream->runtime->state == SNDRV_PCM_STATE_PAUSED || stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
with fewer () and different indentation to what you have above... being consistent with the rest of the file is best, though it would be nice to update the file to do this sort of thing with switches.
On Fri, Apr 26, 2013 at 02:47:06PM +0100, Richard Fitzgerald wrote:
If the stream state is running or paused when device file is closed snd_compr_free() will send a SNDRV_PCM_TRIGGER_STOP to the stream.
Signed-off-by: Richard Fitzgerald rf@opensource.wolfsonmicro.com
sound/core/compress_offload.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 91e6bcb..a81c5e4 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -132,6 +132,11 @@ static int snd_compr_open(struct inode *inode, struct file *f) static int snd_compr_free(struct inode *inode, struct file *f) { struct snd_compr_file *data = f->private_data;
- if ((data->stream.runtime->state == SNDRV_PCM_STATE_RUNNING) ||
(data->stream.runtime->state == SNDRV_PCM_STATE_PAUSED))data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
two things: 1. I like Mark's idea the switch will make it look better :) 2. why do you want to do explicit stop when you are freeing. Esp the Paused cases doesnt make sense. Shouldnt the DSP be able to handle this transistion?
-- ~Vinod
- data->stream.ops->free(&data->stream); kfree(data->stream.runtime->buffer); kfree(data->stream.runtime);
-- 1.7.2.5
On Fri, Apr 26, 2013 at 10:12:55PM +0530, Vinod Koul wrote:
On Fri, Apr 26, 2013 at 02:47:06PM +0100, Richard Fitzgerald wrote:
If the stream state is running or paused when device file is closed snd_compr_free() will send a SNDRV_PCM_TRIGGER_STOP to the stream.
Signed-off-by: Richard Fitzgerald rf@opensource.wolfsonmicro.com
sound/core/compress_offload.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index 91e6bcb..a81c5e4 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -132,6 +132,11 @@ static int snd_compr_open(struct inode *inode, struct file *f) static int snd_compr_free(struct inode *inode, struct file *f) { struct snd_compr_file *data = f->private_data;
- if ((data->stream.runtime->state == SNDRV_PCM_STATE_RUNNING) ||
(data->stream.runtime->state == SNDRV_PCM_STATE_PAUSED))data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);two things:
- I like Mark's idea the switch will make it look better :)
- why do you want to do explicit stop when you are freeing. Esp the Paused
cases doesnt make sense. Shouldnt the DSP be able to handle this transistion?
Userspace process might be killed, or crash, or just be bugged and we need to protect against that. Nicer if the framework handles this instead of pushing the problem down to every codec driver.
participants (3)
-
Mark Brown -
Richard Fitzgerald -
Vinod Koul