[PATCH] staging: comedi: replace ENOSYS by proper error codes

[PATCH] staging: comedi: replace ENOSYS by proper error codes

[julien.dehee]

From: Julien Dehee <julien.dehee@gmail.com>

comedi/comedi_fops.c
    use ENODEV following open manual

comedi/drivers.c
    use ENOTTY following ioctl manual

drivers/serial2002.c
    use ENOTTY following ioctl manual

Signed-off-by: Julien Dehee <julien.dehee@gmail.com>
---
 drivers/staging/comedi/comedi_fops.c        | 2 +-
 drivers/staging/comedi/drivers.c            | 2 +-
 drivers/staging/comedi/drivers/serial2002.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 146ab00..6896a1f 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -2611,7 +2611,7 @@ static int comedi_open(struct inode *inode, struct file *file)
 	}
 	if (dev->attached && dev->use_count == 0) {
 		if (!try_module_get(dev->driver->module)) {
-			rc = -ENOSYS;
+			rc = -ENODEV;
 			goto out;
 		}
 		if (dev->open) {
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index ed0b60c..db89096 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -820,7 +820,7 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 			 "driver '%s' does not support attach using comedi_config\n",
 			 driv->driver_name);
 		module_put(driv->module);
-		ret = -ENOSYS;
+		ret = -ENOTTY;
 		goto out;
 	}
 	dev->driver = driv;
diff --git a/drivers/staging/comedi/drivers/serial2002.c b/drivers/staging/comedi/drivers/serial2002.c
index 83da162..929bf20 100644
--- a/drivers/staging/comedi/drivers/serial2002.c
+++ b/drivers/staging/comedi/drivers/serial2002.c
@@ -101,7 +101,7 @@ static long serial2002_tty_ioctl(struct file *f, unsigned op,
 	if (f->f_op->unlocked_ioctl)
 		return f->f_op->unlocked_ioctl(f, op, param);
 
-	return -ENOSYS;
+	return -ENOTTY;
 }
 
 static int serial2002_tty_write(struct file *f, unsigned char *buf, int count)
-- 
1.9.1

Re: [PATCH] staging: comedi: replace ENOSYS by proper error codes

[Greg KH]

On Fri, Jun 12, 2015 at 10:20:38PM +0200, julien.dehee@gmail.com wrote:
> From: Julien Dehee <julien.dehee@gmail.com>
> 
> comedi/comedi_fops.c
>     use ENODEV following open manual
> 
> comedi/drivers.c
>     use ENOTTY following ioctl manual
> 
> drivers/serial2002.c
>     use ENOTTY following ioctl manual

What do you mean by "ioctl manual"?

And shouldn't this be 3 different patches as they do different things to
different drivers?

thanks,

greg k-h

Re: [PATCH] staging: comedi: replace ENOSYS by proper error codes

[One Thousand Gnomes]

On Fri, 12 Jun 2015 13:43:27 -0700
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Jun 12, 2015 at 10:20:38PM +0200, julien.dehee@gmail.com wrote:
> > From: Julien Dehee <julien.dehee@gmail.com>
> > 
> > comedi/comedi_fops.c
> >     use ENODEV following open manual

That should probably be ENXIO by a strict reading, but Linux has always
used ENODEV 8)

> > 
> > comedi/drivers.c
> >     use ENOTTY following ioctl manual
> > 
> > drivers/serial2002.c
> >     use ENOTTY following ioctl manual
> 
> What do you mean by "ioctl manual"?

man 2 ioctl

Unknown ioctls on a device should error with ENOTTY. It's one of
those crazy pieces of Unix history.

Re: [PATCH] staging: comedi: replace ENOSYS by proper error codes

[Greg KH]

On Fri, Jun 12, 2015 at 10:06:16PM +0100, One Thousand Gnomes wrote:
> On Fri, 12 Jun 2015 13:43:27 -0700
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Jun 12, 2015 at 10:20:38PM +0200, julien.dehee@gmail.com wrote:
> > > From: Julien Dehee <julien.dehee@gmail.com>
> > > 
> > > comedi/comedi_fops.c
> > >     use ENODEV following open manual
> 
> That should probably be ENXIO by a strict reading, but Linux has always
> used ENODEV 8)
> 
> > > 
> > > comedi/drivers.c
> > >     use ENOTTY following ioctl manual
> > > 
> > > drivers/serial2002.c
> > >     use ENOTTY following ioctl manual
> > 
> > What do you mean by "ioctl manual"?
> 
> man 2 ioctl
> 
> Unknown ioctls on a device should error with ENOTTY. It's one of
> those crazy pieces of Unix history.

Note that the man 2 ioctl interface is not always what the kernel
exposes, but rather, what your libc exposes to other programs.  So
setting these kernel values might not change what you get all the way
through, have you tested it?

thanks,

greg k-h

Re: [PATCH] staging: comedi: replace ENOSYS by proper error codes

[Greg KH]

On Fri, Jun 12, 2015 at 03:26:26PM -0700, Greg KH wrote:
> On Fri, Jun 12, 2015 at 10:06:16PM +0100, One Thousand Gnomes wrote:
> > On Fri, 12 Jun 2015 13:43:27 -0700
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Fri, Jun 12, 2015 at 10:20:38PM +0200, julien.dehee@gmail.com wrote:
> > > > From: Julien Dehee <julien.dehee@gmail.com>
> > > > 
> > > > comedi/comedi_fops.c
> > > >     use ENODEV following open manual
> > 
> > That should probably be ENXIO by a strict reading, but Linux has always
> > used ENODEV 8)
> > 
> > > > 
> > > > comedi/drivers.c
> > > >     use ENOTTY following ioctl manual
> > > > 
> > > > drivers/serial2002.c
> > > >     use ENOTTY following ioctl manual
> > > 
> > > What do you mean by "ioctl manual"?
> > 
> > man 2 ioctl
> > 
> > Unknown ioctls on a device should error with ENOTTY. It's one of
> > those crazy pieces of Unix history.
> 
> Note that the man 2 ioctl interface is not always what the kernel
> exposes, but rather, what your libc exposes to other programs.  So
> setting these kernel values might not change what you get all the way
> through, have you tested it?

We also have the issue of the existing userspace comedi code, I don't
know if it's handling the ioctl errors differently than ioctl(2) says,
but that should be checked as well before I can take this patch.

thanks,

greg k-h

Re: [PATCH] staging: comedi: replace ENOSYS by proper error codes

[Ian Abbott]

On 12/06/15 21:20, julien.dehee@gmail.com wrote:
> From: Julien Dehee <julien.dehee@gmail.com>
>
> comedi/comedi_fops.c
>      use ENODEV following open manual
>
> comedi/drivers.c
>      use ENOTTY following ioctl manual
>
> drivers/serial2002.c
>      use ENOTTY following ioctl manual
>
> Signed-off-by: Julien Dehee <julien.dehee@gmail.com>
> ---
>   drivers/staging/comedi/comedi_fops.c        | 2 +-
>   drivers/staging/comedi/drivers.c            | 2 +-
>   drivers/staging/comedi/drivers/serial2002.c | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
> index 146ab00..6896a1f 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2611,7 +2611,7 @@ static int comedi_open(struct inode *inode, struct file *file)
>   	}
>   	if (dev->attached && dev->use_count == 0) {
>   		if (!try_module_get(dev->driver->module)) {
> -			rc = -ENOSYS;
> +			rc = -ENODEV;
>   			goto out;

Seems reasonable as driver for the device is being unloaded.

>   		}
>   		if (dev->open) {
> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> index ed0b60c..db89096 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -820,7 +820,7 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>   			 "driver '%s' does not support attach using comedi_config\n",
>   			 driv->driver_name);
>   		module_put(driv->module);
> -		ret = -ENOSYS;
> +		ret = -ENOTTY;
>   		goto out;

I don't think ENOTTY is the correct error code here. the 
COMEDI_DEVCONFIG ioctl code is an "appropriate" ioctl to send to a 
comedi device.  I'd suggest ENOTSUP as a replacement.

>   	}
>   	dev->driver = driv;
> diff --git a/drivers/staging/comedi/drivers/serial2002.c b/drivers/staging/comedi/drivers/serial2002.c
> index 83da162..929bf20 100644
> --- a/drivers/staging/comedi/drivers/serial2002.c
> +++ b/drivers/staging/comedi/drivers/serial2002.c
> @@ -101,7 +101,7 @@ static long serial2002_tty_ioctl(struct file *f, unsigned op,
>   	if (f->f_op->unlocked_ioctl)
>   		return f->f_op->unlocked_ioctl(f, op, param);
>
> -	return -ENOSYS;
> +	return -ENOTTY;

That is reasonable.  It shouldn't happen anyway if 'f' really is linked 
to a tty device.  And the return value is currently ignored anyway!

>   }
>
>   static int serial2002_tty_write(struct file *f, unsigned char *buf, int count)
>

As Greg mentioned, it should be three different patches.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-