Re: [PATCH v5 4/6] V4L: Events: Add backend

[Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>]

Aguirre, Sergio wrote:
> Heippa!

Hi, Sergio!

Your lines seem to be over 80 characters long. :I

...
>> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>> +{
>> +	struct v4l2_events *events = fh->events;
>> +	struct v4l2_kevent *kev;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +
>> +	if (list_empty(&events->available)) {
>> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +		return -ENOENT;
>> +	}
>> +
>> +	WARN_ON(events->navailable == 0);
> 
> I don't think the above warning will ever happen. Looks a bit over protective to me.

If it does it's a bug somewhere.

> Whenever you update your "events->available" list, you're holding the fh_lock spinlock, so there's no chance that the list of events would contan a different number of elents to what the navailable var is holding. Is it?
> 
> Please correct me if I'm missing something...

At the moment that is true as far as I see it. But if it's changed in
future chances are something goes wrong. It's a simple check but might
save some headaches.

> Or if you insist in checking, you could just have done this instead:
> 
> 	if (list_empty(&events->available) || (events->navailable == 0)) {
> 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 		return -ENOENT;
> 	}
> 
> As it doesn't make sense to proceed if navailable is zero, I believe...

It'd be a bug in the code so it must be WARN_ON().

I think the question is whether the check should be left there or removed.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com