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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
