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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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)?

-- 
Cheers,

Sakari Ailus
sakari.ailus@linux.intel.com