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@xxxxxxxxxxxxxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
