On Tuesday, August 09, 2011 13:22:55 Mauro Carvalho Chehab wrote:
> Em 09-08-2011 07:10, Hans Verkuil escreveu:
> > On Sunday, July 31, 2011 14:43:44 Mauro Carvalho Chehab wrote:
> >> Em 29-07-2011 09:10, Hans Verkuil escreveu:
> >>> 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.
>
> What was left?
A number of things: the priority check has to be done after the 'ENOTTY' check,
the no_ioctl_err() macros had copy and paste errors ('g' was used for both set
and try), and was incomplete (enum_fmt has a subset of the possible ops compared
to get/set/try).
Several error paths were also not handled correctly (returning ENOTTY instead of
EINVAL).
> > 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
>
> I politely disagree that likely/unlikely macros obfuscate the code.
>
> If it actually make some difference or not would require a study using several
> different processors with different types of CPU pipelines, and/or checking the
> generated assembler for the supported processor families.
>
> If someone comes to a conclusion that it doesn't make any difference, we can
> simply discard them.
That's the wrong way around IMHO. Code must serve a purpose. I have no problem
with likely/unlikely in tight loops or often executed code. Neither of which is
the case here.
>
> >
> > 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.
>
> A single code block like:
>
> switch (ioctl) {
> case VIDIOC_foo:
> /* handle foo */
> break;
> ...
> }
>
> Has less complexity than two blocks:
>
> switch (ioctl) {
> case VIDIOC_foo:
> /* Check for errors on foo */
> break;
> ...
> }
>
> switch (ioctl) {
> case VIDIOC_foo:
> /* handle foo */
> break;
> ...
> }
>
> Also, it allows optimizing the error handling logic to only run when there's
> an error, and not before.
>
> In the past, the ioctl handling were a big mess, as there were several switch
> loops to handle ioctl's, internally to each V4L driver. This is harder to
> review, and may lead into mistakes. A single switch improved a lot code
> readability, as now everything is there together.
>
> The priority patches messed it somehow, by adding an extra switch. Let's not
> add more complexity to it.
>
> > I'll try to come up with another approach instead.
>
> What do you have in mind?
I'll post a new patch today. The vivi and ivtv drivers now pass the v4l2-compliance
test again.
Regards,
Hans
>
> >
> > Regards,
> >
> > Hans
> >
> >>
> >>
> >> From: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> >> 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 <hverkuil@xxxxxxxxx>
> >> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxx
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >>
>
> --
> 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
>
--
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
