Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.
From: Hans Verkuil <hidden>
Date: 2011-08-09 10:10:37
On Sunday, July 31, 2011 14:43:44 Mauro Carvalho Chehab wrote:
Em 29-07-2011 09:10, Hans Verkuil escreveu:quoted
Hi all, While converting v4l2-compliance to correctly handle ENOTTY errors I found several regressions in v4l2-ioctl.c: 1) VIDIOC_ENUM/G/S/TRY_FMT would return -ENOTTY if the op for the particular format type was not set, even though the op for other types might have been present. In such a case -EINVAL should have been returned. 2) The priority check could cause -EBUSY or -EINVAL to be returned instead of -ENOTTY if the corresponding ioctl was unsupported. 3) Certain ioctls that have an internal implementation (ENUMSTD, G_STD, S_STD, G_PARM and the extended control ioctls) could return -EINVAL when -ENOTTY should have been returned or vice versa. I first tried to fix this by adding extra code for each affected ioctl, but that made the code rather ugly. So I ended up with this code that first checks whether a certain ioctl is supported or not and returns -ENOTTY if not. Comments?This patch adds an extra cost of double-parsing the ioctl just because the errors. The proper way is to check at the error path. See the enclosed patch.
Your patch fixes some but not all of the problems that my patch fixes. I'm trying to create a new patch on top of yours that actually fixes all the issues, but I'm having a hard time with that. It is getting very difficult to follow the error path, which is exactly why I didn't want to do that in the first place. I've never understood the fixation on performance *without doing any measurements*. As the old saying goes: "Premature optimization is the root of all evil." Code such as the likely/unlikely macros just obfuscate the code and should not be added IMHO unless you can prove that it makes a difference. See for example the discussion whether prefetch is useful or not: http://lwn.net/Articles/444336 Code complexity is by far the biggest problem with all V4L code. I am tempted to completely reorganize v4l2-ioctl.c, but I can't do that for v3.1. I'll try to come up with another approach instead. Regards, Hans
quoted hunk ↗ jump to hunk
From: Mauro Carvalho Chehab <redacted> Date: Sun, 31 Jul 2011 09:37:56 -0300 [PATCH] v4l2-ioctl: properly return -EINVAL when parameters are wrong When an ioctl is implemented, but the parameters are invalid, the error code should be -EINVAL. However, if the ioctl is not defined, it should return -ENOTTY instead. While here, adds a gcc hint that having the ioctl enabled is more likely, as userspace should know what the driver supports due to QUERYCAP call. Reported-by: Hans Verkuil <redacted> Signed-off-by: Mauro Carvalho Chehab <redacted>diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 002ce13..9f80e9d 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c@@ -55,6 +55,14 @@ memset((u8 *)(p) + offsetof(typeof(*(p)), field) + sizeof((p)->field), \ 0, sizeof(*(p)) - offsetof(typeof(*(p)), field) - sizeof((p)->field)) +#define no_ioctl_err(foo) ( ( \ + ops->vidioc_##foo##_fmt_vid_cap || \ + ops->vidioc_##foo##_fmt_vid_out || \ + ops->vidioc_##foo##_fmt_vid_cap_mplane || \ + ops->vidioc_##foo##_fmt_vid_out_mplane || \ + ops->vidioc_##foo##_fmt_vid_overlay || \ + ops->vidioc_##foo##_fmt_type_private) ? -EINVAL : -ENOTTY) + struct std_descr { v4l2_std_id std; const char *descr;@@ -591,7 +599,7 @@ static long __video_do_ioctl(struct file *file, ret = v4l2_prio_check(vfd->prio, vfh->prio); if (ret) goto exit_prio; - ret = -EINVAL; + ret = -ENOTTY; break; } }@@ -638,7 +646,7 @@ static long __video_do_ioctl(struct file *file, enum v4l2_priority *p = arg; if (!ops->vidioc_s_priority && !use_fh_prio) - break; + break; dbgarg(cmd, "setting priority to %d\n", *p); if (ops->vidioc_s_priority) ret = ops->vidioc_s_priority(file, fh, *p);@@ -654,37 +662,37 @@ static long __video_do_ioctl(struct file *file, switch (f->type) { case V4L2_BUF_TYPE_VIDEO_CAPTURE: - if (ops->vidioc_enum_fmt_vid_cap) + if (likely(ops->vidioc_enum_fmt_vid_cap)) ret = ops->vidioc_enum_fmt_vid_cap(file, fh, f); break; case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: - if (ops->vidioc_enum_fmt_vid_cap_mplane) + if (likely(ops->vidioc_enum_fmt_vid_cap_mplane)) ret = ops->vidioc_enum_fmt_vid_cap_mplane(file, fh, f); break; case V4L2_BUF_TYPE_VIDEO_OVERLAY: - if (ops->vidioc_enum_fmt_vid_overlay) + if (likely(ops->vidioc_enum_fmt_vid_overlay)) ret = ops->vidioc_enum_fmt_vid_overlay(file, fh, f); break; case V4L2_BUF_TYPE_VIDEO_OUTPUT: - if (ops->vidioc_enum_fmt_vid_out) + if (likely(ops->vidioc_enum_fmt_vid_out)) ret = ops->vidioc_enum_fmt_vid_out(file, fh, f); break; case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: - if (ops->vidioc_enum_fmt_vid_out_mplane) + if (likely(ops->vidioc_enum_fmt_vid_out_mplane)) ret = ops->vidioc_enum_fmt_vid_out_mplane(file, fh, f); break; case V4L2_BUF_TYPE_PRIVATE: - if (ops->vidioc_enum_fmt_type_private) + if (likely(ops->vidioc_enum_fmt_type_private)) ret = ops->vidioc_enum_fmt_type_private(file, fh, f); break; default: break; } - if (!ret) + if (likely (!ret)) dbgarg(cmd, "index=%d, type=%d, flags=%d, " "pixelformat=%c%c%c%c, description='%s'\n", f->index, f->type, f->flags,@@ -693,6 +701,8 @@ static long __video_do_ioctl(struct file *file, (f->pixelformat >> 16) & 0xff, (f->pixelformat >> 24) & 0xff, f->description); + else if (ret == -ENOTTY) + ret = no_ioctl_err(enum); break; } case VIDIOC_G_FMT:@@ -744,7 +754,7 @@ static long __video_do_ioctl(struct file *file, v4l_print_pix_fmt_mplane(vfd, &f->fmt.pix_mp); break; case V4L2_BUF_TYPE_VIDEO_OVERLAY: - if (ops->vidioc_g_fmt_vid_overlay) + if (likely(ops->vidioc_g_fmt_vid_overlay)) ret = ops->vidioc_g_fmt_vid_overlay(file, fh, f); break;@@ -789,34 +799,36 @@ static long __video_do_ioctl(struct file *file, v4l_print_pix_fmt_mplane(vfd, &f->fmt.pix_mp); break; case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: - if (ops->vidioc_g_fmt_vid_out_overlay) + if (likely(ops->vidioc_g_fmt_vid_out_overlay)) ret = ops->vidioc_g_fmt_vid_out_overlay(file, fh, f); break; case V4L2_BUF_TYPE_VBI_CAPTURE: - if (ops->vidioc_g_fmt_vbi_cap) + if (likely(ops->vidioc_g_fmt_vbi_cap)) ret = ops->vidioc_g_fmt_vbi_cap(file, fh, f); break; case V4L2_BUF_TYPE_VBI_OUTPUT: - if (ops->vidioc_g_fmt_vbi_out) + if (likely(ops->vidioc_g_fmt_vbi_out)) ret = ops->vidioc_g_fmt_vbi_out(file, fh, f); break; case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE: - if (ops->vidioc_g_fmt_sliced_vbi_cap) + if (likely(ops->vidioc_g_fmt_sliced_vbi_cap)) ret = ops->vidioc_g_fmt_sliced_vbi_cap(file, fh, f); break; case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT: - if (ops->vidioc_g_fmt_sliced_vbi_out) + if (likely(ops->vidioc_g_fmt_sliced_vbi_out)) ret = ops->vidioc_g_fmt_sliced_vbi_out(file, fh, f); break; case V4L2_BUF_TYPE_PRIVATE: - if (ops->vidioc_g_fmt_type_private) + if (likely(ops->vidioc_g_fmt_type_private)) ret = ops->vidioc_g_fmt_type_private(file, fh, f); break; } + if (unlikely(ret == -ENOTTY)) + ret = no_ioctl_err(g); break; }@@ -926,33 +938,36 @@ static long __video_do_ioctl(struct file *file, break; case V4L2_BUF_TYPE_VBI_CAPTURE: CLEAR_AFTER_FIELD(f, fmt.vbi); - if (ops->vidioc_s_fmt_vbi_cap) + if (likely(ops->vidioc_s_fmt_vbi_cap)) ret = ops->vidioc_s_fmt_vbi_cap(file, fh, f); break; case V4L2_BUF_TYPE_VBI_OUTPUT: CLEAR_AFTER_FIELD(f, fmt.vbi); - if (ops->vidioc_s_fmt_vbi_out) + if (likely(ops->vidioc_s_fmt_vbi_out)) ret = ops->vidioc_s_fmt_vbi_out(file, fh, f); break; case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE: CLEAR_AFTER_FIELD(f, fmt.sliced); - if (ops->vidioc_s_fmt_sliced_vbi_cap) + if (likely(ops->vidioc_s_fmt_sliced_vbi_cap)) ret = ops->vidioc_s_fmt_sliced_vbi_cap(file, fh, f); break; case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT: CLEAR_AFTER_FIELD(f, fmt.sliced); - if (ops->vidioc_s_fmt_sliced_vbi_out) + if (likely(ops->vidioc_s_fmt_sliced_vbi_out)) ret = ops->vidioc_s_fmt_sliced_vbi_out(file, fh, f); + break; case V4L2_BUF_TYPE_PRIVATE: /* CLEAR_AFTER_FIELD(f, fmt.raw_data); <- does nothing */ - if (ops->vidioc_s_fmt_type_private) + if (likely(ops->vidioc_s_fmt_type_private)) ret = ops->vidioc_s_fmt_type_private(file, fh, f); break; } + if (unlikely(ret == -ENOTTY)) + ret = no_ioctl_err(g); break; } case VIDIOC_TRY_FMT:@@ -1008,7 +1023,7 @@ static long __video_do_ioctl(struct file *file, break; case V4L2_BUF_TYPE_VIDEO_OVERLAY: CLEAR_AFTER_FIELD(f, fmt.win); - if (ops->vidioc_try_fmt_vid_overlay) + if (likely(ops->vidioc_try_fmt_vid_overlay)) ret = ops->vidioc_try_fmt_vid_overlay(file, fh, f); break;@@ -1057,40 +1072,43 @@ static long __video_do_ioctl(struct file *file, break; case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: CLEAR_AFTER_FIELD(f, fmt.win); - if (ops->vidioc_try_fmt_vid_out_overlay) + if (likely(ops->vidioc_try_fmt_vid_out_overlay)) ret = ops->vidioc_try_fmt_vid_out_overlay(file, fh, f); break; case V4L2_BUF_TYPE_VBI_CAPTURE: CLEAR_AFTER_FIELD(f, fmt.vbi); - if (ops->vidioc_try_fmt_vbi_cap) + if (likely(ops->vidioc_try_fmt_vbi_cap)) ret = ops->vidioc_try_fmt_vbi_cap(file, fh, f); break; case V4L2_BUF_TYPE_VBI_OUTPUT: CLEAR_AFTER_FIELD(f, fmt.vbi); - if (ops->vidioc_try_fmt_vbi_out) + if (likely(ops->vidioc_try_fmt_vbi_out)) ret = ops->vidioc_try_fmt_vbi_out(file, fh, f); break; case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE: CLEAR_AFTER_FIELD(f, fmt.sliced); - if (ops->vidioc_try_fmt_sliced_vbi_cap) + if (likely(ops->vidioc_try_fmt_sliced_vbi_cap)) ret = ops->vidioc_try_fmt_sliced_vbi_cap(file, fh, f); break; case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT: CLEAR_AFTER_FIELD(f, fmt.sliced); - if (ops->vidioc_try_fmt_sliced_vbi_out) + if (likely(ops->vidioc_try_fmt_sliced_vbi_out)) ret = ops->vidioc_try_fmt_sliced_vbi_out(file, fh, f); + else + ret = no_ioctl_err(try); break; case V4L2_BUF_TYPE_PRIVATE: /* CLEAR_AFTER_FIELD(f, fmt.raw_data); <- does nothing */ - if (ops->vidioc_try_fmt_type_private) + if (likely(ops->vidioc_try_fmt_type_private)) ret = ops->vidioc_try_fmt_type_private(file, fh, f); break; } - + if (unlikely(ret == -ENOTTY)) + ret = no_ioctl_err(g); break; } /* FIXME: Those buf reqs could be handled here,@@ -1262,16 +1280,15 @@ static long __video_do_ioctl(struct file *file, { v4l2_std_id *id = arg; - ret = 0; /* Calls the specific handler */ if (ops->vidioc_g_std) ret = ops->vidioc_g_std(file, fh, id); - else if (vfd->current_norm) + else if (vfd->current_norm) { + ret = 0; *id = vfd->current_norm; - else - ret = -EINVAL; + } - if (!ret) + if (likely(!ret)) dbgarg(cmd, "std=0x%08Lx\n", (long long unsigned)*id); break; }@@ -1288,8 +1305,6 @@ static long __video_do_ioctl(struct file *file, /* Calls the specific handler */ if (ops->vidioc_s_std) ret = ops->vidioc_s_std(file, fh, &norm); - else - ret = -EINVAL; /* Updates standard information */ if (ret >= 0)@@ -1812,7 +1827,7 @@ static long __video_do_ioctl(struct file *file, if (ops->vidioc_g_std) ret = ops->vidioc_g_std(file, fh, &std); else if (std == 0) - ret = -EINVAL; + ret = -ENOTTY; if (ret == 0) v4l2_video_std_frame_period(std, &p->parm.capture.timeperframe); --To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html