| Message ID | 1462370351-15388-2-git-send-email-ckeepax@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
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(-)