iio: core: return ENODEV if ioctl is unknown

Commit Message

Alexandru Ardelean May 3, 2021, 2:43 p.m. UTC

When the ioctl() mechanism was introduced in IIO core to centralize the
registration of all ioctls in one place via commit 8dedcc3eee3ac ("iio:
core: centralize ioctl() calls to the main chardev"), the return code was
changed from ENODEV to EINVAL, when the ioctl code isn't known.

This was done by accident.

This change reverts back to the old behavior, where if the ioctl() code
isn't known, ENODEV is returned (vs EINVAL).

This was brought into perspective by this patch:
  https://lore.kernel.org/linux-iio/[e-mail]/

Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the main chardev")
Cc: Linus Walleij <[e-mail]>
Cc: Paul Cercueil <[e-mail]>
Cc: Nuno Sa <[e-mail]>
Signed-off-by: Alexandru Ardelean <[e-mail]>
---
 drivers/iio/industrialio-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Nuno Sa May 4, 2021, 7:10 a.m. UTC | #1

> -----Original Message-----
> From: Alexandru Ardelean <[e-mail]>
> Sent: Monday, May 3, 2021 4:44 PM
> To: [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; Alexandru Ardelean
> <[e-mail]>; Linus Walleij <[e-mail]>;
> Paul Cercueil <[e-mail]>; Sa, Nuno
> <[e-mail]>
> Subject: [PATCH] iio: core: return ENODEV if ioctl is unknown
>  
> When the ioctl() mechanism was introduced in IIO core to centralize
> the
> registration of all ioctls in one place via commit 8dedcc3eee3ac ("iio:
> core: centralize ioctl() calls to the main chardev"), the return code was
> changed from ENODEV to EINVAL, when the ioctl code isn't known.
> 
> This was done by accident.
> 
> This change reverts back to the old behavior, where if the ioctl() code
> isn't known, ENODEV is returned (vs EINVAL).
> 
> This was brought into perspective by this patch:
>   https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20210428150815.136150-1-
> [e-mail]/__;!!A3Ni8CS0y2Y!rbiZhkgdYhOPdii_YEAgO0BlX
> UxYANj3juvQuoWtaH61DSon2xK5i_BRar5Jyw$
> 
> Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the main
> chardev")
> Cc: Linus Walleij <[e-mail]>
> Cc: Paul Cercueil <[e-mail]>
> Cc: Nuno Sa <[e-mail]>
> Signed-off-by: Alexandru Ardelean <[e-mail]>
> ---

Reviewed-by: Nuno Sá <[e-mail]>

>  drivers/iio/industrialio-core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> core.c
> index efb4cf91c9e4..9a3a83211a90 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1803,7 +1803,6 @@ static long iio_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
>  	if (!indio_dev->info)
>  		goto out_unlock;
> 
> -	ret = -EINVAL;
>  	list_for_each_entry(h, &iio_dev_opaque->ioctl_handlers,
> entry) {
>  		ret = h->ioctl(indio_dev, filp, cmd, arg);
>  		if (ret != IIO_IOCTL_UNHANDLED)
> @@ -1811,7 +1810,7 @@ static long iio_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
>  	}
> 
>  	if (ret == IIO_IOCTL_UNHANDLED)
> -		ret = -EINVAL;
> +		ret = -ENODEV;
> 
>  out_unlock:
>  	mutex_unlock(&iio_dev_opaque->info_exist_lock);
> --
> 2.31.1

- Nuno Sá

Paul Cercueil May 4, 2021, 9:04 a.m. UTC | #2

Le lun., mai 3 2021 at 17:43:50 +0300, Alexandru Ardelean 
<[e-mail]> a écrit :
> When the ioctl() mechanism was introduced in IIO core to centralize 
> the
> registration of all ioctls in one place via commit 8dedcc3eee3ac 
> ("iio:
> core: centralize ioctl() calls to the main chardev"), the return code 
> was
> changed from ENODEV to EINVAL, when the ioctl code isn't known.
> 
> This was done by accident.
> 
> This change reverts back to the old behavior, where if the ioctl() 
> code
> isn't known, ENODEV is returned (vs EINVAL).
> 
> This was brought into perspective by this patch:
>   
> https://lore.kernel.org/linux-iio/[e-mail]/
> 
> Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the 
> main chardev")
> Cc: Linus Walleij <[e-mail]>
> Cc: Paul Cercueil <[e-mail]>
> Cc: Nuno Sa <[e-mail]>
> Signed-off-by: Alexandru Ardelean <[e-mail]>

Tested-by: Paul Cercueil <[e-mail]>

Thanks!
-Paul

> ---
>  drivers/iio/industrialio-core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c 
> b/drivers/iio/industrialio-core.c
> index efb4cf91c9e4..9a3a83211a90 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1803,7 +1803,6 @@ static long iio_ioctl(struct file *filp, 
> unsigned int cmd, unsigned long arg)
>  	if (!indio_dev->info)
>  		goto out_unlock;
> 
> -	ret = -EINVAL;
>  	list_for_each_entry(h, &iio_dev_opaque->ioctl_handlers, entry) {
>  		ret = h->ioctl(indio_dev, filp, cmd, arg);
>  		if (ret != IIO_IOCTL_UNHANDLED)
> @@ -1811,7 +1810,7 @@ static long iio_ioctl(struct file *filp, 
> unsigned int cmd, unsigned long arg)
>  	}
> 
>  	if (ret == IIO_IOCTL_UNHANDLED)
> -		ret = -EINVAL;
> +		ret = -ENODEV;
> 
>  out_unlock:
>  	mutex_unlock(&iio_dev_opaque->info_exist_lock);
> --
> 2.31.1
>

Linus Walleij May 5, 2021, 12:52 p.m. UTC | #3

On Mon, May 3, 2021 at 4:43 PM Alexandru Ardelean <[e-mail]> wrote:

> When the ioctl() mechanism was introduced in IIO core to centralize the
> registration of all ioctls in one place via commit 8dedcc3eee3ac ("iio:
> core: centralize ioctl() calls to the main chardev"), the return code was
> changed from ENODEV to EINVAL, when the ioctl code isn't known.
>
> This was done by accident.
>
> This change reverts back to the old behavior, where if the ioctl() code
> isn't known, ENODEV is returned (vs EINVAL).
>
> This was brought into perspective by this patch:
>   https://lore.kernel.org/linux-iio/[e-mail]/
>
> Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the main chardev")
> Cc: Linus Walleij <[e-mail]>
> Cc: Paul Cercueil <[e-mail]>
> Cc: Nuno Sa <[e-mail]>
> Signed-off-by: Alexandru Ardelean <[e-mail]>

Reviewed-by: Linus Walleij <[e-mail]>

Yours,
Linus Walleij

Jonathan Cameron May 8, 2021, 3:16 p.m. UTC | #4

On Mon,  3 May 2021 17:43:50 +0300
Alexandru Ardelean <[e-mail]> wrote:

> When the ioctl() mechanism was introduced in IIO core to centralize the
> registration of all ioctls in one place via commit 8dedcc3eee3ac ("iio:
> core: centralize ioctl() calls to the main chardev"), the return code was
> changed from ENODEV to EINVAL, when the ioctl code isn't known.
> 
> This was done by accident.
> 
> This change reverts back to the old behavior, where if the ioctl() code
> isn't known, ENODEV is returned (vs EINVAL).
> 
> This was brought into perspective by this patch:
>   https://lore.kernel.org/linux-iio/[e-mail]/
> 
> Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the main chardev")
> Cc: Linus Walleij <[e-mail]>
> Cc: Paul Cercueil <[e-mail]>
> Cc: Nuno Sa <[e-mail]>
> Signed-off-by: Alexandru Ardelean <[e-mail]>

This is going to be a little messy to apply as lots of churn in that file.
What I've done for now is pulled the fixes-togreg branch forwards onto
current staging/staging-linus but I'll do that again after
staging/staging-linus moves onto an rc1 or similar base.

Anyhow, applied to that tree which is going to have more than it's
normal share of rebases this cycle.

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index efb4cf91c9e4..9a3a83211a90 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1803,7 +1803,6 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	if (!indio_dev->info)
>  		goto out_unlock;
>  
> -	ret = -EINVAL;
>  	list_for_each_entry(h, &iio_dev_opaque->ioctl_handlers, entry) {
>  		ret = h->ioctl(indio_dev, filp, cmd, arg);
>  		if (ret != IIO_IOCTL_UNHANDLED)
> @@ -1811,7 +1810,7 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	}
>  
>  	if (ret == IIO_IOCTL_UNHANDLED)
> -		ret = -EINVAL;
> +		ret = -ENODEV;
>  
>  out_unlock:
>  	mutex_unlock(&iio_dev_opaque->info_exist_lock);

Linus Walleij May 8, 2021, 6:21 p.m. UTC | #5

On Sat, May 8, 2021 at 5:15 PM Jonathan Cameron <[e-mail]> wrote:
> On Mon,  3 May 2021 17:43:50 +0300 Alexandru Ardelean <[e-mail]> wrote:
>
> > When the ioctl() mechanism was introduced in IIO core to centralize the
> > registration of all ioctls in one place via commit 8dedcc3eee3ac ("iio:
> > core: centralize ioctl() calls to the main chardev"), the return code was
> > changed from ENODEV to EINVAL, when the ioctl code isn't known.
> >
> > This was done by accident.
> >
> > This change reverts back to the old behavior, where if the ioctl() code
> > isn't known, ENODEV is returned (vs EINVAL).
> >
> > This was brought into perspective by this patch:
> >   https://lore.kernel.org/linux-iio/[e-mail]/
> >
> > Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the main chardev")
> > Cc: Linus Walleij <[e-mail]>
> > Cc: Paul Cercueil <[e-mail]>
> > Cc: Nuno Sa <[e-mail]>
> > Signed-off-by: Alexandru Ardelean <[e-mail]>
>
> This is going to be a little messy to apply as lots of churn in that file.
> What I've done for now is pulled the fixes-togreg branch forwards onto
> current staging/staging-linus but I'll do that again after
> staging/staging-linus moves onto an rc1 or similar base.

This is starting to become a recurring problem is it not?

Have you considered the option to start to send your pull
requests to Linus (Torvalds) directly?

I suppose the current scheme is used because IIO changes
can affect drivers/staging/ but at this point that thing is
so much smaller than the stuff in drivers/iio proper that
I start to question if it's worth it.

Unless you really like to base your work on Gregs tree for
some reason or other, that is.

Yours,
Linus Walleij

Jonathan Cameron May 9, 2021, 10:19 a.m. UTC | #6

On Sat, 8 May 2021 20:21:08 +0200
Linus Walleij <[e-mail]> wrote:

> On Sat, May 8, 2021 at 5:15 PM Jonathan Cameron <[e-mail]> wrote:
> > On Mon,  3 May 2021 17:43:50 +0300 Alexandru Ardelean <[e-mail]> wrote:
> >  
> > > When the ioctl() mechanism was introduced in IIO core to centralize the
> > > registration of all ioctls in one place via commit 8dedcc3eee3ac ("iio:
> > > core: centralize ioctl() calls to the main chardev"), the return code was
> > > changed from ENODEV to EINVAL, when the ioctl code isn't known.
> > >
> > > This was done by accident.
> > >
> > > This change reverts back to the old behavior, where if the ioctl() code
> > > isn't known, ENODEV is returned (vs EINVAL).
> > >
> > > This was brought into perspective by this patch:
> > >   https://lore.kernel.org/linux-iio/[e-mail]/
> > >
> > > Fixes: 8dedcc3eee3ac ("iio: core: centralize ioctl() calls to the main chardev")
> > > Cc: Linus Walleij <[e-mail]>
> > > Cc: Paul Cercueil <[e-mail]>
> > > Cc: Nuno Sa <[e-mail]>
> > > Signed-off-by: Alexandru Ardelean <[e-mail]>  
> >
> > This is going to be a little messy to apply as lots of churn in that file.
> > What I've done for now is pulled the fixes-togreg branch forwards onto
> > current staging/staging-linus but I'll do that again after
> > staging/staging-linus moves onto an rc1 or similar base.  
> 
> This is starting to become a recurring problem is it not?

This particular one isn't a result of the path IIO patches take,
but rather my jumping the gun on getting these out there before
there is an rc1 release.  So would have been in the same position
but referring to Linus' tree rather than Greg's if things went
directly.

> 
> Have you considered the option to start to send your pull
> requests to Linus (Torvalds) directly?
> 
> I suppose the current scheme is used because IIO changes
> can affect drivers/staging/ but at this point that thing is
> so much smaller than the stuff in drivers/iio proper that
> I start to question if it's worth it.

I guess the perfectionist in me wants to clear the remaining
stuff out of staging before changing the structure that has
worked well (and become very comfortable!). Perhaps you are
right that I shouldn't wait quite as long as that will take.

> 
> Unless you really like to base your work on Gregs tree for
> some reason or other, that is.

Definitely appreciate Greg's help (and patience), but no
particularly strong reason to waste his time dealing with my
mess ups. Hopefully they'll reduce now IIO trees are going directly
into linux-next though.

> 
> Yours,
> Linus Walleij

Linus Walleij May 9, 2021, 3:29 p.m. UTC | #7

On Sun, May 9, 2021 at 12:18 PM Jonathan Cameron <[e-mail]> wrote:
> On Sat, 8 May 2021 20:21:08 +0200
> Linus Walleij <[e-mail]> wrote:

> > Unless you really like to base your work on Gregs tree for
> > some reason or other, that is.
>
> Definitely appreciate Greg's help (and patience), but no
> particularly strong reason to waste his time dealing with my
> mess ups. Hopefully they'll reduce now IIO trees are going directly
> into linux-next though.

I'd suggest to move to sending pulls to Torvalds directly
for IIO to cut the intermediary staging tree step, since
now the subsystem is pretty large and see a bunch of
frequent fixes that need an express path to Torvalds.

Pushing through Greg per se isn't really the problem,
I think the problem is that IIO is going through the
staging tree which (I guess) isn't a high priority activity
and not expected to carry any serious critical fixes and
I guess this can cause lags.

Maybe Greg has some other branch to take in IIO
fixes and for-next but I don't really see the point.

The IIO left in the staging tree is just regular staging
business at this point, the main IIO is much more
important.

Linus 2: would pulling the IIO tree directly work for
you if Jonathan makes up his mind in favor for that?

Yours,
Linus Walleij

Greg Kroah-Hartman May 10, 2021, 6:48 a.m. UTC | #8

On Sun, May 09, 2021 at 05:29:10PM +0200, Linus Walleij wrote:
> On Sun, May 9, 2021 at 12:18 PM Jonathan Cameron <[e-mail]> wrote:
> > On Sat, 8 May 2021 20:21:08 +0200
> > Linus Walleij <[e-mail]> wrote:
> 
> > > Unless you really like to base your work on Gregs tree for
> > > some reason or other, that is.
> >
> > Definitely appreciate Greg's help (and patience), but no
> > particularly strong reason to waste his time dealing with my
> > mess ups. Hopefully they'll reduce now IIO trees are going directly
> > into linux-next though.
> 
> I'd suggest to move to sending pulls to Torvalds directly
> for IIO to cut the intermediary staging tree step, since
> now the subsystem is pretty large and see a bunch of
> frequent fixes that need an express path to Torvalds.
> 
> Pushing through Greg per se isn't really the problem,
> I think the problem is that IIO is going through the
> staging tree which (I guess) isn't a high priority activity
> and not expected to carry any serious critical fixes and
> I guess this can cause lags.
> 
> Maybe Greg has some other branch to take in IIO
> fixes and for-next but I don't really see the point.

I can take IIO changes in my char/misc tree like many other driver
subsystems go, if the staging portions are not involved.  Otherwise, I
really don't see the problem with it as-is, what problems is this
causing at the moment?

thanks,

greg k-h

Linus Walleij May 18, 2021, 12:31 a.m. UTC | #9

On Mon, May 10, 2021 at 8:48 AM Greg KH <[e-mail]> wrote:

> I can take IIO changes in my char/misc tree like many other driver
> subsystems go, if the staging portions are not involved.  Otherwise, I
> really don't see the problem with it as-is, what problems is this
> causing at the moment?

It's in the thread: pipeline stalls, haha :)

It has happened more than once that Jonathan needs to wait for
things to percolate upstream before he can base new stuff on it.

Personally I've encountered fixes that are waiting in your tree
so that new fixes on fixes, or next development cannot be applied
because the fixes need to land in a tag upstream so that can be
merged in first as base for the new development. Essentially
any time patches with dependencies end up on two branches.

Also it takes a while after the merge window for you to move
branches to -rc1 or similar (whether through merge or rebase),
as is natural. Which will delay everything using those. It's just
a natural side-effect of hierarchy.

Nothing disastrous but it makes things congest. Maybe it can
be processed around, I don't exactly know the routine around
your trees and branches.

Yours,
Linus Walleij

Patch

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index efb4cf91c9e4..9a3a83211a90 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1803,7 +1803,6 @@  static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (!indio_dev->info)
 		goto out_unlock;
 
-	ret = -EINVAL;
 	list_for_each_entry(h, &iio_dev_opaque->ioctl_handlers, entry) {
 		ret = h->ioctl(indio_dev, filp, cmd, arg);
 		if (ret != IIO_IOCTL_UNHANDLED)
@@ -1811,7 +1810,7 @@  static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	}
 
 	if (ret == IIO_IOCTL_UNHANDLED)
-		ret = -EINVAL;
+		ret = -ENODEV;
 
 out_unlock:
 	mutex_unlock(&iio_dev_opaque->info_exist_lock);