[alsa-devel] [PATCH 0/5] Fix poll error returns
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(-)
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);
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
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
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
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
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:
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):
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);
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; }
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
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
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
participants (5)
-
Charles Keepax -
Clemens Ladisch -
Takashi Iwai -
Takashi Sakamoto -
Vinod Koul