* [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
@ 2013-04-26 13:47 Richard Fitzgerald
2013-04-26 13:48 ` Mark Brown
2013-04-26 16:42 ` Vinod Koul
0 siblings, 2 replies; 6+ messages in thread
From: Richard Fitzgerald @ 2013-04-26 13:47 UTC (permalink / raw)
To: vinod.koul, tiwai, broonie; +Cc: alsa-devel, patches, lgirdwood
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);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
2013-04-26 13:47 [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed Richard Fitzgerald
@ 2013-04-26 13:48 ` Mark Brown
2013-04-26 13:56 ` Richard Fitzgerald
2013-04-26 16:42 ` Vinod Koul
1 sibling, 1 reply; 6+ messages in thread
From: Mark Brown @ 2013-04-26 13:48 UTC (permalink / raw)
To: Richard Fitzgerald; +Cc: alsa-devel, vinod.koul, patches, lgirdwood, tiwai
[-- Attachment #1.1: Type: text/plain, Size: 345 bytes --]
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.
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
2013-04-26 13:48 ` Mark Brown
@ 2013-04-26 13:56 ` Richard Fitzgerald
2013-04-26 15:38 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Richard Fitzgerald @ 2013-04-26 13:56 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, vinod.koul, patches, lgirdwood, tiwai
>> + 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
2013-04-26 13:56 ` Richard Fitzgerald
@ 2013-04-26 15:38 ` Mark Brown
0 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2013-04-26 15:38 UTC (permalink / raw)
To: Richard Fitzgerald; +Cc: alsa-devel, vinod.koul, patches, lgirdwood, tiwai
[-- Attachment #1.1: Type: text/plain, Size: 958 bytes --]
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.
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
2013-04-26 13:47 [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed Richard Fitzgerald
2013-04-26 13:48 ` Mark Brown
@ 2013-04-26 16:42 ` Vinod Koul
2013-04-29 9:36 ` Richard Fitzgerald
1 sibling, 1 reply; 6+ messages in thread
From: Vinod Koul @ 2013-04-26 16:42 UTC (permalink / raw)
To: Richard Fitzgerald; +Cc: alsa-devel, broonie, tiwai, patches, lgirdwood
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed
2013-04-26 16:42 ` Vinod Koul
@ 2013-04-29 9:36 ` Richard Fitzgerald
0 siblings, 0 replies; 6+ messages in thread
From: Richard Fitzgerald @ 2013-04-29 9:36 UTC (permalink / raw)
To: Vinod Koul; +Cc: alsa-devel, broonie, tiwai, patches, lgirdwood
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:
> 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?
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-04-29 9:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-26 13:47 [PATCH 1/1] ALSA: compress_core: Ensure device is stopped when stream closed Richard Fitzgerald
2013-04-26 13:48 ` Mark Brown
2013-04-26 13:56 ` Richard Fitzgerald
2013-04-26 15:38 ` Mark Brown
2013-04-26 16:42 ` Vinod Koul
2013-04-29 9:36 ` Richard Fitzgerald
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.