[PATCH] ALSA: compress: fix the states to check for allowing read

[PATCH] ALSA: compress: fix the states to check for allowing read

[Vinod Koul]

for reading compressed data, we need to allow when we are paused, draining or
stopped.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

---
 sound/core/compress_offload.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index a0bc47f..5389b9a 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -311,8 +311,14 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
 	stream = &data->stream;
 	mutex_lock(&stream->device->lock);
 
-	/* read is allowed when stream is running */
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
+	/* read is allowed when stream is running, paused, draining and setup
+	 * (yes setup is state which we transistion to after stop, so if user
+	 * wants to read data after stop we allow that
+	 */
+	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING ||
+			stream->runtime->state != SNDRV_PCM_STATE_DRAINING ||
+			stream->runtime->state != SNDRV_PCM_STATE_PAUSED ||
+			stream->runtime->state != SNDRV_PCM_STATE_SETUP) {
 		retval = -EBADFD;
 		goto out;
 	}
-- 
1.7.0.4

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Vinod Koul]

On Mon, Apr 29, 2013 at 10:55:04AM +0200, Takashi Iwai wrote:
> At Sun, 28 Apr 2013 13:35:22 +0530,
> Vinod Koul wrote:
> > 
> > for reading compressed data, we need to allow when we are paused, draining or
> > stopped.
> > 
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > Cc: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > 
> > ---
> >  sound/core/compress_offload.c |   10 ++++++++--
> >  1 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index a0bc47f..5389b9a 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -311,8 +311,14 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
> >  	stream = &data->stream;
> >  	mutex_lock(&stream->device->lock);
> >  
> > -	/* read is allowed when stream is running */
> > -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> > +	/* read is allowed when stream is running, paused, draining and setup
> > +	 * (yes setup is state which we transistion to after stop, so if user
> > +	 * wants to read data after stop we allow that
> > +	 */
> > +	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING ||
> > +			stream->runtime->state != SNDRV_PCM_STATE_DRAINING ||
> > +			stream->runtime->state != SNDRV_PCM_STATE_PAUSED ||
> > +			stream->runtime->state != SNDRV_PCM_STATE_SETUP) {
> >  		retval = -EBADFD;
> 
> Aren't they "&&"?
Yup :(
> 
> Maybe better to use switch for avoiding such a trivial error...
I started with switch and then didnt want to code for all the PCM_STATES... but
yes that makes more sense, i will send that right away.
Is it okay if this get in for 3.10...

--
~Vinod

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Takashi Iwai]

At Sun, 28 Apr 2013 13:35:22 +0530,
Vinod Koul wrote:
> 
> for reading compressed data, we need to allow when we are paused, draining or
> stopped.
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Cc: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> 
> ---
>  sound/core/compress_offload.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index a0bc47f..5389b9a 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -311,8 +311,14 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
>  	stream = &data->stream;
>  	mutex_lock(&stream->device->lock);
>  
> -	/* read is allowed when stream is running */
> -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> +	/* read is allowed when stream is running, paused, draining and setup
> +	 * (yes setup is state which we transistion to after stop, so if user
> +	 * wants to read data after stop we allow that
> +	 */
> +	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING ||
> +			stream->runtime->state != SNDRV_PCM_STATE_DRAINING ||
> +			stream->runtime->state != SNDRV_PCM_STATE_PAUSED ||
> +			stream->runtime->state != SNDRV_PCM_STATE_SETUP) {
>  		retval = -EBADFD;

Aren't they "&&"?

Maybe better to use switch for avoiding such a trivial error...


Takashi

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Vinod Koul]

On Mon, Apr 29, 2013 at 11:20:42AM +0200, Takashi Iwai wrote:
> > I started with switch and then didnt want to code for all the PCM_STATES... but
> > yes that makes more sense, i will send that right away.
> 
> You can code like
> 	switch (stream->runtime->state) {
> 	case SNDRV_PCM_STATE_RUNNING:
> 	case SNDRV_PCM_STATE_DRAINING:
> 	case SNDRV_PCM_STATE_PAUSED:
> 	case SNDRV_PCM_STATE_SETUP:
> 		break; /* OK */
> 	default:
> 		retval = -EBADFD;
> 		break;
> 	}
Yup, and i did exactly opposite, let me know if you want like this :)

--
~Vinod

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Vinod Koul]

On Mon, Apr 29, 2013 at 10:29:00AM +0100, Richard Fitzgerald wrote:
> Ok with me too.
> (You beat me to it with this fix)
Blame it on merge window :) Wanted to ensure 3.10 compress record is fine

--
~Vinod

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Takashi Iwai]

At Mon, 29 Apr 2013 14:15:31 +0530,
Vinod Koul wrote:
> 
> On Mon, Apr 29, 2013 at 10:55:04AM +0200, Takashi Iwai wrote:
> > At Sun, 28 Apr 2013 13:35:22 +0530,
> > Vinod Koul wrote:
> > > 
> > > for reading compressed data, we need to allow when we are paused, draining or
> > > stopped.
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > Cc: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > > 
> > > ---
> > >  sound/core/compress_offload.c |   10 ++++++++--
> > >  1 files changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index a0bc47f..5389b9a 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -311,8 +311,14 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
> > >  	stream = &data->stream;
> > >  	mutex_lock(&stream->device->lock);
> > >  
> > > -	/* read is allowed when stream is running */
> > > -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> > > +	/* read is allowed when stream is running, paused, draining and setup
> > > +	 * (yes setup is state which we transistion to after stop, so if user
> > > +	 * wants to read data after stop we allow that
> > > +	 */
> > > +	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING ||
> > > +			stream->runtime->state != SNDRV_PCM_STATE_DRAINING ||
> > > +			stream->runtime->state != SNDRV_PCM_STATE_PAUSED ||
> > > +			stream->runtime->state != SNDRV_PCM_STATE_SETUP) {
> > >  		retval = -EBADFD;
> > 
> > Aren't they "&&"?
> Yup :(
> > 
> > Maybe better to use switch for avoiding such a trivial error...
> I started with switch and then didnt want to code for all the PCM_STATES... but
> yes that makes more sense, i will send that right away.

You can code like
	switch (stream->runtime->state) {
	case SNDRV_PCM_STATE_RUNNING:
	case SNDRV_PCM_STATE_DRAINING:
	case SNDRV_PCM_STATE_PAUSED:
	case SNDRV_PCM_STATE_SETUP:
		break; /* OK */
	default:
		retval = -EBADFD;
		break;
	}


> Is it okay if this get in for 3.10...

Yes.  It's just a damn simple fix.


thanks,

Takashi

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Richard Fitzgerald]

On Mon, Apr 29, 2013 at 11:20:42AM +0200, Takashi Iwai wrote:
> At Mon, 29 Apr 2013 14:15:31 +0530,
> Vinod Koul wrote:
> > 
> > On Mon, Apr 29, 2013 at 10:55:04AM +0200, Takashi Iwai wrote:
> > > At Sun, 28 Apr 2013 13:35:22 +0530,
> > > Vinod Koul wrote:
> > > > 
> > > > for reading compressed data, we need to allow when we are paused, draining or
> > > > stopped.
> > > > 
> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > Cc: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> > > > 
> > > > ---
> > > >  sound/core/compress_offload.c |   10 ++++++++--
> > > >  1 files changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > > index a0bc47f..5389b9a 100644
> > > > --- a/sound/core/compress_offload.c
> > > > +++ b/sound/core/compress_offload.c
> > > > @@ -311,8 +311,14 @@ static ssize_t snd_compr_read(struct file *f, char __user *buf,
> > > >  	stream = &data->stream;
> > > >  	mutex_lock(&stream->device->lock);
> > > >  
> > > > -	/* read is allowed when stream is running */
> > > > -	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
> > > > +	/* read is allowed when stream is running, paused, draining and setup
> > > > +	 * (yes setup is state which we transistion to after stop, so if user
> > > > +	 * wants to read data after stop we allow that
> > > > +	 */
> > > > +	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING ||
> > > > +			stream->runtime->state != SNDRV_PCM_STATE_DRAINING ||
> > > > +			stream->runtime->state != SNDRV_PCM_STATE_PAUSED ||
> > > > +			stream->runtime->state != SNDRV_PCM_STATE_SETUP) {
> > > >  		retval = -EBADFD;
> > > 
> > > Aren't they "&&"?
> > Yup :(
> > > 
> > > Maybe better to use switch for avoiding such a trivial error...
> > I started with switch and then didnt want to code for all the PCM_STATES... but
> > yes that makes more sense, i will send that right away.
> 
> You can code like
> 	switch (stream->runtime->state) {
> 	case SNDRV_PCM_STATE_RUNNING:
> 	case SNDRV_PCM_STATE_DRAINING:
> 	case SNDRV_PCM_STATE_PAUSED:
> 	case SNDRV_PCM_STATE_SETUP:
> 		break; /* OK */
> 	default:
> 		retval = -EBADFD;
> 		break;
> 	}
> 
> 
> > Is it okay if this get in for 3.10...
> 
> Yes.  It's just a damn simple fix.
> 
> 
> thanks,
> 
> Takashi


Ok with me too.
(You beat me to it with this fix)

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 695 bytes --]

On Mon, Apr 29, 2013 at 12:07:14PM +0100, Mark Brown wrote:
> On Sun, Apr 28, 2013 at 01:35:22PM +0530, Vinod Koul wrote:
> > for reading compressed data, we need to allow when we are paused, draining or
> > stopped.
> 
> Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> > +	/* read is allowed when stream is running, paused, draining and setup
> > +	 * (yes setup is state which we transistion to after stop, so if user
> > +	 * wants to read data after stop we allow that
> > +	 */
> 
> but I think there's a ) missing in here!
Yup :( I guess since Takshi applied it he can fixup it at end of para, and I
will owe him a beer next time we meet :)

--
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: compress: fix the states to check for allowing read

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 464 bytes --]

On Sun, Apr 28, 2013 at 01:35:22PM +0530, Vinod Koul wrote:
> for reading compressed data, we need to allow when we are paused, draining or
> stopped.

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

> +	/* read is allowed when stream is running, paused, draining and setup
> +	 * (yes setup is state which we transistion to after stop, so if user
> +	 * wants to read data after stop we allow that
> +	 */

but I think there's a ) missing in here!

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]