Re: [RFC v2 4/4] v4l: events: Don't sleep in dequeue if none are subscribed

[Hans Verkuil <hverkuil@xs4all.nl>]

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