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 <bphilips@suse.de>
Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org>
diff --git a/drivers/media/common/saa7146_video.c b/drivers/media/common/saa7146_video.c
index f245a3b..7cc4213 100644
--- a/drivers/media/common/saa7146_video.c
+++ b/drivers/media/common/saa7146_video.c
@@ -1440,10 +1440,7 @@
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 89a44f1..de2f56b 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -141,6 +141,7 @@
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 @@
return 0;
}
+/* Locking: Caller holds q->lock */
void videobuf_queue_cancel(struct videobuf_queue *q)
{
unsigned long flags=0;
@@ -208,6 +210,7 @@
/* --------------------------------------------------------------------- */
+/* 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 @@
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 @@
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 @@
}
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 @@
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_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 @@
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 @@
return retval;
}
+/* Locking: Caller holds q->lock */
int videobuf_read_start(struct videobuf_queue *q)
{
enum v4l2_field field;
@@ -705,7 +820,7 @@
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 @@
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 @@
}
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 @@
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_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 0fa5d59..4fd5d0e 100644
--- a/include/media/videobuf-core.h
+++ b/include/media/videobuf-core.h
@@ -208,6 +208,8 @@
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,
