Skip to contentSkip to conversation

v4l2: move tracepoints to video_usercopy

PATCH In review Hans Verkuil 2 participants 2 messages Dec 16, 2013 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 v4l2: move tracepoints to video_usercopy

The (d)qbuf ioctls were traced in the low-level v4l2 ioctl function. The trace was outside the serialization lock, so that can affect the usefulness of the timing. In addition, the __user pointer was expected instead of a proper kernel pointer.

By moving the tracepoints to video_usercopy we ensure that the trace calls use the correct kernel pointer, and that it happens right after the ioctl call to the driver, so certainly inside the serialization lock.

In addition, we only trace if the call was successful.

---
 drivers/media/v4l2-core/v4l2-dev.c   | 9 ---------
 drivers/media/v4l2-core/v4l2-ioctl.c | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)
drivers/media/v4l2-core/v4l2-dev.c +0 -9
index 1cc1749..b5aaaac 100644
@@ -31,10 +31,6 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 
-
-#define CREATE_TRACE_POINTS
-#include <trace/events/v4l2.h>
-
 #define VIDEO_NUM_DEVICES	256
 #define VIDEO_NAME              "video4linux"
 
@@ -395,11 +391,6 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	} else
 		ret = -ENOTTY;
 
-	if (cmd == VIDIOC_DQBUF)
-		trace_v4l2_dqbuf(vdev->minor, (struct v4l2_buffer *)arg);
-	else if (cmd == VIDIOC_QBUF)
-		trace_v4l2_qbuf(vdev->minor, (struct v4l2_buffer *)arg);
-
 	return ret;
 }
 
drivers/media/v4l2-core/v4l2-ioctl.c +9 -0
index 68e6b5e..707aef7 100644
@@ -28,6 +28,9 @@
 #include <media/v4l2-device.h>
 #include <media/videobuf2-core.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/v4l2.h>
+
 /* Zero out the end of the struct pointed to by p.  Everything after, but
  * not including, the specified field is cleared. */
 #define CLEAR_AFTER_FIELD(p, field) \
@@ -2338,6 +2341,12 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	err = func(file, cmd, parg);
 	if (err == -ENOIOCTLCMD)
 		err = -ENOTTY;
+	if (err == 0) {
+		if (cmd == VIDIOC_DQBUF)
+			trace_v4l2_dqbuf(video_devdata(file)->minor, parg);
+		else if (cmd == VIDIOC_QBUF)
+			trace_v4l2_qbuf(video_devdata(file)->minor, parg);
+	}
 
 	if (has_array_args) {
 		*kernel_ptr = user_ptr;
Signature · 1 line
1.8.4.3
Acked-by on v1 1/1
#1 lore raw
Quoted 5 lines · Hans Verkuil

The (d)qbuf ioctls were traced in the low-level v4l2 ioctl function. The trace was outside the serialization lock, so that can affect the usefulness of the timing. In addition, the __user pointer was expected instead of a proper kernel pointer.

Quoted 7 lines

By moving the tracepoints to video_usercopy we ensure that the trace calls use the correct kernel pointer, and that it happens right after the ioctl call to the driver, so certainly inside the serialization lock.

In addition, we only trace if the call was successful.

Looks good to me.

Quoted 59 lines
---
 drivers/media/v4l2-core/v4l2-dev.c   | 9 ---------
 drivers/media/v4l2-core/v4l2-ioctl.c | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)
drivers/media/v4l2-core/v4l2-dev.c +0 -9
index 1cc1749..b5aaaac 100644
@@ -31,10 +31,6 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 
-
-#define CREATE_TRACE_POINTS
-#include <trace/events/v4l2.h>
-
 #define VIDEO_NUM_DEVICES	256
 #define VIDEO_NAME              "video4linux"
 
@@ -395,11 +391,6 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	} else
 		ret = -ENOTTY;
 
-	if (cmd == VIDIOC_DQBUF)
-		trace_v4l2_dqbuf(vdev->minor, (struct v4l2_buffer *)arg);
-	else if (cmd == VIDIOC_QBUF)
-		trace_v4l2_qbuf(vdev->minor, (struct v4l2_buffer *)arg);
-
 	return ret;
 }
 
drivers/media/v4l2-core/v4l2-ioctl.c +9 -0
index 68e6b5e..707aef7 100644
@@ -28,6 +28,9 @@
 #include <media/v4l2-device.h>
 #include <media/videobuf2-core.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/v4l2.h>
+
 /* Zero out the end of the struct pointed to by p.  Everything after, but
  * not including, the specified field is cleared. */
 #define CLEAR_AFTER_FIELD(p, field) \
@@ -2338,6 +2341,12 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	err = func(file, cmd, parg);
 	if (err == -ENOIOCTLCMD)
 		err = -ENOTTY;
+	if (err == 0) {
+		if (cmd == VIDIOC_DQBUF)
+			trace_v4l2_dqbuf(video_devdata(file)->minor, parg);
+		else if (cmd == VIDIOC_QBUF)
+			trace_v4l2_qbuf(video_devdata(file)->minor, parg);
+	}
 
 	if (has_array_args) {
 		*kernel_ptr = user_ptr;
Signature · 7 lines
Wade Farnsworth | Sr. Embedded Linux Dev. Engr.
Mentor Embedded™
Nucleus® | Linux® | Android™ | Services | UI | Multi-OS
Android is a trademark of Google Inc. Use of this trademark is subject
to Google Permissions.
Linux is the registered trademark of Linus Torvalds in the U.S. and
other countries