| Message ID | 1380721516-488-5-git-send-email-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/02/13 15:45, Sakari Ailus wrote: > Dequeueing events was is entirely possible even if none are subscribed, > leading to sleeping indefinitely. Fix this by returning -ENOENT when no > events are subscribed. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c > index b53897e..553a800 100644 > --- a/drivers/media/v4l2-core/v4l2-event.c > +++ b/drivers/media/v4l2-core/v4l2-event.c > @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, > mutex_unlock(fh->vdev->lock); > > do { > - ret = wait_event_interruptible(fh->wait, > - fh->navailable != 0); > + bool subscribed; Can you add an empty line here? > + ret = wait_event_interruptible( > + fh->wait, > + fh->navailable != 0 || > + !(subscribed = v4l2_event_has_subscribed(fh))); > if (ret < 0) > break; > + if (!subscribed) { > + ret = -EIO; Shouldn't this be -ENOENT? > + break; > + } > > ret = __v4l2_event_dequeue(fh, event); > } while (ret == -ENOENT); > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the comments! Hans Verkuil wrote: > On 10/02/13 15:45, Sakari Ailus wrote: >> Dequeueing events was is entirely possible even if none are subscribed, >> leading to sleeping indefinitely. Fix this by returning -ENOENT when no >> events are subscribed. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-event.c >> b/drivers/media/v4l2-core/v4l2-event.c >> index b53897e..553a800 100644 >> --- a/drivers/media/v4l2-core/v4l2-event.c >> +++ b/drivers/media/v4l2-core/v4l2-event.c >> @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct >> v4l2_event *event, >> mutex_unlock(fh->vdev->lock); >> >> do { >> - ret = wait_event_interruptible(fh->wait, >> - fh->navailable != 0); >> + bool subscribed; > > Can you add an empty line here? Sure. >> + ret = wait_event_interruptible( >> + fh->wait, >> + fh->navailable != 0 || >> + !(subscribed = v4l2_event_has_subscribed(fh))); >> if (ret < 0) >> break; >> + if (!subscribed) { >> + ret = -EIO; > > Shouldn't this be -ENOENT? If I use -ENOENT, having no events subscribed is indistinguishable form no events pending condition. Combine that with using select(2), and you can no longer distinguish having no events subscribed from the case where you got an event but someone else (another thread or process) dequeued it. -EIO makes that explicit --- this also mirrors the behaviour of VIDIOC_DQBUF. (And it must be documented as well, which is missing from the patch currently.)
On 10/02/13 16:18, Sakari Ailus wrote: > Hi Hans, > > Thanks for the comments! > > Hans Verkuil wrote: >> On 10/02/13 15:45, Sakari Ailus wrote: >>> Dequeueing events was is entirely possible even if none are subscribed, >>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no >>> events are subscribed. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-event.c >>> b/drivers/media/v4l2-core/v4l2-event.c >>> index b53897e..553a800 100644 >>> --- a/drivers/media/v4l2-core/v4l2-event.c >>> +++ b/drivers/media/v4l2-core/v4l2-event.c >>> @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct >>> v4l2_event *event, >>> mutex_unlock(fh->vdev->lock); >>> >>> do { >>> - ret = wait_event_interruptible(fh->wait, >>> - fh->navailable != 0); >>> + bool subscribed; >> >> Can you add an empty line here? > > Sure. > >>> + ret = wait_event_interruptible( >>> + fh->wait, >>> + fh->navailable != 0 || >>> + !(subscribed = v4l2_event_has_subscribed(fh))); >>> if (ret < 0) >>> break; >>> + if (!subscribed) { >>> + ret = -EIO; >> >> Shouldn't this be -ENOENT? > > If I use -ENOENT, having no events subscribed is indistinguishable > form no events pending condition. Combine that with using select(2), > and you can no longer distinguish having no events subscribed from > the case where you got an event but someone else (another thread or > process) dequeued it. OK, but then your commit message is out of sync with the actual patch since the commit log says ENOENT. > -EIO makes that explicit --- this also mirrors the behaviour of > VIDIOC_DQBUF. (And it must be documented as well, which is missing > from the patch currently.) I don't like using EIO for this. EIO generally is returned if a hardware error or an unexpected hardware condition occurs. How about -ENOMSG? Or perhaps EPIPE? (As in: "the pipe containing events is gone"). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Hans Verkuil wrote: > On 10/02/13 16:18, Sakari Ailus wrote: >> Hi Hans, >> >> Thanks for the comments! >> >> Hans Verkuil wrote: >>> On 10/02/13 15:45, Sakari Ailus wrote: >>>> Dequeueing events was is entirely possible even if none are subscribed, >>>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no >>>> events are subscribed. >>>> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- >>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c >>>> b/drivers/media/v4l2-core/v4l2-event.c >>>> index b53897e..553a800 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-event.c >>>> +++ b/drivers/media/v4l2-core/v4l2-event.c >>>> @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct >>>> v4l2_event *event, >>>> mutex_unlock(fh->vdev->lock); >>>> >>>> do { >>>> - ret = wait_event_interruptible(fh->wait, >>>> - fh->navailable != 0); >>>> + bool subscribed; >>> >>> Can you add an empty line here? >> >> Sure. >> >>>> + ret = wait_event_interruptible( >>>> + fh->wait, >>>> + fh->navailable != 0 || >>>> + !(subscribed = v4l2_event_has_subscribed(fh))); >>>> if (ret < 0) >>>> break; >>>> + if (!subscribed) { >>>> + ret = -EIO; >>> >>> Shouldn't this be -ENOENT? >> >> If I use -ENOENT, having no events subscribed is indistinguishable >> form no events pending condition. Combine that with using select(2), >> and you can no longer distinguish having no events subscribed from >> the case where you got an event but someone else (another thread or >> process) dequeued it. > > OK, but then your commit message is out of sync with the actual patch since > the commit log says ENOENT. Right. The error code was the last thing I changed before sending the patch, and I ignored it was also present in the commit message. :-P >> -EIO makes that explicit --- this also mirrors the behaviour of >> VIDIOC_DQBUF. (And it must be documented as well, which is missing >> from the patch currently.) > > I don't like using EIO for this. EIO generally is returned if a hardware > error or an unexpected hardware condition occurs. How about -ENOMSG? Or > perhaps EPIPE? (As in: "the pipe containing events is gone"). There is no pipe (or at least wasn't; it's a queue or rather is implemented as a fifo :)) so of the two I prefer -ENOMSG. What would you think of -ENODATA or -EPERM (which is used e.g. when writing read-only controls)?
Hans Verkuil wrote: ... >>>> + if (!subscribed) { >>>> + ret = -EIO; >>> >>> Shouldn't this be -ENOENT? >> >> If I use -ENOENT, having no events subscribed is indistinguishable >> form no events pending condition. Combine that with using select(2), >> and you can no longer distinguish having no events subscribed from >> the case where you got an event but someone else (another thread or >> process) dequeued it. > > OK, but then your commit message is out of sync with the actual patch since > the commit log says ENOENT. > >> -EIO makes that explicit --- this also mirrors the behaviour of >> VIDIOC_DQBUF. (And it must be documented as well, which is missing >> from the patch currently.) > > I don't like using EIO for this. EIO generally is returned if a hardware > error or an unexpected hardware condition occurs. How about -ENOMSG? Or > perhaps EPIPE? (As in: "the pipe containing events is gone"). Thinking about this some more, -ENOENT is probably what we should return. *But* when there are no events to dequeue, we should instead return -EAGAIN (i.e. EWOULDBLOCK) which VIDIOC_DQBUF also uses. However I'm not sure if anything depends on -ENOENT currently (probably not really) so changing this might require some consideration. No error codes are currently defined for VIDIOC_DQEVENT; was planning to fix that while we're at this.
Hi Sakari, On Wednesday 02 October 2013 16:45:16 Sakari Ailus wrote: > Dequeueing events was is entirely possible even if none are subscribed, was or is ? :-) > leading to sleeping indefinitely. Fix this by returning -ENOENT when no > events are subscribed. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-event.c > b/drivers/media/v4l2-core/v4l2-event.c index b53897e..553a800 100644 > --- a/drivers/media/v4l2-core/v4l2-event.c > +++ b/drivers/media/v4l2-core/v4l2-event.c > @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct > v4l2_event *event, mutex_unlock(fh->vdev->lock); > > do { > - ret = wait_event_interruptible(fh->wait, > - fh->navailable != 0); > + bool subscribed; > + ret = wait_event_interruptible( > + fh->wait, > + fh->navailable != 0 || > + !(subscribed = v4l2_event_has_subscribed(fh))); > if (ret < 0) > break; > + if (!subscribed) { > + ret = -EIO; > + break; > + } > > ret = __v4l2_event_dequeue(fh, event); > } while (ret == -ENOENT);
Laurent Pinchart wrote: > Hi Sakari, > > On Wednesday 02 October 2013 16:45:16 Sakari Ailus wrote: >> Dequeueing events was is entirely possible even if none are subscribed, > > was or is ? :-) Was. :-) I'll fix that.
On 10/02/13 16:45, Sakari Ailus wrote: > Hi Hans, > > Hans Verkuil wrote: >> On 10/02/13 16:18, Sakari Ailus wrote: >>> Hi Hans, >>> >>> Thanks for the comments! >>> >>> Hans Verkuil wrote: >>>> On 10/02/13 15:45, Sakari Ailus wrote: >>>>> Dequeueing events was is entirely possible even if none are subscribed, >>>>> leading to sleeping indefinitely. Fix this by returning -ENOENT when no >>>>> events are subscribed. >>>>> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- >>>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-event.c >>>>> b/drivers/media/v4l2-core/v4l2-event.c >>>>> index b53897e..553a800 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-event.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-event.c >>>>> @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct >>>>> v4l2_event *event, >>>>> mutex_unlock(fh->vdev->lock); >>>>> >>>>> do { >>>>> - ret = wait_event_interruptible(fh->wait, >>>>> - fh->navailable != 0); >>>>> + bool subscribed; >>>> >>>> Can you add an empty line here? >>> >>> Sure. >>> >>>>> + ret = wait_event_interruptible( >>>>> + fh->wait, >>>>> + fh->navailable != 0 || >>>>> + !(subscribed = v4l2_event_has_subscribed(fh))); >>>>> if (ret < 0) >>>>> break; >>>>> + if (!subscribed) { >>>>> + ret = -EIO; >>>> >>>> Shouldn't this be -ENOENT? >>> >>> If I use -ENOENT, having no events subscribed is indistinguishable >>> form no events pending condition. Combine that with using select(2), >>> and you can no longer distinguish having no events subscribed from >>> the case where you got an event but someone else (another thread or >>> process) dequeued it. >> >> OK, but then your commit message is out of sync with the actual patch since >> the commit log says ENOENT. > > Right. The error code was the last thing I changed before sending the > patch, and I ignored it was also present in the commit message. :-P > >>> -EIO makes that explicit --- this also mirrors the behaviour of >>> VIDIOC_DQBUF. (And it must be documented as well, which is missing >>> from the patch currently.) >> >> I don't like using EIO for this. EIO generally is returned if a hardware >> error or an unexpected hardware condition occurs. How about -ENOMSG? Or >> perhaps EPIPE? (As in: "the pipe containing events is gone"). > > There is no pipe (or at least wasn't; it's a queue or rather is > implemented as a fifo :)) so of the two I prefer -ENOMSG. What would > you think of -ENODATA or -EPERM (which is used e.g. when writing > read-only controls)? > I don't like ENODATA, mostly because it is so close in meaning to ENOENT. EPERM would work for me. It's probably a bit better than ENOMSG. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/02/13 16:49, Sakari Ailus wrote: > Hans Verkuil wrote: > ... >>>>> + if (!subscribed) { >>>>> + ret = -EIO; >>>> >>>> Shouldn't this be -ENOENT? >>> >>> If I use -ENOENT, having no events subscribed is indistinguishable >>> form no events pending condition. Combine that with using select(2), >>> and you can no longer distinguish having no events subscribed from >>> the case where you got an event but someone else (another thread or >>> process) dequeued it. >> >> OK, but then your commit message is out of sync with the actual patch since >> the commit log says ENOENT. >> >>> -EIO makes that explicit --- this also mirrors the behaviour of >>> VIDIOC_DQBUF. (And it must be documented as well, which is missing >>> from the patch currently.) >> >> I don't like using EIO for this. EIO generally is returned if a hardware >> error or an unexpected hardware condition occurs. How about -ENOMSG? Or >> perhaps EPIPE? (As in: "the pipe containing events is gone"). > > Thinking about this some more, -ENOENT is probably what we should > return. *But* when there are no events to dequeue, we should instead > return -EAGAIN (i.e. EWOULDBLOCK) which VIDIOC_DQBUF also uses. > > However I'm not sure if anything depends on -ENOENT currently > (probably not really) so changing this might require some > consideration. No error codes are currently defined for > VIDIOC_DQEVENT; was planning to fix that while we're at this. > Urgh, this is messy. In non-blocking mode DQEVENT should indeed return -EAGAIN if you have subscribed events but no events are pending. If you have no subscribed events, then -ENOENT would be IMHO the most suitable return value. This means that DQEVENT's behavior changes in the non-blocking case. On the other hand, this is actually what you would expect based on the EAGAIN description in the spec: "It is also returned when the ioctl would need to wait for an event, but the device was opened in non-blocking mode." That said, I don't think we can change it. It's been around for too long and you have no idea how it is used in embedded systems that are out there (and that's where you would see this used in practice). I would just document the ENOENT error code (perhaps with a note that it should have been EAGAIN) and add a new error (EPERM?) for when no events are subscribed. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c index b53897e..553a800 100644 --- a/drivers/media/v4l2-core/v4l2-event.c +++ b/drivers/media/v4l2-core/v4l2-event.c @@ -77,10 +77,17 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, mutex_unlock(fh->vdev->lock); do { - ret = wait_event_interruptible(fh->wait, - fh->navailable != 0); + bool subscribed; + ret = wait_event_interruptible( + fh->wait, + fh->navailable != 0 || + !(subscribed = v4l2_event_has_subscribed(fh))); if (ret < 0) break; + if (!subscribed) { + ret = -EIO; + break; + } ret = __v4l2_event_dequeue(fh, event); } while (ret == -ENOENT);
Dequeueing events was is entirely possible even if none are subscribed, leading to sleeping indefinitely. Fix this by returning -ENOENT when no events are subscribed. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-event.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)