From 19bc5133dae9562e8824ef101464061f9854c1d8 Mon Sep 17 00:00:00 2001 From: Brandon Philips Date: Tue, 13 Nov 2007 20:05:38 -0300 Subject: [PATCH] V4L/DVB (6601): V4L: videobuf-core locking fixes and comments - Add comments to functions that require that caller hold q->lock - Add __videobuf_mmap_free that doesn't hold q->lock for use within videobuf - Add locking to videobuf_mmap_free - Fix linux/drivers/media/common/saa7146_video.c which was holding lock around videobuf_read_stop - Add locking to functions that operate on a queue - Add videobuf_stop to take care of stopping in both the read and stream case TODO: bttv still has an unsafe call to videobuf_queue_is_busy Signed-off-by: Brandon Philips Signed-off-by: Mauro Carvalho Chehab --- drivers/media/common/saa7146_video.c | 5 +- drivers/media/video/videobuf-core.c | 247 +++++++++++++++++---------- include/media/videobuf-core.h | 2 + 3 files changed, 161 insertions(+), 93 deletions(-) diff --git a/drivers/media/common/saa7146_video.c b/drivers/media/common/saa7146_video.c index f245a3b2ef..7cc4213ba5 100644 --- a/drivers/media/common/saa7146_video.c +++ b/drivers/media/common/saa7146_video.c @@ -1440,10 +1440,7 @@ static void video_close(struct saa7146_dev *dev, struct file *file) err = saa7146_stop_preview(fh); } - // release all capture buffers - mutex_lock(&q->lock); - videobuf_read_stop(q); - mutex_unlock(&q->lock); + videobuf_stop(q); /* hmm, why is this function declared void? */ /* return err */ diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c index 89a44f16f0..de2f56b191 100644 --- a/drivers/media/video/videobuf-core.c +++ b/drivers/media/video/videobuf-core.c @@ -141,6 +141,7 @@ void videobuf_queue_core_init(struct videobuf_queue* q, INIT_LIST_HEAD(&q->stream); } +/* Locking: Only usage in bttv unsafe find way to remove */ int videobuf_queue_is_busy(struct videobuf_queue *q) { int i; @@ -178,6 +179,7 @@ int videobuf_queue_is_busy(struct videobuf_queue *q) return 0; } +/* Locking: Caller holds q->lock */ void videobuf_queue_cancel(struct videobuf_queue *q) { unsigned long flags=0; @@ -208,6 +210,7 @@ void videobuf_queue_cancel(struct videobuf_queue *q) /* --------------------------------------------------------------------- */ +/* Locking: Caller holds q->lock */ enum v4l2_field videobuf_next_field(struct videobuf_queue *q) { enum v4l2_field field = q->field; @@ -226,6 +229,7 @@ enum v4l2_field videobuf_next_field(struct videobuf_queue *q) return field; } +/* Locking: Caller holds q->lock */ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b, struct videobuf_buffer *vb, enum v4l2_buf_type type) { @@ -281,20 +285,108 @@ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b, b->sequence = vb->field_count >> 1; } +/* Locking: Caller holds q->lock */ +static int __videobuf_mmap_free(struct videobuf_queue *q) +{ + int i; + int rc; + + if (!q) + return 0; + + MAGIC_CHECK(q->int_ops->magic,MAGIC_QTYPE_OPS); + + rc = CALL(q,mmap_free,q); + if (rc<0) + return rc; + + for (i = 0; i < VIDEO_MAX_FRAME; i++) { + if (NULL == q->bufs[i]) + continue; + q->ops->buf_release(q,q->bufs[i]); + kfree(q->bufs[i]); + q->bufs[i] = NULL; + } + + return rc; +} + +int videobuf_mmap_free(struct videobuf_queue *q) +{ + int ret; + mutex_lock(&q->lock); + ret = __videobuf_mmap_free(q); + mutex_unlock(&q->lock); + return ret; +} + +/* Locking: Caller holds q->lock */ +static int __videobuf_mmap_setup(struct videobuf_queue *q, + unsigned int bcount, unsigned int bsize, + enum v4l2_memory memory) +{ + unsigned int i; + int err; + + MAGIC_CHECK(q->int_ops->magic,MAGIC_QTYPE_OPS); + + err = __videobuf_mmap_free(q); + if (0 != err) + return err; + + /* Allocate and initialize buffers */ + for (i = 0; i < bcount; i++) { + q->bufs[i] = videobuf_alloc(q); + + if (q->bufs[i] == NULL) + break; + + q->bufs[i]->i = i; + q->bufs[i]->input = UNSET; + q->bufs[i]->memory = memory; + q->bufs[i]->bsize = bsize; + switch (memory) { + case V4L2_MEMORY_MMAP: + q->bufs[i]->boff = bsize * i; + break; + case V4L2_MEMORY_USERPTR: + case V4L2_MEMORY_OVERLAY: + /* nothing */ + break; + } + } + + if (!i) + return -ENOMEM; + + dprintk(1,"mmap setup: %d buffers, %d bytes each\n", + i, bsize); + + return i; +} + +int videobuf_mmap_setup(struct videobuf_queue *q, + unsigned int bcount, unsigned int bsize, + enum v4l2_memory memory) +{ + int ret; + mutex_lock(&q->lock); + ret = __videobuf_mmap_setup(q, bcount, bsize, memory); + mutex_unlock(&q->lock); + return ret; +} + int videobuf_reqbufs(struct videobuf_queue *q, struct v4l2_requestbuffers *req) { unsigned int size,count; int retval; - if (req->type != q->type) { - dprintk(1,"reqbufs: queue type invalid\n"); - return -EINVAL; - } if (req->count < 1) { dprintk(1,"reqbufs: count invalid (%d)\n",req->count); return -EINVAL; } + if (req->memory != V4L2_MEMORY_MMAP && req->memory != V4L2_MEMORY_USERPTR && req->memory != V4L2_MEMORY_OVERLAY) { @@ -303,6 +395,12 @@ int videobuf_reqbufs(struct videobuf_queue *q, } mutex_lock(&q->lock); + if (req->type != q->type) { + dprintk(1,"reqbufs: queue type invalid\n"); + retval = -EINVAL; + goto done; + } + if (q->streaming) { dprintk(1,"reqbufs: streaming already exists\n"); retval = -EBUSY; @@ -323,7 +421,7 @@ int videobuf_reqbufs(struct videobuf_queue *q, dprintk(1,"reqbufs: bufs=%d, size=0x%x [%d pages total]\n", count, size, (count*size)>>PAGE_SHIFT); - retval = videobuf_mmap_setup(q,count,size,req->memory); + retval = __videobuf_mmap_setup(q,count,size,req->memory); if (retval < 0) { dprintk(1,"reqbufs: mmap setup returned %d\n",retval); goto done; @@ -338,20 +436,28 @@ int videobuf_reqbufs(struct videobuf_queue *q, int videobuf_querybuf(struct videobuf_queue *q, struct v4l2_buffer *b) { + int ret = -EINVAL; + + mutex_lock(&q->lock); if (unlikely(b->type != q->type)) { dprintk(1,"querybuf: Wrong type.\n"); - return -EINVAL; + goto done; } if (unlikely(b->index < 0 || b->index >= VIDEO_MAX_FRAME)) { dprintk(1,"querybuf: index out of range.\n"); - return -EINVAL; + goto done; } if (unlikely(NULL == q->bufs[b->index])) { dprintk(1,"querybuf: buffer is null.\n"); - return -EINVAL; + goto done; } + videobuf_status(q,b,q->bufs[b->index],q->type); - return 0; + + ret = 0; +done: + mutex_unlock(&q->lock); + return ret; } int videobuf_qbuf(struct videobuf_queue *q, @@ -541,22 +647,30 @@ int videobuf_streamon(struct videobuf_queue *q) return retval; } -int videobuf_streamoff(struct videobuf_queue *q) +/* Locking: Caller holds q->lock */ +static int __videobuf_streamoff(struct videobuf_queue *q) { - int retval = -EINVAL; - - mutex_lock(&q->lock); if (!q->streaming) - goto done; + return -EINVAL; + videobuf_queue_cancel(q); q->streaming = 0; - retval = 0; - done: + return 0; +} + +int videobuf_streamoff(struct videobuf_queue *q) +{ + int retval; + + mutex_lock(&q->lock); + retval = __videobuf_streamoff(q); mutex_unlock(&q->lock); + return retval; } +/* Locking: Caller holds q->lock */ static ssize_t videobuf_read_zerocopy(struct videobuf_queue *q, char __user *data, size_t count, loff_t *ppos) @@ -691,6 +805,7 @@ ssize_t videobuf_read_one(struct videobuf_queue *q, return retval; } +/* Locking: Caller holds q->lock */ int videobuf_read_start(struct videobuf_queue *q) { enum v4l2_field field; @@ -705,7 +820,7 @@ int videobuf_read_start(struct videobuf_queue *q) count = VIDEO_MAX_FRAME; size = PAGE_ALIGN(size); - err = videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR); + err = __videobuf_mmap_setup(q, count, size, V4L2_MEMORY_USERPTR); if (err < 0) return err; @@ -728,12 +843,13 @@ int videobuf_read_start(struct videobuf_queue *q) return 0; } -void videobuf_read_stop(struct videobuf_queue *q) +static void __videobuf_read_stop(struct videobuf_queue *q) { int i; + videobuf_queue_cancel(q); - videobuf_mmap_free(q); + __videobuf_mmap_free(q); INIT_LIST_HEAD(&q->stream); for (i = 0; i < VIDEO_MAX_FRAME; i++) { if (NULL == q->bufs[i]) @@ -743,8 +859,30 @@ void videobuf_read_stop(struct videobuf_queue *q) } q->read_buf = NULL; q->reading = 0; + +} + +void videobuf_read_stop(struct videobuf_queue *q) +{ + mutex_lock(&q->lock); + __videobuf_read_stop(q); + mutex_unlock(&q->lock); +} + +void videobuf_stop(struct videobuf_queue *q) +{ + mutex_lock(&q->lock); + + if (q->streaming) + __videobuf_streamoff(q); + + if (q->reading) + __videobuf_read_stop(q); + + mutex_unlock(&q->lock); } + ssize_t videobuf_read_stream(struct videobuf_queue *q, char __user *data, size_t count, loff_t *ppos, int vbihack, int nonblocking) @@ -858,75 +996,6 @@ unsigned int videobuf_poll_stream(struct file *file, return rc; } -int videobuf_mmap_setup(struct videobuf_queue *q, - unsigned int bcount, unsigned int bsize, - enum v4l2_memory memory) -{ - unsigned int i; - int err; - - MAGIC_CHECK(q->int_ops->magic,MAGIC_QTYPE_OPS); - - err = videobuf_mmap_free(q); - if (0 != err) - return err; - - /* Allocate and initialize buffers */ - for (i = 0; i < bcount; i++) { - q->bufs[i] = videobuf_alloc(q); - - if (q->bufs[i] == NULL) - break; - - q->bufs[i]->i = i; - q->bufs[i]->input = UNSET; - q->bufs[i]->memory = memory; - q->bufs[i]->bsize = bsize; - switch (memory) { - case V4L2_MEMORY_MMAP: - q->bufs[i]->boff = bsize * i; - break; - case V4L2_MEMORY_USERPTR: - case V4L2_MEMORY_OVERLAY: - /* nothing */ - break; - } - } - - if (!i) - return -ENOMEM; - - dprintk(1,"mmap setup: %d buffers, %d bytes each\n", - i, bsize); - - return i; -} - -int videobuf_mmap_free(struct videobuf_queue *q) -{ - int i; - int rc; - - if (!q) - return 0; - - MAGIC_CHECK(q->int_ops->magic,MAGIC_QTYPE_OPS); - - rc = CALL(q,mmap_free,q); - if (rc<0) - return rc; - - for (i = 0; i < VIDEO_MAX_FRAME; i++) { - if (NULL == q->bufs[i]) - continue; - q->ops->buf_release(q,q->bufs[i]); - kfree(q->bufs[i]); - q->bufs[i] = NULL; - } - - return rc; -} - int videobuf_mmap_mapper(struct videobuf_queue *q, struct vm_area_struct *vma) { @@ -989,8 +1058,8 @@ EXPORT_SYMBOL_GPL(videobuf_dqbuf); EXPORT_SYMBOL_GPL(videobuf_streamon); EXPORT_SYMBOL_GPL(videobuf_streamoff); -EXPORT_SYMBOL_GPL(videobuf_read_start); EXPORT_SYMBOL_GPL(videobuf_read_stop); +EXPORT_SYMBOL_GPL(videobuf_stop); EXPORT_SYMBOL_GPL(videobuf_read_stream); EXPORT_SYMBOL_GPL(videobuf_read_one); EXPORT_SYMBOL_GPL(videobuf_poll_stream); diff --git a/include/media/videobuf-core.h b/include/media/videobuf-core.h index 0fa5d59125..4fd5d0eaa9 100644 --- a/include/media/videobuf-core.h +++ b/include/media/videobuf-core.h @@ -208,6 +208,8 @@ int videobuf_cgmbuf(struct videobuf_queue *q, int videobuf_streamon(struct videobuf_queue *q); int videobuf_streamoff(struct videobuf_queue *q); +void videobuf_stop(struct videobuf_queue *q); + int videobuf_read_start(struct videobuf_queue *q); void videobuf_read_stop(struct videobuf_queue *q); ssize_t videobuf_read_stream(struct videobuf_queue *q, -- 2.39.5