On 11/29/13 19:16, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Friday 29 November 2013 10:58:36 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Rather than taking the mmap semaphore at a relatively high-level function,
>> push it down to the place where it is really needed.
>>
>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
>> vb2 calls. The only way I can see that a race can happen is when two
>> threads queue the same buffer. The solution for that it to introduce
>> a PREPARING state.
>
> This looks better to me, but what about a vb2_reqbufs(0) call being processed
> during the time window where we release the queue mutex ?
You have a nasty mind. That can still go wrong, but note that this is true for
the existing code as well as far as I can tell.
How about this patch to fix this race?
It just refuses to free buffers if any of them is in the preparing state.
It returns EAGAIN in that case (or would EBUSY be better?).
Regards,
Hans
Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 2aff646..c3ff993 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -284,10 +284,28 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
* related information, if no buffers are left return the queue to an
* uninitialized state. Might be called even if the queue has already been freed.
*/
-static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
+static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
{
unsigned int buffer;
+ /*
+ * Sanity check: when preparing a buffer the queue lock is released for
+ * a short while (see __buf_prepare for the details), which would allow
+ * a race with a reqbufs which can call this function. Removing the
+ * buffers from underneath __buf_prepare is obviously a bad idea, so we
+ * check if any of the buffers is in the state PREPARING, and if so we
+ * just return -EAGAIN.
+ */
+ for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
+ ++buffer) {
+ if (q->bufs[buffer] == NULL)
+ continue;
+ if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
+ dprintk(1, "reqbufs: preparing buffers, cannot free\n");
+ return -EAGAIN;
+ }
+ }
+
/* Call driver-provided cleanup function for each buffer, if provided */
if (q->ops->buf_cleanup) {
for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
@@ -312,6 +330,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
if (!q->num_buffers)
q->memory = 0;
INIT_LIST_HEAD(&q->queued_list);
+ return 0;
}
/**
@@ -644,7 +663,9 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
return -EBUSY;
}
- __vb2_queue_free(q, q->num_buffers);
+ ret = __vb2_queue_free(q, q->num_buffers);
+ if (ret)
+ return ret;
/*
* In case of REQBUFS(0) return immediately without calling
--
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
