Hi Mauro,
> Em 20-09-2010 18:37, Hans Verkuil escreveu:
>> Hi all,
>>
>> I've made a first version of the core locking patches available here:
>>
>> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/test
>>
>> I'm actually surprised how trivial the patches are. Which makes me
>> wonder if
>> I am overlooking something, it feels too easy.
>>
>> One thing I did not yet have time to analyze fully is if it is really OK
>> to
>> unlock/relock the vdev_lock in videobuf_waiton. I hope it is, because
>> without
>> this another thread will find it impossible to access the video node
>> while it
>> is in waiton.
>>
>> Currently I've only tested with vivi. I hope to be able to spend more
>> time
>> this week for a more thorough analysis and converting a few more drivers
>> to
>> this.
>>
>> In the meantime, please feel free to shoot at this code!
>
> Hi Hans,
>
> This patch will likely break most drivers:
>
> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=commitdiff;h=d1ca35f3e69d909a958eb1cf8c75dd1c0bb2a98c
This was indeed something I wanted to review more closely.
> In the case of events and videobuf_waiton, it doesn't seem to be safe to
> just
> unlock when waiting for an event.
>
> For example, in the case of videobuf_waiton, the code for it is:
>
> #define WAITON_CONDITION (vb->state != VIDEOBUF_ACTIVE &&\
> vb->state != VIDEOBUF_QUEUED)
> int videobuf_waiton(struct videobuf_buffer *vb, int non_blocking, int
> intr)
> {
> MAGIC_CHECK(vb->magic, MAGIC_BUFFER);
>
> if (non_blocking) {
> if (WAITON_CONDITION)
> return 0;
> else
> return -EAGAIN;
> }
>
> if (intr)
> return wait_event_interruptible(vb->done, WAITON_CONDITION);
> else
> wait_event(vb->done, WAITON_CONDITION);
>
> return 0;
> }
>
> When called internally, it have the vb mutex_locked, while, when called
> externally, it
> doesn't.
>
> By looking on other parts where vb->done is protected, like on
> videobuf_queue_cancel:
>
> spin_lock_irqsave(q->irqlock, flags);
> for (i = 0; i < VIDEO_MAX_FRAME; i++) {
> if (NULL == q->bufs[i])
> continue;
> if (q->bufs[i]->state == VIDEOBUF_QUEUED) {
> list_del(&q->bufs[i]->queue);
> q->bufs[i]->state = VIDEOBUF_ERROR;
> wake_up_all(&q->bufs[i]->done);
> }
> }
> spin_unlock_irqrestore(q->irqlock, flags);
>
> It is clear that vb state is protected by a spinlock, and not by a mutex.
> Using a mutex
> there makes no sense at all. Instead of touching a mutex, callers of this
> function should
> be reviewed to not call a mutex.
>
> So, the better approach for videobuf_waiton would be to protect it with a
> spinlock.
Sounds reasonable.
> Also, your patches assume that no driver will touch at vdev lock before
> calling videobuf_waiton().
> This seems to be a risky assumption. So, the better would be to define it
> as:
>
> static int is_state_active_or_queued(struct videobuf_buffer *vb, struct
> videobuf_queue *q, )
> {
> bool rc;
>
> spin_lock_irqsave(q->irqlock, flags);
> rc = (vb->state != VIDEOBUF_ACTIVE) && (vb->state != VIDEOBUF_QUEUED));
> spin_unlock_irqrestore(q->irqlock, flags);
>
> return rc;
> };
>
> int videobuf_waiton(struct videobuf_queue *q, struct videobuf_buffer *vb,
> int non_blocking, int intr)
> {
> rc = 0;
> bool is_vdev_locked;
> MAGIC_CHECK(vb->magic, MAGIC_BUFFER);
>
> /*
> * If there's nothing to wait, just return
> */
> if (is_state_active_or_queued(vb, q))
> return 0;
>
> if (non_blocking)
> return -EAGAIN;
>
> /*
> * Need to sleep in order to wait for videobufs to complete.
> * It is not a good idea to sleep while waiting for an event with the
> dev
> lock hold,
> * as it will block any other access to the device. Just unlock it while
> waiting,
> * locking it again at the end.
> */
>
> is_vdev_locked = (q->vdev_lock && mutex_is_locked(q->vdev_lock)) ? true
> : false;
> if (is_vdev_locked)
> mutex_unlock(q->vdev_lock);
> if (intr)
> return wait_event_interruptible(vb->done,
> is_state_active_or_queued(vb,
> q));
This obviously needs to save the return value and continue to make sure
the lock is taken again.
> else
> wait_event(vb->done, is_state_active_or_queued(vb, q));
> if (is_vdev_locked)
> mutex_lock(q->vdev_lock);
>
> return 0;
> }
Agreed. Thanks for reviewing this, it was the one patch that I knew I had
to look into more closely. I'll incorporate your changes.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
