* [REVIEW PATCH for v3.15 0/4] v4l2 core sparse error/warning fixes
@ 2014-03-15 13:07 Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify Hans Verkuil
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Hans Verkuil @ 2014-03-15 13:07 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, pawel
These four patches fix sparse errors and warnings coming from the v4l2
core. There are more, but those seem to be problems with sparse itself (see
my posts from today on that topic).
Please take a good look at patch 3/4 in particular: that fixes sparse
errors introduced by my vb2 changes, and required some rework to get it
accepted by sparse without errors or warnings.
The rework required the introduction of more type-specific call_*op macros,
but on the other hand the fail_op macros could be dropped. Sort of one
step backwards, one step forwards.
If someone can think of a smarter solution for this, then please let me
know.
Regards,
Hans
^ permalink raw reply [flat|nested] 10+ messages in thread
* [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify
2014-03-15 13:07 [REVIEW PATCH for v3.15 0/4] v4l2 core sparse error/warning fixes Hans Verkuil
@ 2014-03-15 13:08 ` Hans Verkuil
2014-03-17 11:44 ` Laurent Pinchart
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 2/4] videobuf2-core: fix sparse errors Hans Verkuil
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2014-03-15 13:08 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, pawel, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The notify function is a void function, yet the v4l2_subdev_notify
define uses it in a ? : construction, which causes sparse warnings.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
include/media/v4l2-subdev.h | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 28f4d8c..0fbf669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -692,9 +692,11 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
(sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
/* Send a notification to v4l2_device. */
-#define v4l2_subdev_notify(sd, notification, arg) \
- ((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
- (sd)->v4l2_dev->notify((sd), (notification), (arg)))
+#define v4l2_subdev_notify(sd, notification, arg) \
+ do { \
+ if ((sd) && (sd)->v4l2_dev && (sd)->v4l2_dev->notify) \
+ (sd)->v4l2_dev->notify((sd), (notification), (arg)); \
+ } while (0)
#define v4l2_subdev_has_op(sd, o, f) \
((sd)->ops->o && (sd)->ops->o->f)
--
1.9.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [REVIEW PATCH for v3.15 2/4] videobuf2-core: fix sparse errors.
2014-03-15 13:07 [REVIEW PATCH for v3.15 0/4] v4l2 core sparse error/warning fixes Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify Hans Verkuil
@ 2014-03-15 13:08 ` Hans Verkuil
2014-03-17 10:20 ` Pawel Osciak
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 3/4] v4l2-common.h: remove __user annotation in struct v4l2_edid Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 4/4] v4l2-ioctl.c: fix sparse __user-related warnings Hans Verkuil
3 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2014-03-15 13:08 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, pawel, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Sparse generated a bunch of errors like this:
drivers/media/v4l2-core/videobuf2-core.c:2045:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:136:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:151:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:168:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:183:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:185:9: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:385:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1115:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1268:33: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1270:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1315:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1324:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1396:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1457:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1482:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1484:9: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1523:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1525:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1815:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1828:17: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1914:25: error: incompatible types in conditional expression (different base types)
drivers/media/v4l2-core/videobuf2-core.c:1944:9: error: incompatible types in conditional expression (different base types)
These are caused by the call*op defines which do something like this:
(ops->op) ? ops->op(args) : 0
which is OK as long as op is not a void function, because in that case one part
of the conditional expression returns void, the other an integer. Hence the sparse
errors.
I've replaced this by introducing three variants of the call_ macros:
call_*op for int returns, call_void_*op for void returns and call_ptr_*op for
pointer returns.
That's the bad news. The good news is that the fail_*op macros could be removed
since the call_*op macros now have enough information to determine if the op
succeeded or not and can increment the op counter only on success. This at least
makes it more robust w.r.t. future changes.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/videobuf2-core.c | 211 +++++++++++++++++++------------
1 file changed, 130 insertions(+), 81 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f9059bb..61149eb 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -36,58 +36,133 @@ module_param(debug, int, 0644);
#ifdef CONFIG_VIDEO_ADV_DEBUG
/*
- * If advanced debugging is on, then count how often each op is called,
- * which can either be per-buffer or per-queue.
+ * If advanced debugging is on, then count how often each op is called
+ * sucessfully, which can either be per-buffer or per-queue.
*
- * If the op failed then the 'fail_' variant is called to decrease the
- * counter. That makes it easy to check that the 'init' and 'cleanup'
+ * This makes it easy to check that the 'init' and 'cleanup'
* (and variations thereof) stay balanced.
*/
+#define log_memop(vb, op) \
+ dprintk(2, "call_memop(%p, %d, %s)%s\n", \
+ (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \
+ (vb)->vb2_queue->mem_ops->op ? "" : " (nop)")
+
#define call_memop(vb, op, args...) \
({ \
struct vb2_queue *_q = (vb)->vb2_queue; \
- dprintk(2, "call_memop(%p, %d, %s)%s\n", \
- _q, (vb)->v4l2_buf.index, #op, \
- _q->mem_ops->op ? "" : " (nop)"); \
+ int err; \
+ \
+ log_memop(vb, op); \
+ err = _q->mem_ops->op ? _q->mem_ops->op(args) : 0; \
+ if (!err) \
+ (vb)->cnt_mem_ ## op++; \
+ err; \
+})
+
+#define call_ptr_memop(vb, op, args...) \
+({ \
+ struct vb2_queue *_q = (vb)->vb2_queue; \
+ void *ptr; \
+ \
+ log_memop(vb, op); \
+ ptr = _q->mem_ops->op ? _q->mem_ops->op(args) : NULL; \
+ if (!IS_ERR_OR_NULL(ptr)) \
+ (vb)->cnt_mem_ ## op++; \
+ ptr; \
+})
+
+#define call_void_memop(vb, op, args...) \
+({ \
+ struct vb2_queue *_q = (vb)->vb2_queue; \
+ \
+ log_memop(vb, op); \
+ if (_q->mem_ops->op) \
+ _q->mem_ops->op(args); \
(vb)->cnt_mem_ ## op++; \
- _q->mem_ops->op ? _q->mem_ops->op(args) : 0; \
})
-#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
+
+#define log_qop(q, op) \
+ dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
+ (q)->ops->op ? "" : " (nop)")
#define call_qop(q, op, args...) \
({ \
- dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
- (q)->ops->op ? "" : " (nop)"); \
+ int err; \
+ \
+ log_qop(q, op); \
+ err = (q)->ops->op ? (q)->ops->op(args) : 0; \
+ if (!err) \
+ (q)->cnt_ ## op++; \
+ err; \
+})
+
+#define call_void_qop(q, op, args...) \
+({ \
+ log_qop(q, op); \
+ if ((q)->ops->op) \
+ (q)->ops->op(args); \
(q)->cnt_ ## op++; \
- (q)->ops->op ? (q)->ops->op(args) : 0; \
})
-#define fail_qop(q, op) ((q)->cnt_ ## op--)
+
+#define log_vb_qop(vb, op, args...) \
+ dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
+ (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \
+ (vb)->vb2_queue->ops->op ? "" : " (nop)")
#define call_vb_qop(vb, op, args...) \
({ \
- struct vb2_queue *_q = (vb)->vb2_queue; \
- dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
- _q, (vb)->v4l2_buf.index, #op, \
- _q->ops->op ? "" : " (nop)"); \
+ int err; \
+ \
+ log_vb_qop(vb, op); \
+ err = (vb)->vb2_queue->ops->op ? \
+ (vb)->vb2_queue->ops->op(args) : 0; \
+ if (!err) \
+ (vb)->cnt_ ## op++; \
+ err; \
+})
+
+#define call_void_vb_qop(vb, op, args...) \
+({ \
+ log_vb_qop(vb, op); \
+ if ((vb)->vb2_queue->ops->op) \
+ (vb)->vb2_queue->ops->op(args); \
(vb)->cnt_ ## op++; \
- _q->ops->op ? _q->ops->op(args) : 0; \
})
-#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
#else
#define call_memop(vb, op, args...) \
- ((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) : 0)
-#define fail_memop(vb, op)
+ ((vb)->vb2_queue->mem_ops->op ? \
+ (vb)->vb2_queue->mem_ops->op(args) : 0)
+
+#define call_ptr_memop(vb, op, args...) \
+ ((vb)->vb2_queue->mem_ops->op ? \
+ (vb)->vb2_queue->mem_ops->op(args) : NULL)
+
+#define call_void_memop(vb, op, args...) \
+ do { \
+ if ((vb)->vb2_queue->mem_ops->op) \
+ (vb)->vb2_queue->mem_ops->op(args); \
+ } while (0)
#define call_qop(q, op, args...) \
((q)->ops->op ? (q)->ops->op(args) : 0)
-#define fail_qop(q, op)
+
+#define call_void_qop(q, op, args...) \
+ do { \
+ if ((q)->ops->op) \
+ (q)->ops->op(args); \
+ } while (0)
#define call_vb_qop(vb, op, args...) \
((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
-#define fail_vb_qop(vb, op)
+
+#define call_void_vb_qop(vb, op, args...) \
+ do { \
+ if ((vb)->vb2_queue->ops->op) \
+ (vb)->vb2_queue->ops->op(args); \
+ } while (0)
#endif
@@ -118,7 +193,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
- mem_priv = call_memop(vb, alloc, q->alloc_ctx[plane],
+ mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane],
size, q->gfp_flags);
if (IS_ERR_OR_NULL(mem_priv))
goto free;
@@ -130,10 +205,9 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
return 0;
free:
- fail_memop(vb, alloc);
/* Free already allocated memory if one of the allocations failed */
for (; plane > 0; --plane) {
- call_memop(vb, put, vb->planes[plane - 1].mem_priv);
+ call_void_memop(vb, put, vb->planes[plane - 1].mem_priv);
vb->planes[plane - 1].mem_priv = NULL;
}
@@ -148,7 +222,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
unsigned int plane;
for (plane = 0; plane < vb->num_planes; ++plane) {
- call_memop(vb, put, vb->planes[plane].mem_priv);
+ call_void_memop(vb, put, vb->planes[plane].mem_priv);
vb->planes[plane].mem_priv = NULL;
dprintk(3, "Freed plane %d of buffer %d\n", plane,
vb->v4l2_buf.index);
@@ -165,7 +239,7 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane) {
if (vb->planes[plane].mem_priv)
- call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
+ call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
vb->planes[plane].mem_priv = NULL;
}
}
@@ -180,9 +254,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
return;
if (p->dbuf_mapped)
- call_memop(vb, unmap_dmabuf, p->mem_priv);
+ call_void_memop(vb, unmap_dmabuf, p->mem_priv);
- call_memop(vb, detach_dmabuf, p->mem_priv);
+ call_void_memop(vb, detach_dmabuf, p->mem_priv);
dma_buf_put(p->dbuf);
memset(p, 0, sizeof(*p));
}
@@ -305,7 +379,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
if (ret) {
dprintk(1, "Buffer %d %p initialization"
" failed\n", buffer, vb);
- fail_vb_qop(vb, buf_init);
__vb2_buf_mem_free(vb);
kfree(vb);
break;
@@ -382,7 +455,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
struct vb2_buffer *vb = q->bufs[buffer];
if (vb && vb->planes[0].mem_priv)
- call_vb_qop(vb, buf_cleanup, vb);
+ call_void_vb_qop(vb, buf_cleanup, vb);
}
/* Release video buffer memory */
@@ -837,10 +910,8 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
*/
ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes,
q->plane_sizes, q->alloc_ctx);
- if (ret) {
- fail_qop(q, queue_setup);
+ if (ret)
return ret;
- }
/* Finally, allocate buffers and video memory */
allocated_buffers = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
@@ -864,8 +935,6 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
ret = call_qop(q, queue_setup, q, NULL, &num_buffers,
&num_planes, q->plane_sizes, q->alloc_ctx);
- if (ret)
- fail_qop(q, queue_setup);
if (!ret && allocated_buffers < num_buffers)
ret = -ENOMEM;
@@ -950,10 +1019,8 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
*/
ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
&num_planes, q->plane_sizes, q->alloc_ctx);
- if (ret) {
- fail_qop(q, queue_setup);
+ if (ret)
return ret;
- }
/* Finally, allocate buffers and video memory */
allocated_buffers = __vb2_queue_alloc(q, create->memory, num_buffers,
@@ -975,8 +1042,6 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
*/
ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
&num_planes, q->plane_sizes, q->alloc_ctx);
- if (ret)
- fail_qop(q, queue_setup);
if (!ret && allocated_buffers < num_buffers)
ret = -ENOMEM;
@@ -1038,7 +1103,7 @@ void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
- return call_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
+ return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
}
EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
@@ -1059,7 +1124,7 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
return NULL;
- return call_memop(vb, cookie, vb->planes[plane_no].mem_priv);
+ return call_ptr_memop(vb, cookie, vb->planes[plane_no].mem_priv);
}
EXPORT_SYMBOL_GPL(vb2_plane_cookie);
@@ -1112,7 +1177,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
/* sync buffers */
for (plane = 0; plane < vb->num_planes; ++plane)
- call_memop(vb, finish, vb->planes[plane].mem_priv);
+ call_void_memop(vb, finish, vb->planes[plane].mem_priv);
/* Add the buffer to the done buffers list */
spin_lock_irqsave(&q->done_lock, flags);
@@ -1265,22 +1330,21 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
if (vb->planes[plane].mem_priv) {
if (!reacquired) {
reacquired = true;
- call_vb_qop(vb, buf_cleanup, vb);
+ call_void_vb_qop(vb, buf_cleanup, vb);
}
- call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
+ call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
}
vb->planes[plane].mem_priv = NULL;
memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
/* Acquire each plane's memory */
- mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
+ mem_priv = call_ptr_memop(vb, get_userptr, q->alloc_ctx[plane],
planes[plane].m.userptr,
planes[plane].length, write);
if (IS_ERR_OR_NULL(mem_priv)) {
dprintk(1, "qbuf: failed acquiring userspace "
"memory for plane %d\n", plane);
- fail_memop(vb, get_userptr);
ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
goto err;
}
@@ -1303,7 +1367,6 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
ret = call_vb_qop(vb, buf_init, vb);
if (ret) {
dprintk(1, "qbuf: buffer initialization failed\n");
- fail_vb_qop(vb, buf_init);
goto err;
}
}
@@ -1311,8 +1374,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
ret = call_vb_qop(vb, buf_prepare, vb);
if (ret) {
dprintk(1, "qbuf: buffer preparation failed\n");
- fail_vb_qop(vb, buf_prepare);
- call_vb_qop(vb, buf_cleanup, vb);
+ call_void_vb_qop(vb, buf_cleanup, vb);
goto err;
}
@@ -1321,7 +1383,7 @@ err:
/* In case of errors, release planes that were already acquired */
for (plane = 0; plane < vb->num_planes; ++plane) {
if (vb->planes[plane].mem_priv)
- call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
+ call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
vb->planes[plane].mem_priv = NULL;
vb->v4l2_planes[plane].m.userptr = 0;
vb->v4l2_planes[plane].length = 0;
@@ -1335,13 +1397,8 @@ err:
*/
static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
{
- int ret;
-
__fill_vb2_buffer(vb, b, vb->v4l2_planes);
- ret = call_vb_qop(vb, buf_prepare, vb);
- if (ret)
- fail_vb_qop(vb, buf_prepare);
- return ret;
+ return call_vb_qop(vb, buf_prepare, vb);
}
/**
@@ -1393,7 +1450,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
if (!reacquired) {
reacquired = true;
- call_vb_qop(vb, buf_cleanup, vb);
+ call_void_vb_qop(vb, buf_cleanup, vb);
}
/* Release previously acquired memory if present */
@@ -1401,11 +1458,10 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
/* Acquire each plane's memory */
- mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
+ mem_priv = call_ptr_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
dbuf, planes[plane].length, write);
if (IS_ERR(mem_priv)) {
dprintk(1, "qbuf: failed to attach dmabuf\n");
- fail_memop(vb, attach_dmabuf);
ret = PTR_ERR(mem_priv);
dma_buf_put(dbuf);
goto err;
@@ -1424,7 +1480,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
if (ret) {
dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
plane);
- fail_memop(vb, map_dmabuf);
goto err;
}
vb->planes[plane].dbuf_mapped = 1;
@@ -1445,7 +1500,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
ret = call_vb_qop(vb, buf_init, vb);
if (ret) {
dprintk(1, "qbuf: buffer initialization failed\n");
- fail_vb_qop(vb, buf_init);
goto err;
}
}
@@ -1453,8 +1507,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
ret = call_vb_qop(vb, buf_prepare, vb);
if (ret) {
dprintk(1, "qbuf: buffer preparation failed\n");
- fail_vb_qop(vb, buf_prepare);
- call_vb_qop(vb, buf_cleanup, vb);
+ call_void_vb_qop(vb, buf_cleanup, vb);
goto err;
}
@@ -1479,9 +1532,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
/* sync buffers */
for (plane = 0; plane < vb->num_planes; ++plane)
- call_memop(vb, prepare, vb->planes[plane].mem_priv);
+ call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
- call_vb_qop(vb, buf_queue, vb);
+ call_void_vb_qop(vb, buf_queue, vb);
}
static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
@@ -1520,9 +1573,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
* mmap_sem and then takes the driver's lock again.
*/
mmap_sem = ¤t->mm->mmap_sem;
- call_qop(q, wait_prepare, q);
+ call_void_qop(q, wait_prepare, q);
down_read(mmap_sem);
- call_qop(q, wait_finish, q);
+ call_void_qop(q, wait_finish, q);
ret = __qbuf_userptr(vb, b);
@@ -1647,7 +1700,6 @@ static int vb2_start_streaming(struct vb2_queue *q)
if (!ret)
return 0;
- fail_qop(q, start_streaming);
dprintk(1, "qbuf: driver refused to start streaming\n");
if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
unsigned i;
@@ -1812,7 +1864,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
* become ready or for streamoff. Driver's lock is released to
* allow streamoff or qbuf to be called while waiting.
*/
- call_qop(q, wait_prepare, q);
+ call_void_qop(q, wait_prepare, q);
/*
* All locks have been released, it is safe to sleep now.
@@ -1825,7 +1877,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
* We need to reevaluate both conditions again after reacquiring
* the locks or return an error if one occurred.
*/
- call_qop(q, wait_finish, q);
+ call_void_qop(q, wait_finish, q);
if (ret) {
dprintk(1, "Sleep was interrupted\n");
return ret;
@@ -1911,7 +1963,7 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
for (i = 0; i < vb->num_planes; ++i) {
if (!vb->planes[i].dbuf_mapped)
continue;
- call_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
+ call_void_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
vb->planes[i].dbuf_mapped = 0;
}
}
@@ -1941,7 +1993,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
return -EINVAL;
}
- call_vb_qop(vb, buf_finish, vb);
+ call_void_vb_qop(vb, buf_finish, vb);
/* Fill buffer information for the userspace */
__fill_v4l2_buffer(vb, b);
@@ -2042,7 +2094,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
if (vb->state != VB2_BUF_STATE_DEQUEUED) {
vb->state = VB2_BUF_STATE_PREPARED;
- call_vb_qop(vb, buf_finish, vb);
+ call_void_vb_qop(vb, buf_finish, vb);
}
__vb2_dqbuf(vb);
}
@@ -2244,11 +2296,10 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
vb_plane = &vb->planes[eb->plane];
- dbuf = call_memop(vb, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
+ dbuf = call_ptr_memop(vb, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
if (IS_ERR_OR_NULL(dbuf)) {
dprintk(1, "Failed to export buffer %d, plane %d\n",
eb->index, eb->plane);
- fail_memop(vb, get_dmabuf);
return -EINVAL;
}
@@ -2341,10 +2392,8 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
}
ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
- if (ret) {
- fail_memop(vb, mmap);
+ if (ret)
return ret;
- }
dprintk(3, "Buffer %d, plane %d successfully mapped\n", buffer, plane);
return 0;
--
1.9.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [REVIEW PATCH for v3.15 3/4] v4l2-common.h: remove __user annotation in struct v4l2_edid
2014-03-15 13:07 [REVIEW PATCH for v3.15 0/4] v4l2 core sparse error/warning fixes Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 2/4] videobuf2-core: fix sparse errors Hans Verkuil
@ 2014-03-15 13:08 ` Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 4/4] v4l2-ioctl.c: fix sparse __user-related warnings Hans Verkuil
3 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2014-03-15 13:08 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, pawel, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The edid array is copied to kernelspace by the v4l2 core, so drivers
shouldn't see the __user annotation. This conforms to other structs like
v4l2_ext_controls where the data pointed to is copied to from user to
kernelspace.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
include/uapi/linux/v4l2-common.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/v4l2-common.h b/include/uapi/linux/v4l2-common.h
index 270db89..e9011cd 100644
--- a/include/uapi/linux/v4l2-common.h
+++ b/include/uapi/linux/v4l2-common.h
@@ -73,7 +73,7 @@ struct v4l2_edid {
__u32 start_block;
__u32 blocks;
__u32 reserved[5];
- __u8 __user *edid;
+ __u8 *edid;
};
#endif /* __V4L2_COMMON__ */
--
1.9.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [REVIEW PATCH for v3.15 4/4] v4l2-ioctl.c: fix sparse __user-related warnings
2014-03-15 13:07 [REVIEW PATCH for v3.15 0/4] v4l2 core sparse error/warning fixes Hans Verkuil
` (2 preceding siblings ...)
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 3/4] v4l2-common.h: remove __user annotation in struct v4l2_edid Hans Verkuil
@ 2014-03-15 13:08 ` Hans Verkuil
2014-03-17 11:59 ` Laurent Pinchart
3 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2014-03-15 13:08 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, pawel, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Drop the use of __user in the user_ptr variable since the v4l2 structs are
actually defined without __user, instead cast to a __user pointer only
there where it is really needed: in the copy_to/from_user calls.
Also remove unnecessary casts in check_array_args and replace a wrong
cast (void *) with the correct one (void **).
This fixes these sparse warnings:
drivers/media/v4l2-core/v4l2-ioctl.c:2284:35: warning: incorrect type in assignment (different address spaces)
drivers/media/v4l2-core/v4l2-ioctl.c:2301:35: warning: incorrect type in assignment (different address spaces)
drivers/media/v4l2-core/v4l2-ioctl.c:2319:35: warning: incorrect type in assignment (different address spaces)
drivers/media/v4l2-core/v4l2-ioctl.c:2386:57: warning: incorrect type in argument 4 (different address spaces)
drivers/media/v4l2-core/v4l2-ioctl.c:2420:29: warning: incorrect type in assignment (different address spaces)
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index d9113cc..3e0cf4f 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2260,7 +2260,7 @@ done:
}
static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
- void * __user *user_ptr, void ***kernel_ptr)
+ void **user_ptr, void ***kernel_ptr)
{
int ret = 0;
@@ -2276,8 +2276,8 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
ret = -EINVAL;
break;
}
- *user_ptr = (void __user *)buf->m.planes;
- *kernel_ptr = (void *)&buf->m.planes;
+ *user_ptr = buf->m.planes;
+ *kernel_ptr = (void **)&buf->m.planes;
*array_size = sizeof(struct v4l2_plane) * buf->length;
ret = 1;
}
@@ -2293,8 +2293,8 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
ret = -EINVAL;
break;
}
- *user_ptr = (void __user *)edid->edid;
- *kernel_ptr = (void *)&edid->edid;
+ *user_ptr = edid->edid;
+ *kernel_ptr = (void **)&edid->edid;
*array_size = edid->blocks * 128;
ret = 1;
}
@@ -2311,8 +2311,8 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
ret = -EINVAL;
break;
}
- *user_ptr = (void __user *)ctrls->controls;
- *kernel_ptr = (void *)&ctrls->controls;
+ *user_ptr = ctrls->controls;
+ *kernel_ptr = (void **)&ctrls->controls;
*array_size = sizeof(struct v4l2_ext_control)
* ctrls->count;
ret = 1;
@@ -2334,7 +2334,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
long err = -EINVAL;
bool has_array_args;
size_t array_size = 0;
- void __user *user_ptr = NULL;
+ void *user_ptr = NULL;
void **kernel_ptr = NULL;
/* Copy arguments into temp kernel buffer */
@@ -2395,7 +2395,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
if (NULL == mbuf)
goto out_array_args;
err = -EFAULT;
- if (copy_from_user(mbuf, user_ptr, array_size))
+ if (copy_from_user(mbuf, (void __user *)user_ptr, array_size))
goto out_array_args;
*kernel_ptr = mbuf;
}
@@ -2413,7 +2413,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
if (has_array_args) {
*kernel_ptr = user_ptr;
- if (copy_to_user(user_ptr, mbuf, array_size))
+ if (copy_to_user((void __user *)user_ptr, mbuf, array_size))
err = -EFAULT;
goto out_array_args;
}
--
1.9.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [REVIEW PATCH for v3.15 2/4] videobuf2-core: fix sparse errors.
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 2/4] videobuf2-core: fix sparse errors Hans Verkuil
@ 2014-03-17 10:20 ` Pawel Osciak
0 siblings, 0 replies; 10+ messages in thread
From: Pawel Osciak @ 2014-03-17 10:20 UTC (permalink / raw)
To: Hans Verkuil; +Cc: LMML, Laurent Pinchart, Hans Verkuil
Hi Hans,
No issues with the patch, apart from one typo in a comment, but it may
not be worth the reupload.
On Sat, Mar 15, 2014 at 10:08 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Sparse generated a bunch of errors like this:
>
> drivers/media/v4l2-core/videobuf2-core.c:2045:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:136:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:151:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:168:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:183:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:185:9: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:385:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1115:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1268:33: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1270:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1315:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1324:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1396:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1457:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1482:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1484:9: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1523:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1525:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1815:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1828:17: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1914:25: error: incompatible types in conditional expression (different base types)
> drivers/media/v4l2-core/videobuf2-core.c:1944:9: error: incompatible types in conditional expression (different base types)
>
> These are caused by the call*op defines which do something like this:
>
> (ops->op) ? ops->op(args) : 0
>
> which is OK as long as op is not a void function, because in that case one part
> of the conditional expression returns void, the other an integer. Hence the sparse
> errors.
>
> I've replaced this by introducing three variants of the call_ macros:
> call_*op for int returns, call_void_*op for void returns and call_ptr_*op for
> pointer returns.
>
> That's the bad news. The good news is that the fail_*op macros could be removed
> since the call_*op macros now have enough information to determine if the op
> succeeded or not and can increment the op counter only on success. This at least
> makes it more robust w.r.t. future changes.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Pawel Osciak <pawel@osciak.com>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 211 +++++++++++++++++++------------
> 1 file changed, 130 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index f9059bb..61149eb 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -36,58 +36,133 @@ module_param(debug, int, 0644);
> #ifdef CONFIG_VIDEO_ADV_DEBUG
>
> /*
> - * If advanced debugging is on, then count how often each op is called,
> - * which can either be per-buffer or per-queue.
> + * If advanced debugging is on, then count how often each op is called
> + * sucessfully, which can either be per-buffer or per-queue.
s/sucessfully/successfully/
> *
> - * If the op failed then the 'fail_' variant is called to decrease the
> - * counter. That makes it easy to check that the 'init' and 'cleanup'
> + * This makes it easy to check that the 'init' and 'cleanup'
> * (and variations thereof) stay balanced.
> */
>
> +#define log_memop(vb, op) \
> + dprintk(2, "call_memop(%p, %d, %s)%s\n", \
> + (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \
> + (vb)->vb2_queue->mem_ops->op ? "" : " (nop)")
> +
> #define call_memop(vb, op, args...) \
> ({ \
> struct vb2_queue *_q = (vb)->vb2_queue; \
> - dprintk(2, "call_memop(%p, %d, %s)%s\n", \
> - _q, (vb)->v4l2_buf.index, #op, \
> - _q->mem_ops->op ? "" : " (nop)"); \
> + int err; \
> + \
> + log_memop(vb, op); \
> + err = _q->mem_ops->op ? _q->mem_ops->op(args) : 0; \
> + if (!err) \
> + (vb)->cnt_mem_ ## op++; \
> + err; \
> +})
> +
> +#define call_ptr_memop(vb, op, args...) \
> +({ \
> + struct vb2_queue *_q = (vb)->vb2_queue; \
> + void *ptr; \
> + \
> + log_memop(vb, op); \
> + ptr = _q->mem_ops->op ? _q->mem_ops->op(args) : NULL; \
> + if (!IS_ERR_OR_NULL(ptr)) \
> + (vb)->cnt_mem_ ## op++; \
> + ptr; \
> +})
> +
> +#define call_void_memop(vb, op, args...) \
> +({ \
> + struct vb2_queue *_q = (vb)->vb2_queue; \
> + \
> + log_memop(vb, op); \
> + if (_q->mem_ops->op) \
> + _q->mem_ops->op(args); \
> (vb)->cnt_mem_ ## op++; \
> - _q->mem_ops->op ? _q->mem_ops->op(args) : 0; \
> })
> -#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
> +
> +#define log_qop(q, op) \
> + dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
> + (q)->ops->op ? "" : " (nop)")
>
> #define call_qop(q, op, args...) \
> ({ \
> - dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
> - (q)->ops->op ? "" : " (nop)"); \
> + int err; \
> + \
> + log_qop(q, op); \
> + err = (q)->ops->op ? (q)->ops->op(args) : 0; \
> + if (!err) \
> + (q)->cnt_ ## op++; \
> + err; \
> +})
> +
> +#define call_void_qop(q, op, args...) \
> +({ \
> + log_qop(q, op); \
> + if ((q)->ops->op) \
> + (q)->ops->op(args); \
> (q)->cnt_ ## op++; \
> - (q)->ops->op ? (q)->ops->op(args) : 0; \
> })
> -#define fail_qop(q, op) ((q)->cnt_ ## op--)
> +
> +#define log_vb_qop(vb, op, args...) \
> + dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
> + (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \
> + (vb)->vb2_queue->ops->op ? "" : " (nop)")
>
> #define call_vb_qop(vb, op, args...) \
> ({ \
> - struct vb2_queue *_q = (vb)->vb2_queue; \
> - dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
> - _q, (vb)->v4l2_buf.index, #op, \
> - _q->ops->op ? "" : " (nop)"); \
> + int err; \
> + \
> + log_vb_qop(vb, op); \
> + err = (vb)->vb2_queue->ops->op ? \
> + (vb)->vb2_queue->ops->op(args) : 0; \
> + if (!err) \
> + (vb)->cnt_ ## op++; \
> + err; \
> +})
> +
> +#define call_void_vb_qop(vb, op, args...) \
> +({ \
> + log_vb_qop(vb, op); \
> + if ((vb)->vb2_queue->ops->op) \
> + (vb)->vb2_queue->ops->op(args); \
> (vb)->cnt_ ## op++; \
> - _q->ops->op ? _q->ops->op(args) : 0; \
> })
> -#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
>
> #else
>
> #define call_memop(vb, op, args...) \
> - ((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) : 0)
> -#define fail_memop(vb, op)
> + ((vb)->vb2_queue->mem_ops->op ? \
> + (vb)->vb2_queue->mem_ops->op(args) : 0)
> +
> +#define call_ptr_memop(vb, op, args...) \
> + ((vb)->vb2_queue->mem_ops->op ? \
> + (vb)->vb2_queue->mem_ops->op(args) : NULL)
> +
> +#define call_void_memop(vb, op, args...) \
> + do { \
> + if ((vb)->vb2_queue->mem_ops->op) \
> + (vb)->vb2_queue->mem_ops->op(args); \
> + } while (0)
>
> #define call_qop(q, op, args...) \
> ((q)->ops->op ? (q)->ops->op(args) : 0)
> -#define fail_qop(q, op)
> +
> +#define call_void_qop(q, op, args...) \
> + do { \
> + if ((q)->ops->op) \
> + (q)->ops->op(args); \
> + } while (0)
>
> #define call_vb_qop(vb, op, args...) \
> ((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
> -#define fail_vb_qop(vb, op)
> +
> +#define call_void_vb_qop(vb, op, args...) \
> + do { \
> + if ((vb)->vb2_queue->ops->op) \
> + (vb)->vb2_queue->ops->op(args); \
> + } while (0)
>
> #endif
>
> @@ -118,7 +193,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> for (plane = 0; plane < vb->num_planes; ++plane) {
> unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
>
> - mem_priv = call_memop(vb, alloc, q->alloc_ctx[plane],
> + mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane],
> size, q->gfp_flags);
> if (IS_ERR_OR_NULL(mem_priv))
> goto free;
> @@ -130,10 +205,9 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>
> return 0;
> free:
> - fail_memop(vb, alloc);
> /* Free already allocated memory if one of the allocations failed */
> for (; plane > 0; --plane) {
> - call_memop(vb, put, vb->planes[plane - 1].mem_priv);
> + call_void_memop(vb, put, vb->planes[plane - 1].mem_priv);
> vb->planes[plane - 1].mem_priv = NULL;
> }
>
> @@ -148,7 +222,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
> unsigned int plane;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - call_memop(vb, put, vb->planes[plane].mem_priv);
> + call_void_memop(vb, put, vb->planes[plane].mem_priv);
> vb->planes[plane].mem_priv = NULL;
> dprintk(3, "Freed plane %d of buffer %d\n", plane,
> vb->v4l2_buf.index);
> @@ -165,7 +239,7 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> if (vb->planes[plane].mem_priv)
> - call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> + call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> vb->planes[plane].mem_priv = NULL;
> }
> }
> @@ -180,9 +254,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
> return;
>
> if (p->dbuf_mapped)
> - call_memop(vb, unmap_dmabuf, p->mem_priv);
> + call_void_memop(vb, unmap_dmabuf, p->mem_priv);
>
> - call_memop(vb, detach_dmabuf, p->mem_priv);
> + call_void_memop(vb, detach_dmabuf, p->mem_priv);
> dma_buf_put(p->dbuf);
> memset(p, 0, sizeof(*p));
> }
> @@ -305,7 +379,6 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
> if (ret) {
> dprintk(1, "Buffer %d %p initialization"
> " failed\n", buffer, vb);
> - fail_vb_qop(vb, buf_init);
> __vb2_buf_mem_free(vb);
> kfree(vb);
> break;
> @@ -382,7 +455,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> struct vb2_buffer *vb = q->bufs[buffer];
>
> if (vb && vb->planes[0].mem_priv)
> - call_vb_qop(vb, buf_cleanup, vb);
> + call_void_vb_qop(vb, buf_cleanup, vb);
> }
>
> /* Release video buffer memory */
> @@ -837,10 +910,8 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> */
> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes,
> q->plane_sizes, q->alloc_ctx);
> - if (ret) {
> - fail_qop(q, queue_setup);
> + if (ret)
> return ret;
> - }
>
> /* Finally, allocate buffers and video memory */
> allocated_buffers = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
> @@ -864,8 +935,6 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>
> ret = call_qop(q, queue_setup, q, NULL, &num_buffers,
> &num_planes, q->plane_sizes, q->alloc_ctx);
> - if (ret)
> - fail_qop(q, queue_setup);
>
> if (!ret && allocated_buffers < num_buffers)
> ret = -ENOMEM;
> @@ -950,10 +1019,8 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
> */
> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
> &num_planes, q->plane_sizes, q->alloc_ctx);
> - if (ret) {
> - fail_qop(q, queue_setup);
> + if (ret)
> return ret;
> - }
>
> /* Finally, allocate buffers and video memory */
> allocated_buffers = __vb2_queue_alloc(q, create->memory, num_buffers,
> @@ -975,8 +1042,6 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
> */
> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
> &num_planes, q->plane_sizes, q->alloc_ctx);
> - if (ret)
> - fail_qop(q, queue_setup);
>
> if (!ret && allocated_buffers < num_buffers)
> ret = -ENOMEM;
> @@ -1038,7 +1103,7 @@ void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
> if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
> return NULL;
>
> - return call_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
> + return call_ptr_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
>
> }
> EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
> @@ -1059,7 +1124,7 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
> if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
> return NULL;
>
> - return call_memop(vb, cookie, vb->planes[plane_no].mem_priv);
> + return call_ptr_memop(vb, cookie, vb->planes[plane_no].mem_priv);
> }
> EXPORT_SYMBOL_GPL(vb2_plane_cookie);
>
> @@ -1112,7 +1177,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>
> /* sync buffers */
> for (plane = 0; plane < vb->num_planes; ++plane)
> - call_memop(vb, finish, vb->planes[plane].mem_priv);
> + call_void_memop(vb, finish, vb->planes[plane].mem_priv);
>
> /* Add the buffer to the done buffers list */
> spin_lock_irqsave(&q->done_lock, flags);
> @@ -1265,22 +1330,21 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> if (vb->planes[plane].mem_priv) {
> if (!reacquired) {
> reacquired = true;
> - call_vb_qop(vb, buf_cleanup, vb);
> + call_void_vb_qop(vb, buf_cleanup, vb);
> }
> - call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> + call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> }
>
> vb->planes[plane].mem_priv = NULL;
> memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
>
> /* Acquire each plane's memory */
> - mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
> + mem_priv = call_ptr_memop(vb, get_userptr, q->alloc_ctx[plane],
> planes[plane].m.userptr,
> planes[plane].length, write);
> if (IS_ERR_OR_NULL(mem_priv)) {
> dprintk(1, "qbuf: failed acquiring userspace "
> "memory for plane %d\n", plane);
> - fail_memop(vb, get_userptr);
> ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
> goto err;
> }
> @@ -1303,7 +1367,6 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> ret = call_vb_qop(vb, buf_init, vb);
> if (ret) {
> dprintk(1, "qbuf: buffer initialization failed\n");
> - fail_vb_qop(vb, buf_init);
> goto err;
> }
> }
> @@ -1311,8 +1374,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> ret = call_vb_qop(vb, buf_prepare, vb);
> if (ret) {
> dprintk(1, "qbuf: buffer preparation failed\n");
> - fail_vb_qop(vb, buf_prepare);
> - call_vb_qop(vb, buf_cleanup, vb);
> + call_void_vb_qop(vb, buf_cleanup, vb);
> goto err;
> }
>
> @@ -1321,7 +1383,7 @@ err:
> /* In case of errors, release planes that were already acquired */
> for (plane = 0; plane < vb->num_planes; ++plane) {
> if (vb->planes[plane].mem_priv)
> - call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> + call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> vb->planes[plane].mem_priv = NULL;
> vb->v4l2_planes[plane].m.userptr = 0;
> vb->v4l2_planes[plane].length = 0;
> @@ -1335,13 +1397,8 @@ err:
> */
> static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> {
> - int ret;
> -
> __fill_vb2_buffer(vb, b, vb->v4l2_planes);
> - ret = call_vb_qop(vb, buf_prepare, vb);
> - if (ret)
> - fail_vb_qop(vb, buf_prepare);
> - return ret;
> + return call_vb_qop(vb, buf_prepare, vb);
> }
>
> /**
> @@ -1393,7 +1450,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>
> if (!reacquired) {
> reacquired = true;
> - call_vb_qop(vb, buf_cleanup, vb);
> + call_void_vb_qop(vb, buf_cleanup, vb);
> }
>
> /* Release previously acquired memory if present */
> @@ -1401,11 +1458,10 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
>
> /* Acquire each plane's memory */
> - mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
> + mem_priv = call_ptr_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
> dbuf, planes[plane].length, write);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "qbuf: failed to attach dmabuf\n");
> - fail_memop(vb, attach_dmabuf);
> ret = PTR_ERR(mem_priv);
> dma_buf_put(dbuf);
> goto err;
> @@ -1424,7 +1480,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> if (ret) {
> dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
> plane);
> - fail_memop(vb, map_dmabuf);
> goto err;
> }
> vb->planes[plane].dbuf_mapped = 1;
> @@ -1445,7 +1500,6 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> ret = call_vb_qop(vb, buf_init, vb);
> if (ret) {
> dprintk(1, "qbuf: buffer initialization failed\n");
> - fail_vb_qop(vb, buf_init);
> goto err;
> }
> }
> @@ -1453,8 +1507,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> ret = call_vb_qop(vb, buf_prepare, vb);
> if (ret) {
> dprintk(1, "qbuf: buffer preparation failed\n");
> - fail_vb_qop(vb, buf_prepare);
> - call_vb_qop(vb, buf_cleanup, vb);
> + call_void_vb_qop(vb, buf_cleanup, vb);
> goto err;
> }
>
> @@ -1479,9 +1532,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>
> /* sync buffers */
> for (plane = 0; plane < vb->num_planes; ++plane)
> - call_memop(vb, prepare, vb->planes[plane].mem_priv);
> + call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>
> - call_vb_qop(vb, buf_queue, vb);
> + call_void_vb_qop(vb, buf_queue, vb);
> }
>
> static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> @@ -1520,9 +1573,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> * mmap_sem and then takes the driver's lock again.
> */
> mmap_sem = ¤t->mm->mmap_sem;
> - call_qop(q, wait_prepare, q);
> + call_void_qop(q, wait_prepare, q);
> down_read(mmap_sem);
> - call_qop(q, wait_finish, q);
> + call_void_qop(q, wait_finish, q);
>
> ret = __qbuf_userptr(vb, b);
>
> @@ -1647,7 +1700,6 @@ static int vb2_start_streaming(struct vb2_queue *q)
> if (!ret)
> return 0;
>
> - fail_qop(q, start_streaming);
> dprintk(1, "qbuf: driver refused to start streaming\n");
> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
> unsigned i;
> @@ -1812,7 +1864,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
> * become ready or for streamoff. Driver's lock is released to
> * allow streamoff or qbuf to be called while waiting.
> */
> - call_qop(q, wait_prepare, q);
> + call_void_qop(q, wait_prepare, q);
>
> /*
> * All locks have been released, it is safe to sleep now.
> @@ -1825,7 +1877,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
> * We need to reevaluate both conditions again after reacquiring
> * the locks or return an error if one occurred.
> */
> - call_qop(q, wait_finish, q);
> + call_void_qop(q, wait_finish, q);
> if (ret) {
> dprintk(1, "Sleep was interrupted\n");
> return ret;
> @@ -1911,7 +1963,7 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
> for (i = 0; i < vb->num_planes; ++i) {
> if (!vb->planes[i].dbuf_mapped)
> continue;
> - call_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
> + call_void_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
> vb->planes[i].dbuf_mapped = 0;
> }
> }
> @@ -1941,7 +1993,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
> return -EINVAL;
> }
>
> - call_vb_qop(vb, buf_finish, vb);
> + call_void_vb_qop(vb, buf_finish, vb);
>
> /* Fill buffer information for the userspace */
> __fill_v4l2_buffer(vb, b);
> @@ -2042,7 +2094,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>
> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> vb->state = VB2_BUF_STATE_PREPARED;
> - call_vb_qop(vb, buf_finish, vb);
> + call_void_vb_qop(vb, buf_finish, vb);
> }
> __vb2_dqbuf(vb);
> }
> @@ -2244,11 +2296,10 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
>
> vb_plane = &vb->planes[eb->plane];
>
> - dbuf = call_memop(vb, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
> + dbuf = call_ptr_memop(vb, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
> if (IS_ERR_OR_NULL(dbuf)) {
> dprintk(1, "Failed to export buffer %d, plane %d\n",
> eb->index, eb->plane);
> - fail_memop(vb, get_dmabuf);
> return -EINVAL;
> }
>
> @@ -2341,10 +2392,8 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> }
>
> ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
> - if (ret) {
> - fail_memop(vb, mmap);
> + if (ret)
> return ret;
> - }
>
> dprintk(3, "Buffer %d, plane %d successfully mapped\n", buffer, plane);
> return 0;
> --
> 1.9.0
>
--
Best regards,
Pawel Osciak
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify Hans Verkuil
@ 2014-03-17 11:44 ` Laurent Pinchart
2014-03-17 11:45 ` Hans Verkuil
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2014-03-17 11:44 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, pawel, Hans Verkuil
Hi Hans,
Thank you for the patch.
On Saturday 15 March 2014 14:08:00 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The notify function is a void function, yet the v4l2_subdev_notify
> define uses it in a ? : construction, which causes sparse warnings.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> include/media/v4l2-subdev.h | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 28f4d8c..0fbf669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -692,9 +692,11 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
> (sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
>
> /* Send a notification to v4l2_device. */
> -#define v4l2_subdev_notify(sd, notification, arg) \
> - ((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
> - (sd)->v4l2_dev->notify((sd), (notification), (arg)))
> +#define v4l2_subdev_notify(sd, notification, arg) \
> + do { \
> + if ((sd) && (sd)->v4l2_dev && (sd)->v4l2_dev->notify) \
> + (sd)->v4l2_dev->notify((sd), (notification), (arg)); \
> + } while (0)
The construct would prevent using v4l2_subdev_notify() as an expression. What
about turning the macro into an inline function instead ?
> #define v4l2_subdev_has_op(sd, o, f) \
> ((sd)->ops->o && (sd)->ops->o->f)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify
2014-03-17 11:44 ` Laurent Pinchart
@ 2014-03-17 11:45 ` Hans Verkuil
2014-03-17 11:49 ` Laurent Pinchart
0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2014-03-17 11:45 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, pawel, Hans Verkuil
On 03/17/2014 12:44 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Saturday 15 March 2014 14:08:00 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The notify function is a void function, yet the v4l2_subdev_notify
>> define uses it in a ? : construction, which causes sparse warnings.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> include/media/v4l2-subdev.h | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 28f4d8c..0fbf669 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -692,9 +692,11 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
>> (sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
>>
>> /* Send a notification to v4l2_device. */
>> -#define v4l2_subdev_notify(sd, notification, arg) \
>> - ((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
>> - (sd)->v4l2_dev->notify((sd), (notification), (arg)))
>> +#define v4l2_subdev_notify(sd, notification, arg) \
>> + do { \
>> + if ((sd) && (sd)->v4l2_dev && (sd)->v4l2_dev->notify) \
>> + (sd)->v4l2_dev->notify((sd), (notification), (arg)); \
>> + } while (0)
>
> The construct would prevent using v4l2_subdev_notify() as an expression. What
> about turning the macro into an inline function instead ?
How can you use a void function in an expression anyway? That was the whole point
of the sparse error.
Regards,
Hans
>
>> #define v4l2_subdev_has_op(sd, o, f) \
>> ((sd)->ops->o && (sd)->ops->o->f)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify
2014-03-17 11:45 ` Hans Verkuil
@ 2014-03-17 11:49 ` Laurent Pinchart
0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2014-03-17 11:49 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, pawel, Hans Verkuil
Hi Hans,
On Monday 17 March 2014 12:45:16 Hans Verkuil wrote:
> On 03/17/2014 12:44 PM, Laurent Pinchart wrote:
> > On Saturday 15 March 2014 14:08:00 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> The notify function is a void function, yet the v4l2_subdev_notify
> >> define uses it in a ? : construction, which causes sparse warnings.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>
> >> include/media/v4l2-subdev.h | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 28f4d8c..0fbf669 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -692,9 +692,11 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
> >>
> >> (sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
> >>
> >> /* Send a notification to v4l2_device. */
> >>
> >> -#define v4l2_subdev_notify(sd, notification, arg) \
> >> - ((!(sd) || !(sd)->v4l2_dev || !(sd)->v4l2_dev->notify) ? -ENODEV : \
> >> - (sd)->v4l2_dev->notify((sd), (notification), (arg)))
> >> +#define v4l2_subdev_notify(sd, notification, arg) \
> >> + do { \
> >> + if ((sd) && (sd)->v4l2_dev && (sd)->v4l2_dev->notify) \
> >> + (sd)->v4l2_dev->notify((sd), (notification), (arg)); \
> >> + } while (0)
> >
> > The construct would prevent using v4l2_subdev_notify() as an expression.
> > What about turning the macro into an inline function instead ?
>
> How can you use a void function in an expression anyway? That was the whole
> point of the sparse error.
In a for loop for instance ?
for (i = 0; i < n; v4l2_subdev_notify(), ++i)
I agree it's a bit far-fetched, but as we need to modify the macro, I'd take
that as an opportunity to turn it into an inline function.
> >> #define v4l2_subdev_has_op(sd, o, f) \
> >>
> >> ((sd)->ops->o && (sd)->ops->o->f)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [REVIEW PATCH for v3.15 4/4] v4l2-ioctl.c: fix sparse __user-related warnings
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 4/4] v4l2-ioctl.c: fix sparse __user-related warnings Hans Verkuil
@ 2014-03-17 11:59 ` Laurent Pinchart
0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2014-03-17 11:59 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, pawel, Hans Verkuil
Hi Hans,
Thank you for the patch.
On Saturday 15 March 2014 14:08:03 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Drop the use of __user in the user_ptr variable since the v4l2 structs are
> actually defined without __user, instead cast to a __user pointer only
> there where it is really needed: in the copy_to/from_user calls.
>
> Also remove unnecessary casts in check_array_args and replace a wrong
> cast (void *) with the correct one (void **).
>
> This fixes these sparse warnings:
>
> drivers/media/v4l2-core/v4l2-ioctl.c:2284:35: warning: incorrect type in
> assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-ioctl.c:2301:35: warning: incorrect type in
> assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-ioctl.c:2319:35: warning: incorrect type in
> assignment (different address spaces)
> drivers/media/v4l2-core/v4l2-ioctl.c:2386:57: warning: incorrect type in
> argument 4 (different address spaces)
> drivers/media/v4l2-core/v4l2-ioctl.c:2420:29: warning: incorrect type in
> assignment (different address spaces)
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index d9113cc..3e0cf4f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2260,7 +2260,7 @@ done:
> }
>
> static int check_array_args(unsigned int cmd, void *parg, size_t
> *array_size,
> - void * __user *user_ptr, void ***kernel_ptr)
> + void **user_ptr, void ***kernel_ptr)
> {
> int ret = 0;
>
> @@ -2276,8 +2276,8 @@ static int check_array_args(unsigned int cmd, void
> *parg, size_t *array_size, ret = -EINVAL;
> break;
> }
> - *user_ptr = (void __user *)buf->m.planes;
> - *kernel_ptr = (void *)&buf->m.planes;
> + *user_ptr = buf->m.planes;
> + *kernel_ptr = (void **)&buf->m.planes;
> *array_size = sizeof(struct v4l2_plane) * buf->length;
> ret = 1;
> }
> @@ -2293,8 +2293,8 @@ static int check_array_args(unsigned int cmd, void
> *parg, size_t *array_size, ret = -EINVAL;
> break;
> }
> - *user_ptr = (void __user *)edid->edid;
> - *kernel_ptr = (void *)&edid->edid;
> + *user_ptr = edid->edid;
> + *kernel_ptr = (void **)&edid->edid;
> *array_size = edid->blocks * 128;
> ret = 1;
> }
> @@ -2311,8 +2311,8 @@ static int check_array_args(unsigned int cmd, void
> *parg, size_t *array_size, ret = -EINVAL;
> break;
> }
> - *user_ptr = (void __user *)ctrls->controls;
> - *kernel_ptr = (void *)&ctrls->controls;
> + *user_ptr = ctrls->controls;
> + *kernel_ptr = (void **)&ctrls->controls;
> *array_size = sizeof(struct v4l2_ext_control)
> * ctrls->count;
> ret = 1;
> @@ -2334,7 +2334,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg, long err = -EINVAL;
> bool has_array_args;
> size_t array_size = 0;
> - void __user *user_ptr = NULL;
> + void *user_ptr = NULL;
> void **kernel_ptr = NULL;
>
> /* Copy arguments into temp kernel buffer */
> @@ -2395,7 +2395,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg, if (NULL == mbuf)
> goto out_array_args;
> err = -EFAULT;
> - if (copy_from_user(mbuf, user_ptr, array_size))
> + if (copy_from_user(mbuf, (void __user *)user_ptr, array_size))
> goto out_array_args;
> *kernel_ptr = mbuf;
> }
> @@ -2413,7 +2413,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> unsigned long arg,
>
> if (has_array_args) {
> *kernel_ptr = user_ptr;
> - if (copy_to_user(user_ptr, mbuf, array_size))
> + if (copy_to_user((void __user *)user_ptr, mbuf, array_size))
Moving the __user annotation to copy_from_user/copy_to_user defeats the whole
point of the annotation. user_ptr is really a user pointer here, and I believe
it should be treated as such. I'd rather fix the sparse warnings where they
really occur.
> err = -EFAULT;
> goto out_array_args;
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-03-17 11:57 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-15 13:07 [REVIEW PATCH for v3.15 0/4] v4l2 core sparse error/warning fixes Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 1/4] v4l2-subdev.h: fix sparse error with v4l2_subdev_notify Hans Verkuil
2014-03-17 11:44 ` Laurent Pinchart
2014-03-17 11:45 ` Hans Verkuil
2014-03-17 11:49 ` Laurent Pinchart
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 2/4] videobuf2-core: fix sparse errors Hans Verkuil
2014-03-17 10:20 ` Pawel Osciak
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 3/4] v4l2-common.h: remove __user annotation in struct v4l2_edid Hans Verkuil
2014-03-15 13:08 ` [REVIEW PATCH for v3.15 4/4] v4l2-ioctl.c: fix sparse __user-related warnings Hans Verkuil
2014-03-17 11:59 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox