[RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.

2011-07-29 Thread Hans Verkuil 
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?

Hans


Signed-off-by: Hans Verkuil 
---
 drivers/media/video/v4l2-ioctl.c |  466 +++---
 1 files changed, 289 insertions(+), 177 deletions(-)

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 002ce13..6a5062a 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -542,13 +542,14 @@ static long __video_do_ioctl(struct file *file,
void *fh = file->private_data;
struct v4l2_fh *vfh = NULL;
struct v4l2_format f_copy;
+   bool have_op;
int use_fh_prio = 0;
-   long ret = -ENOTTY;
+   long ret = -EINVAL;
 
if (ops == NULL) {
printk(KERN_WARNING "videodev: \"%s\" has no ioctl_ops.\n",
vfd->name);
-   return ret;
+   return -ENOTTY;
}
 
if ((vfd->debug & V4L2_DEBUG_IOCTL) &&
@@ -562,6 +563,275 @@ static long __video_do_ioctl(struct file *file,
use_fh_prio = test_bit(V4L2_FL_USE_FH_PRIO, &vfd->flags);
}
 
+   switch (cmd) {
+   case VIDIOC_QUERYCAP:
+   have_op = ops->vidioc_querycap;
+   break;
+   case VIDIOC_G_PRIORITY:
+   have_op = ops->vidioc_g_priority || use_fh_prio;
+   break;
+   case VIDIOC_S_PRIORITY:
+   have_op = ops->vidioc_s_priority || use_fh_prio;
+   break;
+   case VIDIOC_ENUM_FMT:
+   have_op = ops->vidioc_enum_fmt_vid_cap ||
+ ops->vidioc_enum_fmt_vid_out ||
+ ops->vidioc_enum_fmt_vid_cap_mplane ||
+ ops->vidioc_enum_fmt_vid_out_mplane ||
+ ops->vidioc_enum_fmt_vid_overlay ||
+ ops->vidioc_enum_fmt_type_private;
+   break;
+   case VIDIOC_G_FMT:
+   have_op = ops->vidioc_g_fmt_vid_cap ||
+ ops->vidioc_g_fmt_vid_out ||
+ ops->vidioc_g_fmt_vid_cap_mplane ||
+ ops->vidioc_g_fmt_vid_out_mplane ||
+ ops->vidioc_g_fmt_vbi_cap ||
+ ops->vidioc_g_fmt_vid_overlay ||
+ ops->vidioc_g_fmt_vid_out_overlay ||
+ ops->vidioc_g_fmt_vbi_out ||
+ ops->vidioc_g_fmt_sliced_vbi_cap ||
+ ops->vidioc_g_fmt_sliced_vbi_out ||
+ ops->vidioc_g_fmt_type_private;
+   break;
+   case VIDIOC_S_FMT:
+   have_op = ops->vidioc_s_fmt_vid_cap ||
+ ops->vidioc_s_fmt_vid_out ||
+ ops->vidioc_s_fmt_vid_cap_mplane ||
+ ops->vidioc_s_fmt_vid_out_mplane ||
+ ops->vidioc_s_fmt_vbi_cap ||
+ ops->vidioc_s_fmt_vid_overlay ||
+ ops->vidioc_s_fmt_vid_out_overlay ||
+ ops->vidioc_s_fmt_vbi_out ||
+ ops->vidioc_s_fmt_sliced_vbi_cap ||
+ ops->vidioc_s_fmt_sliced_vbi_out ||
+ ops->vidioc_s_fmt_type_private;
+   break;
+   case VIDIOC_TRY_FMT:
+   have_op = ops->vidioc_try_fmt_vid_cap ||
+ ops->vidioc_try_fmt_vid_out ||
+ ops->vidioc_try_fmt_vid_cap_mplane ||
+ ops->vidioc_try_fmt_vid_out_mplane ||
+ ops->vidioc_try_fmt_vbi_cap ||
+ ops->vidioc_try_fmt_vid_overlay ||
+ ops->vidioc_try_fmt_vid_out_overlay ||
+ ops->vidioc_try_fmt_vbi_out ||
+ ops->vidioc_try_fmt_sliced_vbi_cap ||
+ ops->vidioc_try_fmt_sliced_vbi_out ||
+ ops->vidioc_try_fmt_type_private;
+   break;
+   case VIDIOC_REQBUFS:
+   have_op = ops->vidioc_reqbufs;
+   break;

Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.

2011-07-31 Thread Mauro Carvalho Chehab 
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.


From: Mauro Carvalho Chehab 
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 
Signed-off-by: Mauro Carvalho Chehab 

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))

Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.

2011-08-09 Thread Hans Verkuil 
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.

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

> 
> 
> From: Mauro Carvalho Chehab 
> 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 
> Signed-off-by: Mauro Carvalho Chehab 
> 
> 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

Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.

2011-08-09 Thread Mauro Carvalho Chehab 
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?

> 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.

> 
> 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?

> 
> Regards,
> 
>   Hans
> 
>>
>>
>> From: Mauro Carvalho Chehab 
>> 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 
>> Signed-off-by: Mauro Carvalho Chehab 
>>
>> 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#

Re: [RFCv1 PATCH for v3.1] v4l2-ioctl: fix ENOTTY handling.

2011-08-09 Thread Hans Verkuil 
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 
> >> 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 su