Hans Verkuil wrote:
> 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.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
Looks good to me.
Acked-by: Wade Farnsworth <wade_farnsworth@xxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-dev.c | 9 ---------
> drivers/media/v4l2-core/v4l2-ioctl.c | 9 +++++++++
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 1cc1749..b5aaaac 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -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;
> }
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 68e6b5e..707aef7 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -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;
--
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
--
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
