[PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Currently, -EINVAL is used to return either when an IOCTL is not
implemented, or if the ioctl was not implemented.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 Documentation/DocBook/media/Makefile           |    1 +
 Documentation/DocBook/media/v4l/func-ioctl.xml |   17 +++++++++--------
 drivers/media/video/v4l2-ioctl.c               |    4 ++--
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/DocBook/media/Makefile b/Documentation/DocBook/media/Makefile
index 8cb27f3..93722da 100644
--- a/Documentation/DocBook/media/Makefile
+++ b/Documentation/DocBook/media/Makefile
@@ -117,6 +117,7 @@ ERRORS = \
 	EPERM \
 	ERANGE \
 	EPIPE \
+	ENOIOCTLCMD \
 
 ESCAPE = \
 	-e "s/&/\\&amp;/g" \
diff --git a/Documentation/DocBook/media/v4l/func-ioctl.xml b/Documentation/DocBook/media/v4l/func-ioctl.xml
index b60fd37..0c97ba9 100644
--- a/Documentation/DocBook/media/v4l/func-ioctl.xml
+++ b/Documentation/DocBook/media/v4l/func-ioctl.xml
@@ -132,14 +132,15 @@ complete the request.</para>
 &VIDIOC-S-CTRL; ioctl to a value which is out of bounds.</para>
 	</listitem>
       </varlistentry>
+      <varlistentry>
+	<term><errorcode>ENOIOCTLCMD</errorcode></term>
+	<listitem>
+	  <para>The application attempted to use a non-existent ioctl. This is returned by the V4L2 core only.
+		Applications should be able to handle this error code, in order to detect if a new ioctl is
+		not implemented at the current Kernel version. Kernel versions lower than 3.0 returns EINVAL to
+		non-existing ioctl's.</para>
+	</listitem>
+      </varlistentry>
     </variablelist>
   </refsect1>
 </refentry>
-
-<!--
-Local Variables:
-mode: sgml
-sgml-parent-document: "v4l2.sgml"
-indent-tabs-mode: nil
-End:
--->
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 61ac6bf..a0a2466 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -543,12 +543,12 @@ static long __video_do_ioctl(struct file *file,
 	struct v4l2_fh *vfh = NULL;
 	struct v4l2_format f_copy;
 	int use_fh_prio = 0;
-	long ret = -EINVAL;
+	long ret = -ENOIOCTLCMD;
 
 	if (ops == NULL) {
 		printk(KERN_WARNING "videodev: \"%s\" has no ioctl_ops.\n",
 				vfd->name);
-		return -EINVAL;
+		return ret;
 	}
 
 	if ((vfd->debug & V4L2_DEBUG_IOCTL) &&

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Sakari Ailus]

Mauro Carvalho Chehab wrote:
> Currently, -EINVAL is used to return either when an IOCTL is not
> implemented, or if the ioctl was not implemented.

Hi Mauro,

Thanks for the patch.

The V4L2 core probably should return -ENOIOCTLCMD when an IOCTL isn't 
implemented, but as long as vfs_ioctl() would stay as it is, the user 
space would still get -EINVAL. Or is vfs_ioctl() about to change?

fs/ioctl.c:
----8<-----------
static long vfs_ioctl(struct file *filp, unsigned int cmd,
                       unsigned long arg)
{
         int error = -ENOTTY;

         if (!filp->f_op || !filp->f_op->unlocked_ioctl)
                 goto out;

         error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
         if (error == -ENOIOCTLCMD)
                 error = -EINVAL;
  out:
         return error;
}
----8<-----------

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Hi Sakari,

Em 26-06-2011 12:40, Sakari Ailus escreveu:
> Mauro Carvalho Chehab wrote:
>> Currently, -EINVAL is used to return either when an IOCTL is not
>> implemented, or if the ioctl was not implemented.
> 
> Hi Mauro,
> 
> Thanks for the patch.
> 
> The V4L2 core probably should return -ENOIOCTLCMD when an IOCTL isn't implemented, but as long as vfs_ioctl() would stay as it is, the user space would still get -EINVAL. Or is vfs_ioctl() about to change?
> 
> fs/ioctl.c:
> ----8<-----------
> static long vfs_ioctl(struct file *filp, unsigned int cmd,
>                       unsigned long arg)
> {
>         int error = -ENOTTY;
> 
>         if (!filp->f_op || !filp->f_op->unlocked_ioctl)
>                 goto out;
> 
>         error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
>         if (error == -ENOIOCTLCMD)
>                 error = -EINVAL;
>  out:
>         return error;
> }
> ----8<-----------
> 

Good catch!

At the recent git history, the return for -ENOIOCTLCMD were modified
by this changeset:

commit b19dd42faf413b4705d4adb38521e82d73fa4249
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Sun Jul 4 00:15:10 2010 +0200

    bkl: Remove locked .ioctl file operation
...
@@ -39,21 +38,12 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
 {
        int error = -ENOTTY;
 
-   if (!filp->f_op)
+ if (!filp->f_op || !filp->f_op->unlocked_ioctl)
                goto out;
 
-   if (filp->f_op->unlocked_ioctl) {
-           error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
-           if (error == -ENOIOCTLCMD)
-                   error = -EINVAL;
-           goto out;
-   } else if (filp->f_op->ioctl) {
-           lock_kernel();
-           error = filp->f_op->ioctl(filp->f_path.dentry->d_inode,
-                                     filp, cmd, arg);
-           unlock_kernel();
...

Before Arnd's patch, locked ioctl's were returning -ENOIOCTLCMD, and
unlocked ones were returning -EINVAL. Now, the return of -ENOIOCTLCMD
doesn't go to userspace anymore. IMO, that's wrong and can cause
regressions, as some subsystems like DVB were returning -ENOIOCTLCMD
to userspace.

The right fix would be to remove this from fs:

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1d9b9fc..802fbbd 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -41,8 +41,6 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
 		goto out;
 
 	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
-	if (error == -ENOIOCTLCMD)
-		error = -EINVAL;
  out:
 	return error;
 }

However, the replacement from -EINVAL to -ENOIOCTLCMD is there since 2.6.12 for
unlocked_ioctl:

$ git blame b19dd42f^1 fs/ioctl.c 
...
^1da177e (Linus Torvalds    2005-04-16 15:20:36 -0700  46)              error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
^1da177e (Linus Torvalds    2005-04-16 15:20:36 -0700  47)              if (error == -ENOIOCTLCMD)
^1da177e (Linus Torvalds    2005-04-16 15:20:36 -0700  48)                      error = -EINVAL;

Linus,

what would be the expected behaviour?

Thanks,
Mauro

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Arnd Bergmann]

On Sunday 26 June 2011 18:20:21 Mauro Carvalho Chehab wrote:

> > The V4L2 core probably should return -ENOIOCTLCMD when an IOCTL isn't implemented, but as long as vfs_ioctl() would stay as it is, the user space would still get -EINVAL. Or is vfs_ioctl() about to change?
> > 
> > fs/ioctl.c:
> > ----8<-----------
> > static long vfs_ioctl(struct file *filp, unsigned int cmd,
> >                       unsigned long arg)
> > {
> >         int error = -ENOTTY;
> > 
> >         if (!filp->f_op || !filp->f_op->unlocked_ioctl)
> >                 goto out;
> > 
> >         error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
> >         if (error == -ENOIOCTLCMD)
> >                 error = -EINVAL;
> >  out:
> >         return error;
> > }
> > ----8<-----------

One of the differences between the old ->ioctl() and the ->unlocked_ioctl()
function is that unlocked_ioctl could point to the same function as
->compat_ioctl(), so we have to catch functions returning -ENOIOCTLCMD.

> Good catch!
> 
> At the recent git history, the return for -ENOIOCTLCMD were modified
> by this changeset:
> 
> commit b19dd42faf413b4705d4adb38521e82d73fa4249
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Sun Jul 4 00:15:10 2010 +0200
> 
>     bkl: Remove locked .ioctl file operation
> ...
> @@ -39,21 +38,12 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
>  {
>         int error = -ENOTTY;
>  
> -   if (!filp->f_op)
> + if (!filp->f_op || !filp->f_op->unlocked_ioctl)
>                 goto out;
>  
> -   if (filp->f_op->unlocked_ioctl) {
> -           error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
> -           if (error == -ENOIOCTLCMD)
> -                   error = -EINVAL;
> -           goto out;
> -   } else if (filp->f_op->ioctl) {
> -           lock_kernel();
> -           error = filp->f_op->ioctl(filp->f_path.dentry->d_inode,
> -                                     filp, cmd, arg);
> -           unlock_kernel();
> ...
> 
> Before Arnd's patch, locked ioctl's were returning -ENOIOCTLCMD, and
> unlocked ones were returning -EINVAL. Now, the return of -ENOIOCTLCMD
> doesn't go to userspace anymore. IMO, that's wrong and can cause
> regressions, as some subsystems like DVB were returning -ENOIOCTLCMD
> to userspace.

ENOIOCTLCMD should never be returned to user space, see the comment
in include/linux/errno.h:

/*
 * These should never be seen by user programs.  To return one of ERESTART*
 * codes, signal_pending() MUST be set.  Note that ptrace can observe these
 * at syscall exit tracing, but they will never be left for the debugged user
 * process to see.
 */

There was a lot of debate whether undefined ioctls on non-ttys should
return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to
-EINVAL at some point in the pre-git era, IIRC.

Inside of v4l2, I believe this is handled by video_usercopy(), which
turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe
where this is not done correctly and we do return ENOIOCTLCMD to
vfs_ioctl?

> The right fix would be to remove this from fs:
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1d9b9fc..802fbbd 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -41,8 +41,6 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
>  		goto out;
>  
>  	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
> -	if (error == -ENOIOCTLCMD)
> -		error = -EINVAL;
>   out:
>  	return error;
>  }
> 
> However, the replacement from -EINVAL to -ENOIOCTLCMD is there since 2.6.12 for
> unlocked_ioctl:
> 
> $ git blame b19dd42f^1 fs/ioctl.c 
> ...
> ^1da177e (Linus Torvalds    2005-04-16 15:20:36 -0700  46)              error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
> ^1da177e (Linus Torvalds    2005-04-16 15:20:36 -0700  47)              if (error == -ENOIOCTLCMD)
> ^1da177e (Linus Torvalds    2005-04-16 15:20:36 -0700  48)                      error = -EINVAL;
> 
> Linus,
> 
> what would be the expected behaviour?

Note that 1da177e is the initial commit to git, Linus did not write that
code, although he might have an opinion.

	Arnd

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 26-06-2011 14:13, Arnd Bergmann escreveu:
> On Sunday 26 June 2011 18:20:21 Mauro Carvalho Chehab wrote:
> 
>>> The V4L2 core probably should return -ENOIOCTLCMD when an IOCTL isn't implemented, but as long as vfs_ioctl() would stay as it is, the user space would still get -EINVAL. Or is vfs_ioctl() about to change?
>>>
>>> fs/ioctl.c:
>>> ----8<-----------
>>> static long vfs_ioctl(struct file *filp, unsigned int cmd,
>>>                       unsigned long arg)
>>> {
>>>         int error = -ENOTTY;
>>>
>>>         if (!filp->f_op || !filp->f_op->unlocked_ioctl)
>>>                 goto out;
>>>
>>>         error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
>>>         if (error == -ENOIOCTLCMD)
>>>                 error = -EINVAL;
>>>  out:
>>>         return error;
>>> }
>>> ----8<-----------
> 
> One of the differences between the old ->ioctl() and the ->unlocked_ioctl()
> function is that unlocked_ioctl could point to the same function as
> ->compat_ioctl(), so we have to catch functions returning -ENOIOCTLCMD.
> 
>> Good catch!
>>
>> At the recent git history, the return for -ENOIOCTLCMD were modified
>> by this changeset:
>>
>> commit b19dd42faf413b4705d4adb38521e82d73fa4249
>> Author: Arnd Bergmann <arnd@arndb.de>
>> Date:   Sun Jul 4 00:15:10 2010 +0200
>>
>>     bkl: Remove locked .ioctl file operation
>> ...
>> @@ -39,21 +38,12 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
>>  {
>>         int error = -ENOTTY;
>>  
>> -   if (!filp->f_op)
>> + if (!filp->f_op || !filp->f_op->unlocked_ioctl)
>>                 goto out;
>>  
>> -   if (filp->f_op->unlocked_ioctl) {
>> -           error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
>> -           if (error == -ENOIOCTLCMD)
>> -                   error = -EINVAL;
>> -           goto out;
>> -   } else if (filp->f_op->ioctl) {
>> -           lock_kernel();
>> -           error = filp->f_op->ioctl(filp->f_path.dentry->d_inode,
>> -                                     filp, cmd, arg);
>> -           unlock_kernel();
>> ...
>>
>> Before Arnd's patch, locked ioctl's were returning -ENOIOCTLCMD, and
>> unlocked ones were returning -EINVAL. Now, the return of -ENOIOCTLCMD
>> doesn't go to userspace anymore. IMO, that's wrong and can cause
>> regressions, as some subsystems like DVB were returning -ENOIOCTLCMD
>> to userspace.
> 
> ENOIOCTLCMD should never be returned to user space, see the comment
> in include/linux/errno.h:
> 
> /*
>  * These should never be seen by user programs.  To return one of ERESTART*
>  * codes, signal_pending() MUST be set.  Note that ptrace can observe these
>  * at syscall exit tracing, but they will never be left for the debugged user
>  * process to see.
>  */
> 

Ah, ok.

> There was a lot of debate whether undefined ioctls on non-ttys should
> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to
> -EINVAL at some point in the pre-git era, IIRC.
> 
> Inside of v4l2, I believe this is handled by video_usercopy(), which
> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe
> where this is not done correctly and we do return ENOIOCTLCMD to
> vfs_ioctl?

Well, currently, it is returning -EINVAL maybe due to the mass-conversions
you've mentioned.

The point is that -EINVAL has too many meanings at V4L. It currently can be
either that an ioctl is not supported, or that one of the parameters had
an invalid parameter. If the userspace can't distinguish between an unimplemented
ioctl and an invalid parameter, it can't decide if it needs to fall back to
some different methods of handling a V4L device.

Maybe the answer would be to return -ENOTTY when an ioctl is not implemented.

>> Linus,
>>
>> what would be the expected behaviour?
> 
> Note that 1da177e is the initial commit to git, Linus did not write that
> code, although he might have an opinion.

Yes, I know. I'm expecting his comments on it.

Mauro.

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Arnd Bergmann]

On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
> > There was a lot of debate whether undefined ioctls on non-ttys should
> > return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to
> > -EINVAL at some point in the pre-git era, IIRC.
> > 
> > Inside of v4l2, I believe this is handled by video_usercopy(), which
> > turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe
> > where this is not done correctly and we do return ENOIOCTLCMD to
> > vfs_ioctl?
> 
> Well, currently, it is returning -EINVAL maybe due to the mass-conversions
> you've mentioned.

I mean what do you return *to* vfs_ioctl from v4l? The conversions must
have been long before we introduced compat_ioctl and ENOIOCTLCMD.

As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into
EINVAL, so changing the vfs functions would not have any effect.

> The point is that -EINVAL has too many meanings at V4L. It currently can be
> either that an ioctl is not supported, or that one of the parameters had
> an invalid parameter. If the userspace can't distinguish between an unimplemented
> ioctl and an invalid parameter, it can't decide if it needs to fall back to
> some different methods of handling a V4L device.
> 
> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented.

That is what a lot of subsystems do these days. But wouldn't that change
your ABI?

	Arnd

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 26-06-2011 15:20, Arnd Bergmann escreveu:
> On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
>>> There was a lot of debate whether undefined ioctls on non-ttys should
>>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to
>>> -EINVAL at some point in the pre-git era, IIRC.
>>>
>>> Inside of v4l2, I believe this is handled by video_usercopy(), which
>>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe
>>> where this is not done correctly and we do return ENOIOCTLCMD to
>>> vfs_ioctl?
>>
>> Well, currently, it is returning -EINVAL maybe due to the mass-conversions
>> you've mentioned.
> 
> I mean what do you return *to* vfs_ioctl from v4l? The conversions must
> have been long before we introduced compat_ioctl and ENOIOCTLCMD.
> 
> As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into
> EINVAL, so changing the vfs functions would not have any effect.

Yes.  This discussion was originated by a RFC patch proposing to change 
video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.

>> The point is that -EINVAL has too many meanings at V4L. It currently can be
>> either that an ioctl is not supported, or that one of the parameters had
>> an invalid parameter. If the userspace can't distinguish between an unimplemented
>> ioctl and an invalid parameter, it can't decide if it needs to fall back to
>> some different methods of handling a V4L device.
>>
>> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented.
> 
> That is what a lot of subsystems do these days. But wouldn't that change
> your ABI?

Yes. The patch in question is also changing the DocBook spec for the ABI. We'll
likely need to drop some notes about that at the features-to-be-removed.txt.

I don't think that applications are relying at -EINVAL in order to detect if
an ioctl is not supported, but before merging such patch, we need to double-check.

Mauro.

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Arnd Bergmann]

On Sunday 26 June 2011 20:51:37 Mauro Carvalho Chehab wrote:
> > 
> > I mean what do you return to vfs_ioctl from v4l? The conversions must
> > have been long before we introduced compat_ioctl and ENOIOCTLCMD.
> > 
> > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into
> > EINVAL, so changing the vfs functions would not have any effect.
> 
> Yes.  This discussion was originated by a RFC patch proposing to change 
> video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.

Ok, I see. So returning -ENOIOCTLCMD is not an option IMHO, but if you
are confident that it doesn't break anything, returning -ENOTTY would
be possible and doesn't require any core changes.

	Arnd

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Hans Verkuil]

On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote:
> Em 26-06-2011 15:20, Arnd Bergmann escreveu:
> > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
> >>> There was a lot of debate whether undefined ioctls on non-ttys should
> >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to
> >>> -EINVAL at some point in the pre-git era, IIRC.
> >>>
> >>> Inside of v4l2, I believe this is handled by video_usercopy(), which
> >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe
> >>> where this is not done correctly and we do return ENOIOCTLCMD to
> >>> vfs_ioctl?
> >>
> >> Well, currently, it is returning -EINVAL maybe due to the mass-conversions
> >> you've mentioned.
> > 
> > I mean what do you return *to* vfs_ioctl from v4l? The conversions must
> > have been long before we introduced compat_ioctl and ENOIOCTLCMD.
> > 
> > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into
> > EINVAL, so changing the vfs functions would not have any effect.
> 
> Yes.  This discussion was originated by a RFC patch proposing to change 
> video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.
> 
> >> The point is that -EINVAL has too many meanings at V4L. It currently can be
> >> either that an ioctl is not supported, or that one of the parameters had
> >> an invalid parameter. If the userspace can't distinguish between an unimplemented
> >> ioctl and an invalid parameter, it can't decide if it needs to fall back to
> >> some different methods of handling a V4L device.
> >>
> >> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented.
> > 
> > That is what a lot of subsystems do these days. But wouldn't that change
> > your ABI?
> 
> Yes. The patch in question is also changing the DocBook spec for the ABI. We'll
> likely need to drop some notes about that at the features-to-be-removed.txt.
> 
> I don't think that applications are relying at -EINVAL in order to detect if
> an ioctl is not supported, but before merging such patch, we need to double-check.

I really don't think we can change this behavior. It's been part of the spec since
forever and it is not just open source apps that can rely on this, but also closed
source. Making an ABI change like this can really mess up applications.

We should instead review the spec and ensure that applications can discover what
is and what isn't supported through e.g. capabilities.

Regards,

	Hans

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Sakari Ailus]

Hi Hans,

On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote:
> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote:
> > Em 26-06-2011 15:20, Arnd Bergmann escreveu:
> > > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
> > >>> There was a lot of debate whether undefined ioctls on non-ttys should
> > >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY to
> > >>> -EINVAL at some point in the pre-git era, IIRC.
> > >>>
> > >>> Inside of v4l2, I believe this is handled by video_usercopy(), which
> > >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you observe
> > >>> where this is not done correctly and we do return ENOIOCTLCMD to
> > >>> vfs_ioctl?
> > >>
> > >> Well, currently, it is returning -EINVAL maybe due to the mass-conversions
> > >> you've mentioned.
> > > 
> > > I mean what do you return *to* vfs_ioctl from v4l? The conversions must
> > > have been long before we introduced compat_ioctl and ENOIOCTLCMD.
> > > 
> > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD into
> > > EINVAL, so changing the vfs functions would not have any effect.
> > 
> > Yes.  This discussion was originated by a RFC patch proposing to change 
> > video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.
> > 
> > >> The point is that -EINVAL has too many meanings at V4L. It currently can be
> > >> either that an ioctl is not supported, or that one of the parameters had
> > >> an invalid parameter. If the userspace can't distinguish between an unimplemented
> > >> ioctl and an invalid parameter, it can't decide if it needs to fall back to
> > >> some different methods of handling a V4L device.
> > >>
> > >> Maybe the answer would be to return -ENOTTY when an ioctl is not implemented.
> > > 
> > > That is what a lot of subsystems do these days. But wouldn't that change
> > > your ABI?
> > 
> > Yes. The patch in question is also changing the DocBook spec for the ABI. We'll
> > likely need to drop some notes about that at the features-to-be-removed.txt.
> > 
> > I don't think that applications are relying at -EINVAL in order to detect if
> > an ioctl is not supported, but before merging such patch, we need to double-check.
> 
> I really don't think we can change this behavior. It's been part of the spec since
> forever and it is not just open source apps that can rely on this, but also closed
> source. Making an ABI change like this can really mess up applications.
> 
> We should instead review the spec and ensure that applications can discover what
> is and what isn't supported through e.g. capabilities.

As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY
to tell that an ioctl doesn't exist; there are others. And many switched
from EINVAL they used in the past. From that point it would be good to do it
on V4L2 as well. Although I have to reckon that the V4L2 API does serve use
cases of quite different natures than these --- I can't think of an
equivalent e.g. to that astronomy application using V4L1 in the scope of
these:

Examples:
- Networking
- KVM
- SCSI/libata-scsi

Currently EINVAL is used to signal from a phletora of conditions in V4L2,
usually bad, in a way or another, parameters to an ioctl. The more low level
APIs we add (for cameras, for example), the less guessing of parameters can
be done in general. I think it would be important to distinguish the two
cases and we don't have enumeration capability (do we?) to tell which IOCTLs
the application should be expect to be able to use.

Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return
ENOTTY *right now* when the IOCTL handler is not defined. Have we heard
about this up to now? :-)

As you mention, switching to ENOTTY in general would change the ABI which
would potentially break applications. Can this be handled in a way or
another? My understanding is that not many applications would rely on EINVAL
telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its
attempt to figure out what kind of image sizes the device supports. Fixing
this would be a very small change.

In short, I think it would be beneficial to switch to ENOTTY in the long
run even if it causes some momentary pain.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Hans Verkuil]

> Hi Hans,
>
> On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote:
>> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote:
>> > Em 26-06-2011 15:20, Arnd Bergmann escreveu:
>> > > On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
>> > >>> There was a lot of debate whether undefined ioctls on non-ttys
>> should
>> > >>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY
>> to
>> > >>> -EINVAL at some point in the pre-git era, IIRC.
>> > >>>
>> > >>> Inside of v4l2, I believe this is handled by video_usercopy(),
>> which
>> > >>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you
>> observe
>> > >>> where this is not done correctly and we do return ENOIOCTLCMD to
>> > >>> vfs_ioctl?
>> > >>
>> > >> Well, currently, it is returning -EINVAL maybe due to the
>> mass-conversions
>> > >> you've mentioned.
>> > >
>> > > I mean what do you return *to* vfs_ioctl from v4l? The conversions
>> must
>> > > have been long before we introduced compat_ioctl and ENOIOCTLCMD.
>> > >
>> > > As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD
>> into
>> > > EINVAL, so changing the vfs functions would not have any effect.
>> >
>> > Yes.  This discussion was originated by a RFC patch proposing to
>> change
>> > video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.
>> >
>> > >> The point is that -EINVAL has too many meanings at V4L. It
>> currently can be
>> > >> either that an ioctl is not supported, or that one of the
>> parameters had
>> > >> an invalid parameter. If the userspace can't distinguish between an
>> unimplemented
>> > >> ioctl and an invalid parameter, it can't decide if it needs to fall
>> back to
>> > >> some different methods of handling a V4L device.
>> > >>
>> > >> Maybe the answer would be to return -ENOTTY when an ioctl is not
>> implemented.
>> > >
>> > > That is what a lot of subsystems do these days. But wouldn't that
>> change
>> > > your ABI?
>> >
>> > Yes. The patch in question is also changing the DocBook spec for the
>> ABI. We'll
>> > likely need to drop some notes about that at the
>> features-to-be-removed.txt.
>> >
>> > I don't think that applications are relying at -EINVAL in order to
>> detect if
>> > an ioctl is not supported, but before merging such patch, we need to
>> double-check.
>>
>> I really don't think we can change this behavior. It's been part of the
>> spec since
>> forever and it is not just open source apps that can rely on this, but
>> also closed
>> source. Making an ABI change like this can really mess up applications.
>>
>> We should instead review the spec and ensure that applications can
>> discover what
>> is and what isn't supported through e.g. capabilities.
>
> As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY
> to tell that an ioctl doesn't exist; there are others. And many switched
> from EINVAL they used in the past. From that point it would be good to do
> it
> on V4L2 as well. Although I have to reckon that the V4L2 API does serve
> use
> cases of quite different natures than these --- I can't think of an
> equivalent e.g. to that astronomy application using V4L1 in the scope of
> these:
>
> Examples:
> - Networking
> - KVM
> - SCSI/libata-scsi
>
> Currently EINVAL is used to signal from a phletora of conditions in V4L2,
> usually bad, in a way or another, parameters to an ioctl. The more low
> level
> APIs we add (for cameras, for example), the less guessing of parameters
> can
> be done in general. I think it would be important to distinguish the two
> cases and we don't have enumeration capability (do we?) to tell which
> IOCTLs
> the application should be expect to be able to use.

While we don't have an enum capability, in many cases you can deduce
whether a particular ioctl should be supported or not. Usually based on
capabilities, sometimes because certain ioctls allow 'NOP' operations that
allow you to test for their presence.

Of course, drivers are not always consistent here, but that's a separate
problem.

> Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return
> ENOTTY *right now* when the IOCTL handler is not defined. Have we heard
> about this up to now? :-)

No, but that's because all drivers have an ioctl handler :-) So you never
see ENOTTY.

> As you mention, switching to ENOTTY in general would change the ABI which
> would potentially break applications. Can this be handled in a way or
> another? My understanding is that not many applications would rely on
> EINVAL
> telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its
> attempt to figure out what kind of image sizes the device supports. Fixing
> this would be a very small change.
>
> In short, I think it would be beneficial to switch to ENOTTY in the long
> run even if it causes some momentary pain.

I would like that as well, but the V4L2 Specification explicitly mentions
EINVAL as the error code if an ioctl is not supported. It has done so
since it was created. You cannot just change that. And closed source
programs may  very well rely on this.

I don't think changing such an important return value is acceptable.

A better approach would be to allow applications to deduce whether ioctls
are (should be) present or not based on capabilities, etc. And document
that in the spec and ensure that drivers do this right.

The v4l2-compliance tool is already checking that where possible.

Regards,

      Hans

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 27-06-2011 09:17, Hans Verkuil escreveu:
>> Hi Hans,
>>
>> On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote:
>>> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote:
>>>> Em 26-06-2011 15:20, Arnd Bergmann escreveu:
>>>>> On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
>>>>>>> There was a lot of debate whether undefined ioctls on non-ttys
>>> should
>>>>>>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY
>>> to
>>>>>>> -EINVAL at some point in the pre-git era, IIRC.
>>>>>>>
>>>>>>> Inside of v4l2, I believe this is handled by video_usercopy(),
>>> which
>>>>>>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you
>>> observe
>>>>>>> where this is not done correctly and we do return ENOIOCTLCMD to
>>>>>>> vfs_ioctl?
>>>>>>
>>>>>> Well, currently, it is returning -EINVAL maybe due to the
>>> mass-conversions
>>>>>> you've mentioned.
>>>>>
>>>>> I mean what do you return *to* vfs_ioctl from v4l? The conversions
>>> must
>>>>> have been long before we introduced compat_ioctl and ENOIOCTLCMD.
>>>>>
>>>>> As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD
>>> into
>>>>> EINVAL, so changing the vfs functions would not have any effect.
>>>>
>>>> Yes.  This discussion was originated by a RFC patch proposing to
>>> change
>>>> video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.
>>>>
>>>>>> The point is that -EINVAL has too many meanings at V4L. It
>>> currently can be
>>>>>> either that an ioctl is not supported, or that one of the
>>> parameters had
>>>>>> an invalid parameter. If the userspace can't distinguish between an
>>> unimplemented
>>>>>> ioctl and an invalid parameter, it can't decide if it needs to fall
>>> back to
>>>>>> some different methods of handling a V4L device.
>>>>>>
>>>>>> Maybe the answer would be to return -ENOTTY when an ioctl is not
>>> implemented.
>>>>>
>>>>> That is what a lot of subsystems do these days. But wouldn't that
>>> change
>>>>> your ABI?
>>>>
>>>> Yes. The patch in question is also changing the DocBook spec for the
>>> ABI. We'll
>>>> likely need to drop some notes about that at the
>>> features-to-be-removed.txt.
>>>>
>>>> I don't think that applications are relying at -EINVAL in order to
>>> detect if
>>>> an ioctl is not supported, but before merging such patch, we need to
>>> double-check.
>>>
>>> I really don't think we can change this behavior. It's been part of the
>>> spec since
>>> forever and it is not just open source apps that can rely on this, but
>>> also closed
>>> source. Making an ABI change like this can really mess up applications.
>>>
>>> We should instead review the spec and ensure that applications can
>>> discover what
>>> is and what isn't supported through e.g. capabilities.
>>
>> As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY
>> to tell that an ioctl doesn't exist; there are others. And many switched
>> from EINVAL they used in the past. From that point it would be good to do
>> it
>> on V4L2 as well. Although I have to reckon that the V4L2 API does serve
>> use
>> cases of quite different natures than these --- I can't think of an
>> equivalent e.g. to that astronomy application using V4L1 in the scope of
>> these:
>>
>> Examples:
>> - Networking
>> - KVM
>> - SCSI/libata-scsi
>>
>> Currently EINVAL is used to signal from a phletora of conditions in V4L2,
>> usually bad, in a way or another, parameters to an ioctl. The more low
>> level
>> APIs we add (for cameras, for example), the less guessing of parameters
>> can
>> be done in general. I think it would be important to distinguish the two
>> cases and we don't have enumeration capability (do we?) to tell which
>> IOCTLs
>> the application should be expect to be able to use.
> 
> While we don't have an enum capability, in many cases you can deduce
> whether a particular ioctl should be supported or not. Usually based on
> capabilities, sometimes because certain ioctls allow 'NOP' operations that
> allow you to test for their presence.
> 
> Of course, drivers are not always consistent here, but that's a separate
> problem.

Any "hint" code that would try to do some NOP operations may fail. One of the
reasons is that such hint is not documented. Yet, I don't officially support
such "hint" methods at the API.

>> Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return
>> ENOTTY *right now* when the IOCTL handler is not defined. Have we heard
>> about this up to now? :-)
> 
> No, but that's because all drivers have an ioctl handler :-) So you never
> see ENOTTY.

Well, a V4L1 call now returns -ENOTTY, with the current behaviour. 

Btw, there are two drivers returning -ENOTTY, when the device got disconnected
(or firmware were not uploaded).

The truth is that the current API specs for return code is bogus.

> 
>> As you mention, switching to ENOTTY in general would change the ABI which
>> would potentially break applications. Can this be handled in a way or
>> another? My understanding is that not many applications would rely on
>> EINVAL
>> telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its
>> attempt to figure out what kind of image sizes the device supports. Fixing
>> this would be a very small change.
>>
>> In short, I think it would be beneficial to switch to ENOTTY in the long
>> run even if it causes some momentary pain.
> 
> I would like that as well, but the V4L2 Specification explicitly mentions
> EINVAL as the error code if an ioctl is not supported. It has done so
> since it was created. You cannot just change that. And closed source
> programs may  very well rely on this.

The V4L2 spec needs to be fixed with respect to error codes. Driver authors
are much more creative than DocBook authors ;) There are a lot of return
codes used by the drivers whose API spec doesn't mention (and, on this subject, 
the same applies to the DVB API). What I've seen is that:
- Sometimes, a core return code is returned. One of the important examples is
  the ENOSPC error returned when the usb core refuses to stream when the USB
  bus reached 80% of the available bandwidth. There's a patch floating around that
  would allow to override the 80% hard limit, via sysfs. So, if properly documented,
  an userspace application could give a hint that the user needs to either use a
  different bus or try to change the hard limit;
- For every ioctl, it presents its own "private" list of error codes. If someone wants
  to add a new code (for example, standardizing ENOTTY or ENOSPC), all affected
  ioctl's will need to be touched. This is hard to maintain;
- Drivers are not compliant with error codes.

The right thing to do is to create a separate chapter for error codes, based on errno(3)
man page, where we document all error codes that should be used by the drivers. Then,
at the ioctl pages, link to the common chapter and, only when needed, document special
cases where an error code for that specific ioctl has some special meaning.

I ran a script here to check how many different error codes are used inside drivers/media:

$ find drivers/media -type f -name '*.[ch]'  >files
$ grep define `find . -name errno*.h`|perl -ne 'print "$1\n" if (/\#define\s+(E[^\s]+)/)'|sort|uniq >errors
$ for i in `cat errors`; do COUNT=$(git grep -c $i `cat files`|wc -l); if [ "$COUNT" != "0" ]; then echo $i $COUNT; fi; done

The result is that we're using 53 different types of errors, but the API specs documents
only 17 of them. Those are the currently used errors at drivers/media:

ERROR CODE     |NUMBER OF *.c/*.h FILES USING IT
---------------|--------------------------------
E2BIG           1
EACCES          8
EAGAIN          66
EBADF           1
EBADFD          1
EBADR           2
EBADRQC         2
EBUSY           149
ECHILD          1
ECONNRESET      25
EDEADLK         1
EDOM            1
EEXIST          3
EFAULT          230
EFBIG           1
EILSEQ          8
EINIT           2
EINPROGRESS     6
EINTR           21
EINVAL          501
EIO             305
EMFILE          1
ENFILE          7
ENOBUFS         7
ENODATA         4
ENODEV          270
ENOENT          46
ENOIOCTLCMD     31
ENOMEM          359
ENOSPC          13
ENOSR           7
ENOSYS          15
ENOTSUP         3
ENOTSUPP        3
ENOTTY          5
ENXIO           26
EOPNOTSUPP      19
EOVERFLOW       14
EPERM           47
EPIPE           12
EPROTO          11
ERANGE          25
EREMOTE         80
EREMOTEIO       80
ERESTART        32
ERESTARTSYS     32
ESHUTDOWN       27
ESPIPE          3
ETIME           53
ETIMEDOUT       37
EUSERS          2
EWOULDBLOCK     14
EXDEV           1

I suspect that we'll need to both fix some drivers, and the API, as I bet that
the same error conditions are reported differently on different drivers.

> I don't think changing such an important return value is acceptable.

As I said, the current API is bogus with respect to error codes. Of course,
we need to do take care to avoid userspace applications breakage, but we can't
use the excuse that it is there for a long time as a reason for not fixing it.

> A better approach would be to allow applications to deduce whether ioctls
> are (should be) present or not based on capabilities, etc. And document
> that in the spec and ensure that drivers do this right.
> 
> The v4l2-compliance tool is already checking that where possible.
> 
> Regards,
> 
>       Hans
> 

Thanks,
Mauro

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Hans Verkuil]

On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote:
> Em 27-06-2011 09:17, Hans Verkuil escreveu:
> >> Hi Hans,
> >>
> >> On Mon, Jun 27, 2011 at 07:38:27AM +0200, Hans Verkuil wrote:
> >>> On Sunday, June 26, 2011 20:51:37 Mauro Carvalho Chehab wrote:
> >>>> Em 26-06-2011 15:20, Arnd Bergmann escreveu:
> >>>>> On Sunday 26 June 2011 19:30:46 Mauro Carvalho Chehab wrote:
> >>>>>>> There was a lot of debate whether undefined ioctls on non-ttys
> >>> should
> >>>>>>> return -EINVAL or -ENOTTY, including mass-conversions from -ENOTTY
> >>> to
> >>>>>>> -EINVAL at some point in the pre-git era, IIRC.
> >>>>>>>
> >>>>>>> Inside of v4l2, I believe this is handled by video_usercopy(),
> >>> which
> >>>>>>> turns the driver's -ENOIOCTLCMD into -ENOTTY. What cases do you
> >>> observe
> >>>>>>> where this is not done correctly and we do return ENOIOCTLCMD to
> >>>>>>> vfs_ioctl?
> >>>>>>
> >>>>>> Well, currently, it is returning -EINVAL maybe due to the
> >>> mass-conversions
> >>>>>> you've mentioned.
> >>>>>
> >>>>> I mean what do you return *to* vfs_ioctl from v4l? The conversions
> >>> must
> >>>>> have been long before we introduced compat_ioctl and ENOIOCTLCMD.
> >>>>>
> >>>>> As far as I can tell, video_ioctl2 has always converted ENOIOCTLCMD
> >>> into
> >>>>> EINVAL, so changing the vfs functions would not have any effect.
> >>>>
> >>>> Yes.  This discussion was originated by a RFC patch proposing to
> >>> change
> >>>> video_ioctl2 to return -ENOIOCTLCMD instead of -EINVAL.
> >>>>
> >>>>>> The point is that -EINVAL has too many meanings at V4L. It
> >>> currently can be
> >>>>>> either that an ioctl is not supported, or that one of the
> >>> parameters had
> >>>>>> an invalid parameter. If the userspace can't distinguish between an
> >>> unimplemented
> >>>>>> ioctl and an invalid parameter, it can't decide if it needs to fall
> >>> back to
> >>>>>> some different methods of handling a V4L device.
> >>>>>>
> >>>>>> Maybe the answer would be to return -ENOTTY when an ioctl is not
> >>> implemented.
> >>>>>
> >>>>> That is what a lot of subsystems do these days. But wouldn't that
> >>> change
> >>>>> your ABI?
> >>>>
> >>>> Yes. The patch in question is also changing the DocBook spec for the
> >>> ABI. We'll
> >>>> likely need to drop some notes about that at the
> >>> features-to-be-removed.txt.
> >>>>
> >>>> I don't think that applications are relying at -EINVAL in order to
> >>> detect if
> >>>> an ioctl is not supported, but before merging such patch, we need to
> >>> double-check.
> >>>
> >>> I really don't think we can change this behavior. It's been part of the
> >>> spec since
> >>> forever and it is not just open source apps that can rely on this, but
> >>> also closed
> >>> source. Making an ABI change like this can really mess up applications.
> >>>
> >>> We should instead review the spec and ensure that applications can
> >>> discover what
> >>> is and what isn't supported through e.g. capabilities.
> >>
> >> As far as I understand, V4L2 wouldn't be the only kernel API to use ENOTTY
> >> to tell that an ioctl doesn't exist; there are others. And many switched
> >> from EINVAL they used in the past. From that point it would be good to do
> >> it
> >> on V4L2 as well. Although I have to reckon that the V4L2 API does serve
> >> use
> >> cases of quite different natures than these --- I can't think of an
> >> equivalent e.g. to that astronomy application using V4L1 in the scope of
> >> these:
> >>
> >> Examples:
> >> - Networking
> >> - KVM
> >> - SCSI/libata-scsi
> >>
> >> Currently EINVAL is used to signal from a phletora of conditions in V4L2,
> >> usually bad, in a way or another, parameters to an ioctl. The more low
> >> level
> >> APIs we add (for cameras, for example), the less guessing of parameters
> >> can
> >> be done in general. I think it would be important to distinguish the two
> >> cases and we don't have enumeration capability (do we?) to tell which
> >> IOCTLs
> >> the application should be expect to be able to use.
> > 
> > While we don't have an enum capability, in many cases you can deduce
> > whether a particular ioctl should be supported or not. Usually based on
> > capabilities, sometimes because certain ioctls allow 'NOP' operations that
> > allow you to test for their presence.
> > 
> > Of course, drivers are not always consistent here, but that's a separate
> > problem.
> 
> Any "hint" code that would try to do some NOP operations may fail. One of the
> reasons is that such hint is not documented. Yet, I don't officially support
> such "hint" methods at the API.

The point is that the spec can easily be improved to make such 'NOP' operations
explicit, or to require that if a capability is present, then the corresponding
ioctl(s) must also be present. Things like that are easy to verify as well with
v4l2-compliance.

> >> Interestingly enough, V4L2 core (v4l2_ioctl() in v4l2-dev.c) does return
> >> ENOTTY *right now* when the IOCTL handler is not defined. Have we heard
> >> about this up to now? :-)
> > 
> > No, but that's because all drivers have an ioctl handler :-) So you never
> > see ENOTTY.
> 
> Well, a V4L1 call now returns -ENOTTY, with the current behaviour. 

How? I don't see that. All drivers have an ioctl handler, so all ioctls will
go through that handler, which returns -EINVAL for V4L1 ioctls.

> Btw, there are two drivers returning -ENOTTY, when the device got disconnected
> (or firmware were not uploaded).
> 
> The truth is that the current API specs for return code is bogus.

Bogus in what way? It's been documented very clearly for years. We may not like
that design decision (I certainly don't like it), but someone clearly thought
about it at the time.

You know that I am usually more than willing to make/accept ABI changes, especially
in ambiguous circumstances (and we have a lot of those). But this particular one
is actually for once consistently implemented in all drivers and apps can (and
probably do) rely on them. And that includes also closed source applications,
and there is no way we can change those. Neither is this something that only
affects some niche products, this is subsystem-wide.

> 
> > 
> >> As you mention, switching to ENOTTY in general would change the ABI which
> >> would potentially break applications. Can this be handled in a way or
> >> another? My understanding is that not many applications would rely on
> >> EINVAL
> >> telling an IOCTL isn't implemented. GStreamer v4l2src might be one in its
> >> attempt to figure out what kind of image sizes the device supports. Fixing
> >> this would be a very small change.
> >>
> >> In short, I think it would be beneficial to switch to ENOTTY in the long
> >> run even if it causes some momentary pain.
> > 
> > I would like that as well, but the V4L2 Specification explicitly mentions
> > EINVAL as the error code if an ioctl is not supported. It has done so
> > since it was created. You cannot just change that. And closed source
> > programs may  very well rely on this.
> 
> The V4L2 spec needs to be fixed with respect to error codes. Driver authors
> are much more creative than DocBook authors ;) There are a lot of return
> codes used by the drivers whose API spec doesn't mention (and, on this subject, 
> the same applies to the DVB API). What I've seen is that:
> - Sometimes, a core return code is returned. One of the important examples is
>   the ENOSPC error returned when the usb core refuses to stream when the USB
>   bus reached 80% of the available bandwidth. There's a patch floating around that
>   would allow to override the 80% hard limit, via sysfs. So, if properly documented,
>   an userspace application could give a hint that the user needs to either use a
>   different bus or try to change the hard limit;
> - For every ioctl, it presents its own "private" list of error codes. If someone wants
>   to add a new code (for example, standardizing ENOTTY or ENOSPC), all affected
>   ioctl's will need to be touched. This is hard to maintain;
> - Drivers are not compliant with error codes.
> 
> The right thing to do is to create a separate chapter for error codes, based on errno(3)
> man page, where we document all error codes that should be used by the drivers. Then,
> at the ioctl pages, link to the common chapter and, only when needed, document special
> cases where an error code for that specific ioctl has some special meaning.

Great, I've no problem with that. But this particular error code you want to change
is actually implemented *consistently* in all drivers. There is no confusion, no
ambiguity, and it is according to the spec.

> I ran a script here to check how many different error codes are used inside drivers/media:
> 
> $ find drivers/media -type f -name '*.[ch]'  >files
> $ grep define `find . -name errno*.h`|perl -ne 'print "$1\n" if (/\#define\s+(E[^\s]+)/)'|sort|uniq >errors
> $ for i in `cat errors`; do COUNT=$(git grep -c $i `cat files`|wc -l); if [ "$COUNT" != "0" ]; then echo $i $COUNT; fi; done
> 
> The result is that we're using 53 different types of errors, but the API specs documents
> only 17 of them. Those are the currently used errors at drivers/media:
> 
> ERROR CODE     |NUMBER OF *.c/*.h FILES USING IT
> ---------------|--------------------------------
> E2BIG           1
> EACCES          8
> EAGAIN          66
> EBADF           1
> EBADFD          1
> EBADR           2
> EBADRQC         2
> EBUSY           149
> ECHILD          1
> ECONNRESET      25
> EDEADLK         1
> EDOM            1
> EEXIST          3
> EFAULT          230
> EFBIG           1
> EILSEQ          8
> EINIT           2
> EINPROGRESS     6
> EINTR           21
> EINVAL          501
> EIO             305
> EMFILE          1
> ENFILE          7
> ENOBUFS         7
> ENODATA         4
> ENODEV          270
> ENOENT          46
> ENOIOCTLCMD     31
> ENOMEM          359
> ENOSPC          13
> ENOSR           7
> ENOSYS          15
> ENOTSUP         3
> ENOTSUPP        3
> ENOTTY          5
> ENXIO           26
> EOPNOTSUPP      19
> EOVERFLOW       14
> EPERM           47
> EPIPE           12
> EPROTO          11
> ERANGE          25
> EREMOTE         80
> EREMOTEIO       80
> ERESTART        32
> ERESTARTSYS     32
> ESHUTDOWN       27
> ESPIPE          3
> ETIME           53
> ETIMEDOUT       37
> EUSERS          2
> EWOULDBLOCK     14
> EXDEV           1
> 
> I suspect that we'll need to both fix some drivers, and the API, as I bet that
> the same error conditions are reported differently on different drivers.
> 
> > I don't think changing such an important return value is acceptable.
> 
> As I said, the current API is bogus with respect to error codes. Of course,
> we need to do take care to avoid userspace applications breakage, but we can't
> use the excuse that it is there for a long time as a reason for not fixing it.

The fact that many drivers use error codes creatively doesn't give us an excuse
to just change the one error code that is actually used everywhere according to
the spec! That's faulty logic.

Regards,

	Hans

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 27-06-2011 11:56, Hans Verkuil escreveu:
> On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote:
>> Em 27-06-2011 09:17, Hans Verkuil escreveu:
>>> While we don't have an enum capability, in many cases you can deduce
>>> whether a particular ioctl should be supported or not. Usually based on
>>> capabilities, sometimes because certain ioctls allow 'NOP' operations that
>>> allow you to test for their presence.
>>>
>>> Of course, drivers are not always consistent here, but that's a separate
>>> problem.
>>
>> Any "hint" code that would try to do some NOP operations may fail. One of the
>> reasons is that such hint is not documented. Yet, I don't officially support
>> such "hint" methods at the API.
> 
> The point is that the spec can easily be improved to make such 'NOP' operations
> explicit, or to require that if a capability is present, then the corresponding
> ioctl(s) must also be present. Things like that are easy to verify as well with
> v4l2-compliance.

We currently have more than 64 ioctl's. Adding a capability bit for each doesn't
seem the right thing to do. Ok, some could be grouped, but, even so, there are
drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S.
So, I think we don't have enough available bits for doing that.

>> Btw, there are two drivers returning -ENOTTY, when the device got disconnected
>> (or firmware were not uploaded).
>>
>> The truth is that the current API specs for return code is bogus.
> 
> Bogus in what way? It's been documented very clearly for years. We may not like
> that design decision (I certainly don't like it), but someone clearly thought
> about it at the time.

Bogus in the sense that drivers don't follow them, as they're returning undocumented
values. Any application strictly following it will have troubles.

>> The right thing to do is to create a separate chapter for error codes, based on errno(3)
>> man page, where we document all error codes that should be used by the drivers. Then,
>> at the ioctl pages, link to the common chapter and, only when needed, document special
>> cases where an error code for that specific ioctl has some special meaning.
> 
> Great, I've no problem with that. But this particular error code you want to change
> is actually implemented *consistently* in all drivers. There is no confusion, no
> ambiguity, and it is according to the spec.

As I said, from userspace perspective, it is not consistent to assume that EINVAL means
not implemented. For sure at VIDIOC_S_foo, this is not consistent. Even on some GET types
of ioctl, like for example [1][2], there are other reasons for an EINVAL return.

[1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-cropcap.html
[2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-audio.html

The only way to make it consistent is to use different return codes for "invalid parameters" 
and for "unsupported ioctl".

>> I ran a script here to check how many different error codes are used inside drivers/media:
>>
>> $ find drivers/media -type f -name '*.[ch]'  >files
>> $ grep define `find . -name errno*.h`|perl -ne 'print "$1\n" if (/\#define\s+(E[^\s]+)/)'|sort|uniq >errors
>> $ for i in `cat errors`; do COUNT=$(git grep -c $i `cat files`|wc -l); if [ "$COUNT" != "0" ]; then echo $i $COUNT; fi; done
>>
>> The result is that we're using 53 different types of errors, but the API specs documents
>> only 17 of them. Those are the currently used errors at drivers/media:
>>
>> ERROR CODE     |NUMBER OF *.c/*.h FILES USING IT
>> ---------------|--------------------------------
>> E2BIG           1
>> EACCES          8
>> EAGAIN          66
>> EBADF           1
>> EBADFD          1
>> EBADR           2
>> EBADRQC         2
>> EBUSY           149
>> ECHILD          1
>> ECONNRESET      25
>> EDEADLK         1
>> EDOM            1
>> EEXIST          3
>> EFAULT          230
>> EFBIG           1
>> EILSEQ          8
>> EINIT           2
>> EINPROGRESS     6
>> EINTR           21
>> EINVAL          501
>> EIO             305
>> EMFILE          1
>> ENFILE          7
>> ENOBUFS         7
>> ENODATA         4
>> ENODEV          270
>> ENOENT          46
>> ENOIOCTLCMD     31
>> ENOMEM          359
>> ENOSPC          13
>> ENOSR           7
>> ENOSYS          15
>> ENOTSUP         3
>> ENOTSUPP        3
>> ENOTTY          5
>> ENXIO           26
>> EOPNOTSUPP      19
>> EOVERFLOW       14
>> EPERM           47
>> EPIPE           12
>> EPROTO          11
>> ERANGE          25
>> EREMOTE         80
>> EREMOTEIO       80
>> ERESTART        32
>> ERESTARTSYS     32
>> ESHUTDOWN       27
>> ESPIPE          3
>> ETIME           53
>> ETIMEDOUT       37
>> EUSERS          2
>> EWOULDBLOCK     14
>> EXDEV           1
>>
>> I suspect that we'll need to both fix some drivers, and the API, as I bet that
>> the same error conditions are reported differently on different drivers.
>>
>>> I don't think changing such an important return value is acceptable.
>>
>> As I said, the current API is bogus with respect to error codes. Of course,
>> we need to do take care to avoid userspace applications breakage, but we can't
>> use the excuse that it is there for a long time as a reason for not fixing it.
> 
> The fact that many drivers use error codes creatively doesn't give us an excuse
> to just change the one error code that is actually used everywhere according to
> the spec! That's faulty logic.

The fix that it is needed is to provide a consistent way for an userspace application
to know for sure when an ioctl is not supported. It can be done on a simple way of
just returning a different error code for it, or with complex mechanisms like adding
a per-ioctl flag and some hint logics based on NOP.

The V4L2 is complex enough for us to add more complexity with hints and cap flags.

Thanks,
Mauro

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Arnd Bergmann]

On Monday 27 June 2011, Mauro Carvalho Chehab wrote:
> > The point is that the spec can easily be improved to make such 'NOP' operations
> > explicit, or to require that if a capability is present, then the corresponding
> > ioctl(s) must also be present. Things like that are easy to verify as well with
> > v4l2-compliance.
> 
> We currently have more than 64 ioctl's. Adding a capability bit for each doesn't
> seem the right thing to do. Ok, some could be grouped, but, even so, there are
> drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S.
> So, I think we don't have enough available bits for doing that.

It shouldn't be too hard to do an ioctl command that returns a le_bitmask with the
ioctl command number as an index (0 to 91, currently), and the bit set for each
command that has the corresponding v4l2_ioctl_ops member filled for the device.
That would be an obvious way to query the operations, but I don't know if it's
useful.

	Arnd

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 27-06-2011 13:14, Arnd Bergmann escreveu:
> On Monday 27 June 2011, Mauro Carvalho Chehab wrote:
>>> The point is that the spec can easily be improved to make such 'NOP' operations
>>> explicit, or to require that if a capability is present, then the corresponding
>>> ioctl(s) must also be present. Things like that are easy to verify as well with
>>> v4l2-compliance.
>>
>> We currently have more than 64 ioctl's. Adding a capability bit for each doesn't
>> seem the right thing to do. Ok, some could be grouped, but, even so, there are
>> drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S.
>> So, I think we don't have enough available bits for doing that.
> 
> It shouldn't be too hard to do an ioctl command that returns a le_bitmask with the
> ioctl command number as an index (0 to 91, currently), and the bit set for each
> command that has the corresponding v4l2_ioctl_ops member filled for the device.
> That would be an obvious way to query the operations, but I don't know if it's
> useful.

Tricks like that could be done, but that would not be consistent with other subsystems
that use -ENOTTY for such purpose. 

Also, to avoid having magic numbers, this will end by adding a somewhat complex logic 
at userspace (or having some sort of macro exported together with videodev2.h), in 
order to associate a V4L2 ioctl with the corresponding le_bitmask bit.

Also, 91 ioctls seems a complex-enough API to me. We should avoid adding more stuff there
without very good reasons.

In any case, we should also double check how this is done by the other API's used at the
media subsystem, and try to do the same there (lirc, Media Controller and DVB APIs).

My intention is to split the error handling changes from the original series that remove
the linux/version.h, and spend some time preparing a new RFC about that, trying to cover
all the API's defined under drivers/media. One idea could be to postpone a decision about
it, and discuss this topic further during the media workshop at the KS/2011.

Thanks,
Mauro.

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Hans Verkuil]

On Monday, June 27, 2011 17:33:58 Mauro Carvalho Chehab wrote:
> Em 27-06-2011 11:56, Hans Verkuil escreveu:
> > On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote:
> >> Em 27-06-2011 09:17, Hans Verkuil escreveu:
> >>> While we don't have an enum capability, in many cases you can deduce
> >>> whether a particular ioctl should be supported or not. Usually based on
> >>> capabilities, sometimes because certain ioctls allow 'NOP' operations that
> >>> allow you to test for their presence.
> >>>
> >>> Of course, drivers are not always consistent here, but that's a separate
> >>> problem.
> >>
> >> Any "hint" code that would try to do some NOP operations may fail. One of the
> >> reasons is that such hint is not documented. Yet, I don't officially support
> >> such "hint" methods at the API.
> > 
> > The point is that the spec can easily be improved to make such 'NOP' operations
> > explicit, or to require that if a capability is present, then the corresponding
> > ioctl(s) must also be present. Things like that are easy to verify as well with
> > v4l2-compliance.
> 
> We currently have more than 64 ioctl's. Adding a capability bit for each doesn't
> seem the right thing to do. Ok, some could be grouped, but, even so, there are
> drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S.
> So, I think we don't have enough available bits for doing that.

No, that's not what I meant.

Whether or not ioctls are implemented can in many cases be deduced from the
QUERYCAP capabilities: e.g. if V4L2_CAP_STREAMING is set, then the buffer I/O
ioctls have to be there. For other ioctls the test whether they are implemented
is also often straightforward: e.g. if VIDIOC_G_INPUT returns -EINVAL, then
that can only mean that it isn't implemented, and neither are ENUM_INPUT and
S_INPUT.

In cases where only the G variant is implemented, there we need to tighten
the spec and require that these ioctls are properly implemented: you either
implement all of the ENUM/G/TRY/S ioctls or none.

Currently ENUM_FRAMESIZES/INTERVALS is one set of ioctls where this is very
ambiguous. For most of the others it is pretty straightforward.

> >> Btw, there are two drivers returning -ENOTTY, when the device got disconnected
> >> (or firmware were not uploaded).
> >>
> >> The truth is that the current API specs for return code is bogus.
> > 
> > Bogus in what way? It's been documented very clearly for years. We may not like
> > that design decision (I certainly don't like it), but someone clearly thought
> > about it at the time.
> 
> Bogus in the sense that drivers don't follow them, as they're returning undocumented
> values. Any application strictly following it will have troubles.

I suspect that in most cases the drivers are fairly reasonable, but the spec
wasn't updated with the new error codes.

> >> The right thing to do is to create a separate chapter for error codes, based on errno(3)
> >> man page, where we document all error codes that should be used by the drivers. Then,
> >> at the ioctl pages, link to the common chapter and, only when needed, document special
> >> cases where an error code for that specific ioctl has some special meaning.
> > 
> > Great, I've no problem with that. But this particular error code you want to change
> > is actually implemented *consistently* in all drivers. There is no confusion, no
> > ambiguity, and it is according to the spec.
> 
> As I said, from userspace perspective, it is not consistent to assume that EINVAL means
> not implemented. For sure at VIDIOC_S_foo, this is not consistent. Even on some GET types
> of ioctl, like for example [1][2], there are other reasons for an EINVAL return.
> 
> [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-cropcap.html
> [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-audio.html
> 
> The only way to make it consistent is to use different return codes for "invalid parameters" 
> and for "unsupported ioctl".

No, what we do is perfectly consistent: i.e. we always return EINVAL when an
ioctl is not supported. That's what 'consistent' means. Whether that is the
*right* error code is something else.

But right now our API and our documentation is perfectly consistent and has been
for years.

<snip>

> > The fact that many drivers use error codes creatively doesn't give us an excuse
> > to just change the one error code that is actually used everywhere according to
> > the spec! That's faulty logic.
> 
> The fix that it is needed is to provide a consistent way for an userspace application
> to know for sure when an ioctl is not supported. It can be done on a simple way of
> just returning a different error code for it, or with complex mechanisms like adding
> a per-ioctl flag and some hint logics based on NOP.

It's a 'fix' that I fear may break applications because drivers suddenly change
their behavior. You can't just ignore that. You also can't analyze applications,
since closed source apps may also use it, and we obviously have no control over
those.

> The V4L2 is complex enough for us to add more complexity with hints and cap flags.

Actually, all it needs for the most part is that current implicit rules are made
explicit: if a certain querycap flag is set, then a corresponding set of ioctls
must be implemented. If I can call the GET ioctl, then the ENUM and SET (and
TRY) must also be implemented. Sensible rules at any time, they just need to be
made explicit.

If you want to change it to ENOTTY, then go right ahead. But you can explain
it to our customers when their app suddenly breaks for some hardware.

I don't get it. This is really not a problem. We rarely, if ever, get complaints
about it, and there is a ton of other much more important stuff that needs to
be done.

Regards,

	Hans

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 27-06-2011 14:07, Hans Verkuil escreveu:
> On Monday, June 27, 2011 17:33:58 Mauro Carvalho Chehab wrote:
>> Em 27-06-2011 11:56, Hans Verkuil escreveu:
>>> On Monday, June 27, 2011 15:54:11 Mauro Carvalho Chehab wrote:
>>>> Em 27-06-2011 09:17, Hans Verkuil escreveu:
>>>>> While we don't have an enum capability, in many cases you can deduce
>>>>> whether a particular ioctl should be supported or not. Usually based on
>>>>> capabilities, sometimes because certain ioctls allow 'NOP' operations that
>>>>> allow you to test for their presence.
>>>>>
>>>>> Of course, drivers are not always consistent here, but that's a separate
>>>>> problem.
>>>>
>>>> Any "hint" code that would try to do some NOP operations may fail. One of the
>>>> reasons is that such hint is not documented. Yet, I don't officially support
>>>> such "hint" methods at the API.
>>>
>>> The point is that the spec can easily be improved to make such 'NOP' operations
>>> explicit, or to require that if a capability is present, then the corresponding
>>> ioctl(s) must also be present. Things like that are easy to verify as well with
>>> v4l2-compliance.
>>
>> We currently have more than 64 ioctl's. Adding a capability bit for each doesn't
>> seem the right thing to do. Ok, some could be grouped, but, even so, there are
>> drivers that implement the VIDIOC_G, but doesn't implement the corresponding VIDIO_S.
>> So, I think we don't have enough available bits for doing that.
> 
> No, that's not what I meant.
> 
> Whether or not ioctls are implemented can in many cases be deduced from the
> QUERYCAP capabilities: e.g. if V4L2_CAP_STREAMING is set, then the buffer I/O
> ioctls have to be there.

No. only the MMAP-based and OVERLAY-based ioctls could be deduced from the flags
there, plus hw freq seek. Anything else will be just a guess.

> For other ioctls the test whether they are implemented
> is also often straightforward: e.g. if VIDIOC_G_INPUT returns -EINVAL, then
> that can only mean that it isn't implemented, and neither are ENUM_INPUT and
> S_INPUT.

At the best, it is a hint coding.

> In cases where only the G variant is implemented, there we need to tighten
> the spec and require that these ioctls are properly implemented: you either
> implement all of the ENUM/G/TRY/S ioctls or none.

The V4L1 compat layer required that some VIDIOC_G variants to be implemented for
some things to work. This code were moved to userspace, but the requirement is
probably still there.

In other words, drivers had/have no option but implementing some VIDIOC_G even
when VIDIOC_S is not supported.

So, changing the V4L2 spec to require that all 3 to be implemented will also 
cause compatibility issues that will be harder to map than the check for a proper
check of the EINVAL return code.

> Currently ENUM_FRAMESIZES/INTERVALS is one set of ioctls where this is very
> ambiguous. For most of the others it is pretty straightforward.

>>>> Btw, there are two drivers returning -ENOTTY, when the device got disconnected
>>>> (or firmware were not uploaded).
>>>>
>>>> The truth is that the current API specs for return code is bogus.
>>>
>>> Bogus in what way? It's been documented very clearly for years. We may not like
>>> that design decision (I certainly don't like it), but someone clearly thought
>>> about it at the time.
>>
>> Bogus in the sense that drivers don't follow them, as they're returning undocumented
>> values. Any application strictly following it will have troubles.
> 
> I suspect that in most cases the drivers are fairly reasonable, but the spec
> wasn't updated with the new error codes.

If driver foo returns ENOTTY because the device got removed and driver bar returns another
error, they're not consistent. Developers tried to get the errors that they considered to
be the more applicable to that situation, but, as different developers took different
decisions, the end result is that several error codes will need to be fixed^Wchanged, in
order to allow userspace to properly deal/report with such error conditions.

>>>> The right thing to do is to create a separate chapter for error codes, based on errno(3)
>>>> man page, where we document all error codes that should be used by the drivers. Then,
>>>> at the ioctl pages, link to the common chapter and, only when needed, document special
>>>> cases where an error code for that specific ioctl has some special meaning.
>>>
>>> Great, I've no problem with that. But this particular error code you want to change
>>> is actually implemented *consistently* in all drivers. There is no confusion, no
>>> ambiguity, and it is according to the spec.
>>
>> As I said, from userspace perspective, it is not consistent to assume that EINVAL means
>> not implemented. For sure at VIDIOC_S_foo, this is not consistent. Even on some GET types
>> of ioctl, like for example [1][2], there are other reasons for an EINVAL return.
>>
>> [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-cropcap.html
>> [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-audio.html
>>
>> The only way to make it consistent is to use different return codes for "invalid parameters" 
>> and for "unsupported ioctl".
> 
> No, what we do is perfectly consistent: i.e. we always return EINVAL when an
> ioctl is not supported. That's what 'consistent' means. Whether that is the
> *right* error code is something else.
> 
> But right now our API and our documentation is perfectly consistent and has been
> for years.
> 
> <snip>
> 
>>> The fact that many drivers use error codes creatively doesn't give us an excuse
>>> to just change the one error code that is actually used everywhere according to
>>> the spec! That's faulty logic.
>>
>> The fix that it is needed is to provide a consistent way for an userspace application
>> to know for sure when an ioctl is not supported. It can be done on a simple way of
>> just returning a different error code for it, or with complex mechanisms like adding
>> a per-ioctl flag and some hint logics based on NOP.
> 
> It's a 'fix' that I fear may break applications because drivers suddenly change
> their behavior. You can't just ignore that. You also can't analyze applications,
> since closed source apps may also use it, and we obviously have no control over
> those.

That's the price that a closed source app pays: we can't do much to detect or prevent 
breaking a closed source application. A simple driver patch may hurt those applications, 
and only the app developer can fix. On the other hand, closed-source applications have
a team of developers paid to keep their application working. So, if properly announced,
they can change their code in advance in order to check for the QUERYCAP version and
handle the new behaviour accordingly.

I think that we should take a look at the existing open source applications and see how
they handle -EINVAL, sending patches to them fixing the behaviour, if they're broken
by such change.

>> The V4L2 is complex enough for us to add more complexity with hints and cap flags.
> 
> Actually, all it needs for the most part is that current implicit rules are made
> explicit: if a certain querycap flag is set, then a corresponding set of ioctls
> must be implemented. If I can call the GET ioctl, then the ENUM and SET (and
> TRY) must also be implemented. Sensible rules at any time, they just need to be
> made explicit.
> 
> If you want to change it to ENOTTY, then go right ahead. But you can explain
> it to our customers when their app suddenly breaks for some hardware.
> 
> I don't get it. This is really not a problem. We rarely, if ever, get complaints
> about it, and there is a ton of other much more important stuff that needs to
> be done.
> 
> Regards,
> 
> 	Hans

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Linus Torvalds]

On Mon, Jun 27, 2011 at 10:07 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> No, what we do is perfectly consistent: i.e. we always return EINVAL when an
> ioctl is not supported. That's what 'consistent' means. Whether that is the
> *right* error code is something else.

You don't even understand the problem.

The problem is two-fold:
 (a) it's the _wrong_ error code (this part you seem to get)
but also
 (b) you ALSO return -EINVAL for ioctl's you support!

How har dis (b) to understand? The fact is, -EINVAL does not mean "I
don't support that ioctl". It _should_ mean "I _do_ support that
ioctl, but you passed me bogus arguments for that ioctl that I cannot
handle".

And right now, for the v4l crowd, -EINVAL means "random error code
that _could_ be because the ioctl doesn't exist, but it could also be
because the ioctl _does_ exist but didn't like it's value".

See the problem?

The correct error code for "I don't understand this ioctl" is ENOTTY.
The naming may be odd, but you should think of that error value as a
"unrecognized ioctl number, you're feeding me random numbers that I
don't understand and I assume for historical reasons that you tried to
do some tty operation on me".

I don't understand why the v4l people have such a hard time getting
this. This has been going on for years.

I would suggest that somebody just switch around the EINVAL in
__video_do_ioctl() to -ENOTTY (and change the ENOIOCTLCMD translation
to also make it ENOTTY instead of EINVAL) and just try to see if
anything breaks.

Maybe things actually break, and we'd have to undo it for
compatibility reasons (and perhaps add a comment about the program
that is so flaky that it needs a EINVAL return from the ioctl), but I
really do not understand people like you who seem to argue against
doing the right thing without even _trying_ it.

Saying that your API is "consistent" is clearly bullshit. Your API is
_wrong_. It's not consistent. Returning EINVAL can mean two different
things, there's no way to tell which case it was - that's not
"consistent" by any stretch of the imagination.

                            Linus

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Hans Verkuil]

On Monday, June 27, 2011 22:48:48 Linus Torvalds wrote:
> On Mon, Jun 27, 2011 at 10:07 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > No, what we do is perfectly consistent: i.e. we always return EINVAL when an
> > ioctl is not supported. That's what 'consistent' means. Whether that is the
> > *right* error code is something else.
> 
> You don't even understand the problem.
> 
> The problem is two-fold:
>  (a) it's the _wrong_ error code (this part you seem to get)
> but also
>  (b) you ALSO return -EINVAL for ioctl's you support!
> 
> How har dis (b) to understand? The fact is, -EINVAL does not mean "I
> don't support that ioctl". It _should_ mean "I _do_ support that
> ioctl, but you passed me bogus arguments for that ioctl that I cannot
> handle".
> 
> And right now, for the v4l crowd, -EINVAL means "random error code
> that _could_ be because the ioctl doesn't exist, but it could also be
> because the ioctl _does_ exist but didn't like it's value".
> 
> See the problem?

Duh. Of course I see the problem. I've seen the problem ever since I started
working on V4L back in 2004.

Of course, since vfs_ioctl does the same thing this is clearly isn't some
isolated V4L thing. Pot. Kettle.
 
> The correct error code for "I don't understand this ioctl" is ENOTTY.
> The naming may be odd, but you should think of that error value as a
> "unrecognized ioctl number, you're feeding me random numbers that I
> don't understand and I assume for historical reasons that you tried to
> do some tty operation on me".
> 
> I don't understand why the v4l people have such a hard time getting
> this. This has been going on for years.
> 
> I would suggest that somebody just switch around the EINVAL in
> __video_do_ioctl() to -ENOTTY (and change the ENOIOCTLCMD translation
> to also make it ENOTTY instead of EINVAL) and just try to see if
> anything breaks.
> 
> Maybe things actually break, and we'd have to undo it for
> compatibility reasons (and perhaps add a comment about the program
> that is so flaky that it needs a EINVAL return from the ioctl), but I
> really do not understand people like you who seem to argue against
> doing the right thing without even _trying_ it.
> 
> Saying that your API is "consistent" is clearly bullshit. Your API is
> _wrong_. It's not consistent. Returning EINVAL can mean two different
> things, there's no way to tell which case it was - that's not
> "consistent" by any stretch of the imagination.

I call it consistently wrong :-)

Listen, this error code is wrong. But it unfortunately has been documented
like that for years. It's part of the V4L userspace API. I have no clue who
may rely on this. I have no clue what might break if we change it. And the
only way to know that is by actually releasing a kernel with that change.

It was my understanding that we shouldn't break the userspace API. This breaks
the userspace API. If everyone else says it's fine to break the userspace API
this time, then who am I to object?

Regards,

	Hans

> 
>                             Linus
> 

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Linus Torvalds]

On Mon, Jun 27, 2011 at 11:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Of course, since vfs_ioctl does the same thing this is clearly isn't some
> isolated V4L thing. Pot. Kettle.

Oh, absolutely. V4L is not at all the only confused user around.

The EINVAL thing goes way back, and is a disaster. It predates Linux
itself, as far as I can tell. You'll fin dlots of man-pages that have
this line in it:

  EINVAL Request or argp is not valid.

and it shows up in POSIX etc. And sadly, it generally shows up
_before_ the line that says

  ENOTTY The specified request does not apply to the kind of object
that the descriptor d references.

so a lot of people get to the EINVAL, and never even notice the ENOTTY.

And the above is from the current Linux man-pages - they are better
than most. Googling for posix and ioctl, the first hit is some
streams-specific man-page that is much worse, and makes you really
think that EINVAL would be the right error for a bad ioctl.

The reason is probably that ENOTTY has always been a "wft?" kind of
error code. It doesn't make sense as a name for some filesystem or
driver writer. Any sane person goes "Of _course_ it's not a tty, why
would I say that?"

At least glibc (and hopefully other C libraries) use a _string_ that
makes much more sense: strerror(ENOTTY) is "Inappropriate ioctl for
device"

The particular translation of ENOIOCTLCMD to EINVAL was always a
mistake. I'll happily try to change it for 3.1, but I'll need to do it
early in the merge window to check for any issues.

(In fact, the _correct_ thing to do would probably be to just do

   #define ENOIOCTLCMD ENOTTY

and get rid of any translation - just giving ENOTTY a more appropriate
name and less chance for confusion)

> It was my understanding that we shouldn't break the userspace API. This breaks
> the userspace API. If everyone else says it's fine to break the userspace API
> this time, then who am I to object?

We are indeed never supposed to break userspace.

But that does not mean that we cannot fix bugs. It only means exactly
what it says: we cannot break user space.

If we fix a bug, and it turns out that user space actually depends on
that bug, then we need to revert the fix. But it never means "you can
never make any changes at all to interfaces". It only means "you
cannot make any changes that break applications".

There may be applications out there that really break when they get
ENOTTY instead of EINVAL. But most cases that check for errors from
ioctl's tend to just say "did this succeed or not" rather than "did
this return EINVAL". That's *doubly* true since the error code has
been ambiguous, so checking for the exact error code has always been
pretty pointless.

So the common use of the error code tends to be for things like
strerror(), and then it would be a real improvement to show
"Inappropriate ioctl for device" when the device doesn't support that
ioctl, wouldn't it?

But yes, let's try to fix it, and if it turns out that that breaks
something, we must simply revert and probably add a comment to the
source code ("EINVAL is wrong, it should be ENOTTY, but xyzzy only
works with EINVAL").

                                       Linus

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Alan Cox]

> (In fact, the _correct_ thing to do would probably be to just do
> 
>    #define ENOIOCTLCMD ENOTTY
> 
> and get rid of any translation - just giving ENOTTY a more appropriate
> name and less chance for confusion)

Some code uses the two to separate 'the driver specific helper code
doesn't handle this' and 'does handle this'. In that situation you take
away the ability of a driver to override a midlayer ioctl with -ENOTTY to
say "I don't support this even if most people do"

> There may be applications out there that really break when they get
> ENOTTY instead of EINVAL. But most cases that check for errors from
> ioctl's tend to just say "did this succeed or not" rather than "did
> this return EINVAL". That's *doubly* true since the error code has
> been ambiguous, so checking for the exact error code has always been
> pretty pointless.

Chances are if anything is busted its busted the other way on Linux and
expects -ENOTTY. Certainly the large number I've been fixing over time
haven't shown up any problems.

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Arnd Bergmann]

On Tuesday 28 June 2011, Alan Cox wrote:
> > (In fact, the correct thing to do would probably be to just do
> > 
> >    #define ENOIOCTLCMD ENOTTY
> > 
> > and get rid of any translation - just giving ENOTTY a more appropriate
> > name and less chance for confusion)
> 
> Some code uses the two to separate 'the driver specific helper code
> doesn't handle this' and 'does handle this'. In that situation you take
> away the ability of a driver to override a midlayer ioctl with -ENOTTY to
> say "I don't support this even if most people do"

Right. Similarly, in compat_sys_ioctl returning -ENOIOCTLCMD from
fops->compat_ioctl means "the driver has provided no compatibility
handler for this command, need to check the global translation table",
while -ENOTTY returned from ->compat_ioctl means "this command won't
work on this device, don't bother looking at the table and don't
print an annoying message".

	Arnd

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 28-06-2011 13:05, Linus Torvalds escreveu:
> On Mon, Jun 27, 2011 at 11:04 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:

>> It was my understanding that we shouldn't break the userspace API. This breaks
>> the userspace API. If everyone else says it's fine to break the userspace API
>> this time, then who am I to object?
> 
> We are indeed never supposed to break userspace.
> 
> But that does not mean that we cannot fix bugs. It only means exactly
> what it says: we cannot break user space.
> 
> If we fix a bug, and it turns out that user space actually depends on
> that bug, then we need to revert the fix. But it never means "you can
> never make any changes at all to interfaces". It only means "you
> cannot make any changes that break applications".
> 
> There may be applications out there that really break when they get
> ENOTTY instead of EINVAL. But most cases that check for errors from
> ioctl's tend to just say "did this succeed or not" rather than "did
> this return EINVAL". That's *doubly* true since the error code has
> been ambiguous, so checking for the exact error code has always been
> pretty pointless.
> 
> So the common use of the error code tends to be for things like
> strerror(), and then it would be a real improvement to show
> "Inappropriate ioctl for device" when the device doesn't support that
> ioctl, wouldn't it?
> 
> But yes, let's try to fix it, and if it turns out that that breaks
> something, we must simply revert and probably add a comment to the
> source code ("EINVAL is wrong, it should be ENOTTY, but xyzzy only
> works with EINVAL").

I've applied the fix locally, using ENOTTY, and tested with 3 drivers that
have several non-implemented ioctl's: vivi, uvcvideo and gspca (they basically
don't implement tuner ioctl's. The uvcvideo doesn't implement the *STD ioctls,
and the gspca driver doesn't implement the ENUM_FRAME* ioctl's).

I tested with camorama (using libv4l1 conversion code), vlc, qv4l2, xawtv3 and 
mplayer. I also tested two closed source applications (Skype and flash - at twitcam). 
With all drivers, the applications worked as expected.

There is just a minor glitch with xawtv3, as it currently doesn't print an
error log if VIDIOC_G_STD returns -EINVAL when debug is disabled.
Yet, the error is not fatal and happens only once, during device probing.
Xawtv works fine, and suppressing the noise is an easy fix. As we're releasing 
version 1.101 of it with alsa streaming support, I'll latter add a code there
fixing it.

Complex TV applications like mythtv won't work if the devices don't implement
tuner and control ioctl's. I don't believe that it would fail if we change the
return code for unimplemented ioctls, because if an ioctl that mythtv
needs is not implemented, it will fail anyway. Unfortunately, installing
mythtv is a very complex task, and requires hours/days of work. I can't
affort to install it for testing ATM, but I'm sure others have it installed,
and can provide us some feedback.

So, I'll prepare the patches for replacing EINVAL to ENOTTY and add it at
next. This'll give us some time to get feedback about eventual breakages
on other tools that I didn't test.

Thanks,
Mauro

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Andy Walls]

On Mon, 2011-06-27 at 10:54 -0300, Mauro Carvalho Chehab wrote:

> The right thing to do is to create a separate chapter for error codes, based on errno(3)
> man page [snip]

IMO, the IEEE Std 1003.1 is a better source for errno definitions than
the errno(3) manpage:

http://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_03.html

Regards,
Andy

Re: [PATCH] [media] v4l2 core: return -ENOIOCTLCMD if an ioctl doesn't exist

[Mauro Carvalho Chehab]

Em 27-06-2011 12:12, Andy Walls escreveu:
> On Mon, 2011-06-27 at 10:54 -0300, Mauro Carvalho Chehab wrote:
> 
>> The right thing to do is to create a separate chapter for error codes, based on errno(3)
>> man page [snip]
> 
> IMO, the IEEE Std 1003.1 is a better source for errno definitions than
> the errno(3) manpage:
> 
> http://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_03.html

True, but it doesn't cover all error types. For sure we need to use the POSIX standard
when working at the DocBook.

Cheers,
Mauro