[PATCH 0/5] Fix poll error returns

[PATCH 0/5] Fix poll error returns

[Charles Keepax]

We return negative values from the file_operations poll callback
in a few places, this callback returns an unsigned int and is
expected to only return the poll specific flags. This series
fixes up these issues and makes a couple of small tidy ups to the
code around the edges of those functions.

I noticed this issue whilst doing some additional testing on my
propagation of compressed stream error series, but I decided to
push these up separately as it is worth getting the fixes in
now and not tying them up with that patch chain which is taking
longer to get merged.  Also I included the first patch of that
chain (Replace complex if statement with switch) because it is a
trivial tidy up and might as well get merged now as well.

Thanks,
Charles

Charles Keepax (5):
  ALSA: pcm: Fix poll error return codes
  ALSA: compress: Use snd_compr_get_poll on error path
  ALSA: compress: Remove pointless NULL check
  ALSA: compress: Fix poll error return codes
  ALSA: compress: Replace complex if statement with switch

 sound/core/compress_offload.c | 25 ++++++++++++-------------
 sound/core/pcm_native.c       |  4 ++--
 2 files changed, 14 insertions(+), 15 deletions(-)

-- 
2.1.4

[PATCH 1/5] ALSA: pcm: Fix poll error return codes

[Charles Keepax]

We can't return a negative error code from the poll callback the return
type is unsigned and is checked against the poll specific flags we need
to return POLLERR if we encounter an error.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/pcm_native.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 9106d8e..c61fd50 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3161,7 +3161,7 @@ static unsigned int snd_pcm_playback_poll(struct file *file, poll_table * wait)
 
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
-		return -ENXIO;
+		return POLLOUT | POLLWRNORM | POLLERR;
 	runtime = substream->runtime;
 
 	poll_wait(file, &runtime->sleep, wait);
@@ -3200,7 +3200,7 @@ static unsigned int snd_pcm_capture_poll(struct file *file, poll_table * wait)
 
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
-		return -ENXIO;
+		return POLLIN | POLLRDNORM | POLLERR;
 	runtime = substream->runtime;
 
 	poll_wait(file, &runtime->sleep, wait);
-- 
2.1.4

Re: [PATCH 1/5] ALSA: pcm: Fix poll error return codes

[Takashi Sakamoto]

Hi,

On May 4 2016 22:59, Charles Keepax wrote:
> We can't return a negative error code from the poll callback the return
> type is unsigned and is checked against the poll specific flags we need
> to return POLLERR if we encounter an error.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  sound/core/pcm_native.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 9106d8e..c61fd50 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3161,7 +3161,7 @@ static unsigned int snd_pcm_playback_poll(struct file *file, poll_table * wait)
>  
>  	substream = pcm_file->substream;
>  	if (PCM_RUNTIME_CHECK(substream))
> -		return -ENXIO;
> +		return POLLOUT | POLLWRNORM | POLLERR;
>  	runtime = substream->runtime;
>  
>  	poll_wait(file, &runtime->sleep, wait);
> @@ -3200,7 +3200,7 @@ static unsigned int snd_pcm_capture_poll(struct file *file, poll_table * wait)
>  
>  	substream = pcm_file->substream;
>  	if (PCM_RUNTIME_CHECK(substream))
> -		return -ENXIO;
> +		return POLLIN | POLLRDNORM | POLLERR;
>  	runtime = substream->runtime;
>  
>  	poll_wait(file, &runtime->sleep, wait);

I agree with the concept of your patch to fix the return value of ALSA
PCM core. It should return a value which consists of POLLxxx masks.

On the other hand, I think POLLOUT, POLLIN, POLLWRNORM and POLLRDNORM
should not be included in the value. PCM_RUNTIME_CHECK() ensures PCM
substream or PCM runtime is NULL. This means that subsequent I/O
operations are failed, at least for handling PCM frames.

I think it better to return 'POLLERR | POLLHUP'.


Regards

Takashi Sakamoto

Re: [PATCH 1/5] ALSA: pcm: Fix poll error return codes

[Clemens Ladisch]

Takashi Sakamoto wrote:
> On May 4 2016 22:59, Charles Keepax wrote:
>>  	if (PCM_RUNTIME_CHECK(substream))
>> -		return -ENXIO;
>> +		return POLLIN | POLLRDNORM | POLLERR;
>
> [...]
> On the other hand, I think POLLOUT, POLLIN, POLLWRNORM and POLLRDNORM
> should not be included in the value. PCM_RUNTIME_CHECK() ensures PCM
> substream or PCM runtime is NULL. This means that subsequent I/O
> operations are failed, at least for handling PCM frames.
>
> I think it better to return 'POLLERR | POLLHUP'.

To expand on this: POLLIN/POLLOUT imply that it is possible to read/
write data without blocking.  Sockets and pipes combine POLLHUP with
POLLIN because the read() (or recv()) returns 0 bytes without blocking
to indicate the end of the stream.

But in this situation, snd_pcm_read*/write* will always fail, so it is,
strictly speaking, indeed not appropriate to set POLLIN/OUT.

On the other hand, PCM devices do include the POLLIN/OUT bits when they
are in a wrong state.  (This is probably to catch programs that do not
check the error bits; with POLLIN/OUT set, these programs will try to
read/write, and will then get the error code.)

So for consistency, the bits should be included.  (Or the other error
case fixed to remove these bits.)


Regards,
Clemens

Re: [PATCH 1/5] ALSA: pcm: Fix poll error return codes

[Charles Keepax]

On Thu, May 05, 2016 at 11:39:34AM +0200, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
> > On May 4 2016 22:59, Charles Keepax wrote:
> >>  	if (PCM_RUNTIME_CHECK(substream))
> >> -		return -ENXIO;
> >> +		return POLLIN | POLLRDNORM | POLLERR;
> >
> > [...]
> > On the other hand, I think POLLOUT, POLLIN, POLLWRNORM and POLLRDNORM
> > should not be included in the value. PCM_RUNTIME_CHECK() ensures PCM
> > substream or PCM runtime is NULL. This means that subsequent I/O
> > operations are failed, at least for handling PCM frames.
> >
> > I think it better to return 'POLLERR | POLLHUP'.
> 
> To expand on this: POLLIN/POLLOUT imply that it is possible to read/
> write data without blocking.  Sockets and pipes combine POLLHUP with
> POLLIN because the read() (or recv()) returns 0 bytes without blocking
> to indicate the end of the stream.
> 
> But in this situation, snd_pcm_read*/write* will always fail, so it is,
> strictly speaking, indeed not appropriate to set POLLIN/OUT.
> 
> On the other hand, PCM devices do include the POLLIN/OUT bits when they
> are in a wrong state.  (This is probably to catch programs that do not
> check the error bits; with POLLIN/OUT set, these programs will try to
> read/write, and will then get the error code.)
> 
> So for consistency, the bits should be included.  (Or the other error
> case fixed to remove these bits.)

Thanks for the explaination guys. I definitely agree that all
the return values should be consistent. I am happy to change the
values if people prefer but I guess the decision really rests
with Takashi and if he is happy to change the returns to
POLLERR | POLLHUP, as I guess there is the potential for some
user-space fall out. Perhaps I should do this as a seperate patch
on top of this chain, so we can review explicitly.

I have had a look and both tinyalsa and alsalib look like they
would handle the change correctly.

Thanks,
Charles

Re: [PATCH 1/5] ALSA: pcm: Fix poll error return codes

[Takashi Sakamoto]

Hi Charles and Clemens,

Sorry to be late.

On May 5 2016 20:46, Charles Keepax wrote:
> On Thu, May 05, 2016 at 11:39:34AM +0200, Clemens Ladisch wrote:
>> Takashi Sakamoto wrote:
>>> On May 4 2016 22:59, Charles Keepax wrote:
>>>>  	if (PCM_RUNTIME_CHECK(substream))
>>>> -		return -ENXIO;
>>>> +		return POLLIN | POLLRDNORM | POLLERR;
>>>
>>> [...]
>>> On the other hand, I think POLLOUT, POLLIN, POLLWRNORM and POLLRDNORM
>>> should not be included in the value. PCM_RUNTIME_CHECK() ensures PCM
>>> substream or PCM runtime is NULL. This means that subsequent I/O
>>> operations are failed, at least for handling PCM frames.
>>>
>>> I think it better to return 'POLLERR | POLLHUP'.
>>
>> To expand on this: POLLIN/POLLOUT imply that it is possible to read/
>> write data without blocking.  Sockets and pipes combine POLLHUP with
>> POLLIN because the read() (or recv()) returns 0 bytes without blocking
>> to indicate the end of the stream.
>>
>> But in this situation, snd_pcm_read*/write* will always fail, so it is,
>> strictly speaking, indeed not appropriate to set POLLIN/OUT.
>>
>> On the other hand, PCM devices do include the POLLIN/OUT bits when they
>> are in a wrong state.  (This is probably to catch programs that do not
>> check the error bits; with POLLIN/OUT set, these programs will try to
>> read/write, and will then get the error code.)
>>
>> So for consistency, the bits should be included.  (Or the other error
>> case fixed to remove these bits.)

I agree with the Clemens's view. So this patch is OK to me.

Reviewd-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

But if we have the intention for including POLLIN/OUT, it's better to
add some comments about it.

> Thanks for the explaination guys. I definitely agree that all
> the return values should be consistent. I am happy to change the
> values if people prefer but I guess the decision really rests
> with Takashi and if he is happy to change the returns to
> POLLERR | POLLHUP, as I guess there is the potential for some
> user-space fall out. Perhaps I should do this as a seperate patch
> on top of this chain, so we can review explicitly.

I guess that this patch can be applied to ALSA PCM core separately from
the others for ALSA Compress core. So it's better to post two series;
one includes this patch, another includes the rest.

> I have had a look and both tinyalsa and alsalib look like they
> would handle the change correctly.

In the same time, it's better for us to consider that
userspace applications can be programmed directly to use
kernel/userspace interface without these I/O libraries.


Regards

Takashi (not-maintainer) Sakamoto

[PATCH 2/5] ALSA: compress: Use snd_compr_get_poll on error path

[Charles Keepax]

We have a function that returns the appropriate flags for the stream
direction, so we should use it.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index a9933c07..5268546 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -421,10 +421,7 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 			retval = snd_compr_get_poll(stream);
 		break;
 	default:
-		if (stream->direction == SND_COMPRESS_PLAYBACK)
-			retval = POLLOUT | POLLWRNORM | POLLERR;
-		else
-			retval = POLLIN | POLLRDNORM | POLLERR;
+		retval = snd_compr_get_poll(stream) | POLLERR;
 		break;
 	}
 out:
-- 
2.1.4

[PATCH 3/5] ALSA: compress: Remove pointless NULL check

[Charles Keepax]

stream can't be NULL here as we have just taken the address of it, so no
need for the check.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 5268546..5215df2 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -392,9 +392,8 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 
 	if (snd_BUG_ON(!data))
 		return -EFAULT;
+
 	stream = &data->stream;
-	if (snd_BUG_ON(!stream))
-		return -EFAULT;
 
 	mutex_lock(&stream->device->lock);
 	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
@@ -799,9 +798,9 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
 
 	if (snd_BUG_ON(!data))
 		return -EFAULT;
+
 	stream = &data->stream;
-	if (snd_BUG_ON(!stream))
-		return -EFAULT;
+
 	mutex_lock(&stream->device->lock);
 	switch (_IOC_NR(cmd)) {
 	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
-- 
2.1.4

[PATCH 4/5] ALSA: compress: Fix poll error return codes

[Charles Keepax]

We can't return a negative error code from the poll callback the return
type is unsigned and is checked against the poll specific flags we need
to return POLLERR if we encounter an error.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 5215df2..f56f4e3 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -391,13 +391,13 @@ static unsigned int snd_compr_poll(struct file *f, poll_table *wait)
 	int retval = 0;
 
 	if (snd_BUG_ON(!data))
-		return -EFAULT;
+		return POLLERR;
 
 	stream = &data->stream;
 
 	mutex_lock(&stream->device->lock);
 	if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
-		retval = -EBADFD;
+		retval = snd_compr_get_poll(stream) | POLLERR;
 		goto out;
 	}
 	poll_wait(f, &stream->runtime->sleep, wait);
-- 
2.1.4

[PATCH v6 RESEND 5/5] ALSA: compress: Replace complex if statement with switch

[Charles Keepax]

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>
Acked-by: Vinod Koul <vinod.koul@intel.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 f56f4e3..9b3334b 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;
 	}
-- 
2.1.4

Re: [PATCH 0/5] Fix poll error returns

[Takashi Iwai]

On Wed, 04 May 2016 15:59:06 +0200,
Charles Keepax wrote:
> 
> We return negative values from the file_operations poll callback
> in a few places, this callback returns an unsigned int and is
> expected to only return the poll specific flags. This series
> fixes up these issues and makes a couple of small tidy ups to the
> code around the edges of those functions.
> 
> I noticed this issue whilst doing some additional testing on my
> propagation of compressed stream error series, but I decided to
> push these up separately as it is worth getting the fixes in
> now and not tying them up with that patch chain which is taking
> longer to get merged.  Also I included the first patch of that
> chain (Replace complex if statement with switch) because it is a
> trivial tidy up and might as well get merged now as well.

Patches 2, 3 and 4 miss Vinod's ack.  Vinod, are you OK with them?


thanks,

Takashi

> 
> Thanks,
> Charles
> 
> Charles Keepax (5):
>   ALSA: pcm: Fix poll error return codes
>   ALSA: compress: Use snd_compr_get_poll on error path
>   ALSA: compress: Remove pointless NULL check
>   ALSA: compress: Fix poll error return codes
>   ALSA: compress: Replace complex if statement with switch
> 
>  sound/core/compress_offload.c | 25 ++++++++++++-------------
>  sound/core/pcm_native.c       |  4 ++--
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> -- 
> 2.1.4
> 
> 

Re: [PATCH 0/5] Fix poll error returns

[Vinod Koul]

On Mon, May 09, 2016 at 02:13:55PM +0200, Takashi Iwai wrote:
> On Wed, 04 May 2016 15:59:06 +0200,
> Charles Keepax wrote:
> > 
> > We return negative values from the file_operations poll callback
> > in a few places, this callback returns an unsigned int and is
> > expected to only return the poll specific flags. This series
> > fixes up these issues and makes a couple of small tidy ups to the
> > code around the edges of those functions.
> > 
> > I noticed this issue whilst doing some additional testing on my
> > propagation of compressed stream error series, but I decided to
> > push these up separately as it is worth getting the fixes in
> > now and not tying them up with that patch chain which is taking
> > longer to get merged.  Also I included the first patch of that
> > chain (Replace complex if statement with switch) because it is a
> > trivial tidy up and might as well get merged now as well.
> 
> Patches 2, 3 and 4 miss Vinod's ack.  Vinod, are you OK with them?

Yup, they look good to me so:

Acked-by: Vinod Koul <vinod.koul@intel.com>

Thanks
-- 
~Vinod

Re: [PATCH 0/5] Fix poll error returns

[Takashi Iwai]

On Mon, 09 May 2016 17:19:19 +0200,
Vinod Koul wrote:
> 
> On Mon, May 09, 2016 at 02:13:55PM +0200, Takashi Iwai wrote:
> > On Wed, 04 May 2016 15:59:06 +0200,
> > Charles Keepax wrote:
> > > 
> > > We return negative values from the file_operations poll callback
> > > in a few places, this callback returns an unsigned int and is
> > > expected to only return the poll specific flags. This series
> > > fixes up these issues and makes a couple of small tidy ups to the
> > > code around the edges of those functions.
> > > 
> > > I noticed this issue whilst doing some additional testing on my
> > > propagation of compressed stream error series, but I decided to
> > > push these up separately as it is worth getting the fixes in
> > > now and not tying them up with that patch chain which is taking
> > > longer to get merged.  Also I included the first patch of that
> > > chain (Replace complex if statement with switch) because it is a
> > > trivial tidy up and might as well get merged now as well.
> > 
> > Patches 2, 3 and 4 miss Vinod's ack.  Vinod, are you OK with them?
> 
> Yup, they look good to me so:
> 
> Acked-by: Vinod Koul <vinod.koul@intel.com>

OK, merged all five patches now.  Thanks.


Takashi