On Sunday 21 February 2010 21:57:47 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > Hi Sakari,
>
> Hi Hans,
>
> And many thanks for the comments again!
>
> > Here are some more comments.
> >
> ...
> >> +
> >> +static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> >> +{
> >> + struct v4l2_events *events = fh->events;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >> +
> >> + while (!list_empty(&events->subscribed)) {
> >> + struct v4l2_subscribed_event *sev;
> >> +
> >> + sev = list_first_entry(&events->subscribed,
> >> + struct v4l2_subscribed_event, list);
> >> +
> >> + list_del(&sev->list);
> >> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >> + kfree(sev);
> >> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >> + }
> >> +
> >> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >> +}
> >
> > What about this:
> >
> > static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> > {
> > struct v4l2_events *events = fh->events;
> > struct v4l2_subscribed_event *sev;
> > unsigned long flags;
> >
> > do {
> > sev = NULL;
> >
> > spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> > if (!list_empty(&events->subscribed)) {
> > sev = list_first_entry(&events->subscribed,
> > struct v4l2_subscribed_event, list);
> > list_del(&sev->list);
> > }
> > spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> > kfree(sev);
> > } while (sev);
> > }
> >
> > This avoids the 'interleaved' locking which I never like.
>
> Can do. I don't see anything bad in that kind of locking, though. ;-)
Locking is hard to get right. So it is important to keep the locking code as
straightforward as possible. In the original code you really have to look
closely at the code to check that the locking is correct. In the rewritten
code it is much more obvious because of the simplified control flow.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
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
