diff mbox series

[22/34] iio: inkern: only return error codes in iio_channel_get_*() APIs

Message ID 20220610084545.547700-23-nuno.sa@analog.com (mailing list archive)
State Changes Requested
Headers show
Series make iio inkern interface firmware agnostic | expand

Commit Message

Nuno Sá June 10, 2022, 8:45 a.m. UTC
APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all() were
returning a mix of NULL and error pointers being NULL the way to
"notify" that we should do a "system" lookup for channels. This make
it very confusing and prone to errors as commit dbbccf7c20bf
("iio: inkern: fix return value in devm_of_iio_channel_get_by_name()")
proves. On top of this, patterns like 'if (channel != NULL) return channel'
were being used where channel could actually be an error code which
makes the code hard to read.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/inkern.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko June 10, 2022, 3:05 p.m. UTC | #1
On Fri, Jun 10, 2022 at 10:48 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all() were
> returning a mix of NULL and error pointers being NULL the way to
> "notify" that we should do a "system" lookup for channels. This make
> it very confusing and prone to errors as commit dbbccf7c20bf
> ("iio: inkern: fix return value in devm_of_iio_channel_get_by_name()")
> proves. On top of this, patterns like 'if (channel != NULL) return channel'
> were being used where channel could actually be an error code which
> makes the code hard to read.

...

>                 np = np->parent;
>                 if (np && !of_get_property(np, "io-channel-ranges", NULL))
> -                       return NULL;
> +                       return chan;

Shouldn't it return a dedicated error code and not some arbitrary one?
It may be I missed something and chan has a correct error code in this
case...

...

> +       if (nummaps == 0)       /* return -ENODEV to search map table */

Comment is superfluous, the next line is self-explaining.

> +               return ERR_PTR(-ENODEV);

...

> -               if (channel != NULL)
> +               if (!IS_ERR(channel) || PTR_ERR(channel) == -EPROBE_DEFER)

Btw, in the GPIO library we have a macro or inliner (don't remember)
that represents such a conditional.
Perhaps make it (if it's a macro) global, or introduce an inline in IIO?

Okay, it's here:
https://elixir.bootlin.com/linux/v5.19-rc1/source/drivers/gpio/gpiolib.h#L179

It's similar, but not the same, so just play with an idea to introduce
something in this file, maybe it's worth doing this, maybe not.
Nuno Sá June 10, 2022, 7:48 p.m. UTC | #2
On Fri, 2022-06-10 at 17:05 +0200, Andy Shevchenko wrote:
> On Fri, Jun 10, 2022 at 10:48 AM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all()
> > were
> > returning a mix of NULL and error pointers being NULL the way to
> > "notify" that we should do a "system" lookup for channels. This
> > make
> > it very confusing and prone to errors as commit dbbccf7c20bf
> > ("iio: inkern: fix return value in
> > devm_of_iio_channel_get_by_name()")
> > proves. On top of this, patterns like 'if (channel != NULL) return
> > channel'
> > were being used where channel could actually be an error code which
> > makes the code hard to read.
> 
> ...
> 
> >                 np = np->parent;
> >                 if (np && !of_get_property(np, "io-channel-ranges",
> > NULL))
> > -                       return NULL;
> > +                       return chan;
> 
> Shouldn't it return a dedicated error code and not some arbitrary
> one?
> It may be I missed something and chan has a correct error code in
> this
> case...
> 

Since in this case we won't look for channels in the parent device, I'm
just honoring the code returned by 'of_iio_channel_get()'.

> ...
> 
> > +       if (nummaps == 0)       /* return -ENODEV to search map
> > table */
> 
> Comment is superfluous, the next line is self-explaining.
> 

Well, I agree. I just kept as it was on the original code. Can hapilly
remove it if no one objects against it.

> > +               return ERR_PTR(-ENODEV);
> 
> ...
> 
> > -               if (channel != NULL)
> > +               if (!IS_ERR(channel) || PTR_ERR(channel) == -
> > EPROBE_DEFER)
> 
> Btw, in the GPIO library we have a macro or inliner (don't remember)
> that represents such a conditional.
> Perhaps make it (if it's a macro) global, or introduce an inline in
> IIO?
> 
> Okay, it's here:
> https://elixir.bootlin.com/linux/v5.19-rc1/source/drivers/gpio/gpiolib.h#L179
> 
> It's similar, but not the same, so just play with an idea to
> introduce
> something in this file, maybe it's worth doing this, maybe not.
> 

I would also argue that could be something done after this series gets
applied...

- Nuno Sá
Jonathan Cameron June 11, 2022, 3:17 p.m. UTC | #3
On Fri, 10 Jun 2022 10:45:33 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all() were
> returning a mix of NULL and error pointers being NULL the way to

pointers with NULL being the way to...

> "notify" that we should do a "system" lookup for channels. This make
> it very confusing and prone to errors as commit dbbccf7c20bf
> ("iio: inkern: fix return value in devm_of_iio_channel_get_by_name()")
> proves. On top of this, patterns like 'if (channel != NULL) return channel'
> were being used where channel could actually be an error code which
> makes the code hard to read.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  drivers/iio/inkern.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 87fd2a0d44f2..31d9c122199a 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -214,7 +214,7 @@ static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
>  struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  					       const char *name)
>  {
> -	struct iio_channel *chan = NULL;
> +	struct iio_channel *chan;
>  
>  	/* Walk up the tree of devices looking for a matching iio channel */
>  	while (np) {
> @@ -231,11 +231,11 @@ struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  							 name);
>  		chan = of_iio_channel_get(np, index);
>  		if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
> -			break;

This original behaviour is 'interesting'. If we get a error like -ENOMEM
we should return it rather than carry on.  Do we have enough knowledge
of possible errors here to be more explicit on when we keep looking up
the tree?  I think we can get -ENOENT from of_parse_phandle_with_args()

That raises an interesting question on whether -ENODEV is the right response
for the previously NULL case or is -ENOENT more consistent with other
of_ functions?  No device could be thought of as being the case that needs
to defer (in hope it turns up later) whereas no entry means it will never
succeed.

> -		else if (name && index >= 0) {
> +			return chan;
> +		if (name && index >= 0) {
>  			pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
>  				np, name ? name : "", index);
> -			return NULL;
> +			return chan;
>  		}
>  
>  		/*
> @@ -245,10 +245,10 @@ struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
>  		 */
>  		np = np->parent;
>  		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> -			return NULL;
> +			return chan;
>  	}
>  
> -	return chan;
> +	return ERR_PTR(-ENODEV);
>  }
>  EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name);
>  
> @@ -267,8 +267,8 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  			break;
>  	} while (++nummaps);
>  
> -	if (nummaps == 0)	/* no error, return NULL to search map table */
> -		return NULL;
> +	if (nummaps == 0)	/* return -ENODEV to search map table */
> +		return ERR_PTR(-ENODEV);
>  
>  	/* NULL terminated array to save passing size */
>  	chans = kcalloc(nummaps + 1, sizeof(*chans), GFP_KERNEL);
> @@ -295,7 +295,7 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  
>  static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENODEV);
>  }
>  
>  #endif /* CONFIG_OF */
> @@ -362,7 +362,7 @@ struct iio_channel *iio_channel_get(struct device *dev,
>  	if (dev) {
>  		channel = of_iio_channel_get_by_name(dev->of_node,
>  						     channel_name);
> -		if (channel != NULL)
> +		if (!IS_ERR(channel) || PTR_ERR(channel) == -EPROBE_DEFER)
>  			return channel;
>  	}
>  
> @@ -412,8 +412,6 @@ struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev,
>  	channel = of_iio_channel_get_by_name(np, channel_name);
>  	if (IS_ERR(channel))
>  		return channel;
> -	if (!channel)
> -		return ERR_PTR(-ENODEV);
>  
>  	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
>  	if (ret)
> @@ -436,7 +434,7 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>  		return ERR_PTR(-EINVAL);
>  
>  	chans = of_iio_channel_get_all(dev);
> -	if (chans)
> +	if (!IS_ERR(chans) || PTR_ERR(chans) == -EPROBE_DEFER)

Hmm. We only want to carry on if the error is -ENODEV.  Anything else
should be reported up the stack.

That might be the only error left, but I think we should be explicit.

>  		return chans;
>  
>  	name = dev_name(dev);
Nuno Sá June 13, 2022, 7:06 a.m. UTC | #4
On Sat, 2022-06-11 at 16:17 +0100, Jonathan Cameron wrote:
> On Fri, 10 Jun 2022 10:45:33 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all()
> > were
> > returning a mix of NULL and error pointers being NULL the way to
> 
> pointers with NULL being the way to...
> 
> > "notify" that we should do a "system" lookup for channels. This
> > make
> > it very confusing and prone to errors as commit dbbccf7c20bf
> > ("iio: inkern: fix return value in
> > devm_of_iio_channel_get_by_name()")
> > proves. On top of this, patterns like 'if (channel != NULL) return
> > channel'
> > were being used where channel could actually be an error code which
> > makes the code hard to read.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  drivers/iio/inkern.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index 87fd2a0d44f2..31d9c122199a 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -214,7 +214,7 @@ static struct iio_channel
> > *of_iio_channel_get(struct device_node *np, int index)
> >  struct iio_channel *of_iio_channel_get_by_name(struct device_node
> > *np,
> >                                                const char *name)
> >  {
> > -       struct iio_channel *chan = NULL;
> > +       struct iio_channel *chan;
> >  
> >         /* Walk up the tree of devices looking for a matching iio
> > channel */
> >         while (np) {
> > @@ -231,11 +231,11 @@ struct iio_channel
> > *of_iio_channel_get_by_name(struct device_node *np,
> >                                                          name);
> >                 chan = of_iio_channel_get(np, index);
> >                 if (!IS_ERR(chan) || PTR_ERR(chan) == -
> > EPROBE_DEFER)
> > -                       break;
> 
> This original behaviour is 'interesting'. If we get a error like -
> ENOMEM
> we should return it rather than carry on.  Do we have enough
> knowledge
> of possible errors here to be more explicit on when we keep looking
> up
> the tree?  I think we can get -ENOENT from
> of_parse_phandle_with_args()
> 
> That raises an interesting question on whether -ENODEV is the right
> response
> for the previously NULL case or is -ENOENT more consistent with other
> of_ functions?  No device could be thought of as being the case that
> needs
> to defer (in hope it turns up later) whereas no entry means it will
> never
> succeed.

From what I could see, of_parse_phandle_with_args() either returns 
-EINVAL or -ENOENT. We also have the internal of_iio_channel_get()
which can return -ENOMEM. So I guess we should only continue looking if
we get -ENOENT?

To be clear, do you still prefer to explicitly return -ENODEV in the
previous NULL cases or should we honor the return code from 
of_parse_phandle_with_args() and just return chans (and thus ENOENT)?

- Nuno Sá
Jonathan Cameron June 18, 2022, 2:06 p.m. UTC | #5
On Mon, 13 Jun 2022 09:06:49 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2022-06-11 at 16:17 +0100, Jonathan Cameron wrote:
> > On Fri, 10 Jun 2022 10:45:33 +0200
> > Nuno Sá <nuno.sa@analog.com> wrote:
> >   
> > > APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all()
> > > were
> > > returning a mix of NULL and error pointers being NULL the way to  
> > 
> > pointers with NULL being the way to...
> >   
> > > "notify" that we should do a "system" lookup for channels. This
> > > make
> > > it very confusing and prone to errors as commit dbbccf7c20bf
> > > ("iio: inkern: fix return value in
> > > devm_of_iio_channel_get_by_name()")
> > > proves. On top of this, patterns like 'if (channel != NULL) return
> > > channel'
> > > were being used where channel could actually be an error code which
> > > makes the code hard to read.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/inkern.c | 24 +++++++++++-------------
> > >  1 file changed, 11 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index 87fd2a0d44f2..31d9c122199a 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -214,7 +214,7 @@ static struct iio_channel
> > > *of_iio_channel_get(struct device_node *np, int index)
> > >  struct iio_channel *of_iio_channel_get_by_name(struct device_node
> > > *np,
> > >                                                const char *name)
> > >  {
> > > -       struct iio_channel *chan = NULL;
> > > +       struct iio_channel *chan;
> > >  
> > >         /* Walk up the tree of devices looking for a matching iio
> > > channel */
> > >         while (np) {
> > > @@ -231,11 +231,11 @@ struct iio_channel
> > > *of_iio_channel_get_by_name(struct device_node *np,
> > >                                                          name);
> > >                 chan = of_iio_channel_get(np, index);
> > >                 if (!IS_ERR(chan) || PTR_ERR(chan) == -
> > > EPROBE_DEFER)
> > > -                       break;  
> > 
> > This original behaviour is 'interesting'. If we get a error like -
> > ENOMEM
> > we should return it rather than carry on.  Do we have enough
> > knowledge
> > of possible errors here to be more explicit on when we keep looking
> > up
> > the tree?  I think we can get -ENOENT from
> > of_parse_phandle_with_args()
> > 
> > That raises an interesting question on whether -ENODEV is the right
> > response
> > for the previously NULL case or is -ENOENT more consistent with other
> > of_ functions?  No device could be thought of as being the case that
> > needs
> > to defer (in hope it turns up later) whereas no entry means it will
> > never
> > succeed.  
> 
> From what I could see, of_parse_phandle_with_args() either returns 
> -EINVAL or -ENOENT. We also have the internal of_iio_channel_get()
> which can return -ENOMEM. So I guess we should only continue looking if
> we get -ENOENT?
> 
> To be clear, do you still prefer to explicitly return -ENODEV in the
> previous NULL cases or should we honor the return code from 
> of_parse_phandle_with_args() and just return chans (and thus ENOENT)?
You've looked at this more than me, so whilst I think -ENOENT is probably
slightly more consistent I'll go with whatever you conclude is the
best option.  Maybe add a small amount of description on what you chose
and why to the relevant patch descriptions.

Thanks,

Jonathan


> 
> - Nuno Sá
diff mbox series

Patch

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 87fd2a0d44f2..31d9c122199a 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -214,7 +214,7 @@  static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
 struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
 					       const char *name)
 {
-	struct iio_channel *chan = NULL;
+	struct iio_channel *chan;
 
 	/* Walk up the tree of devices looking for a matching iio channel */
 	while (np) {
@@ -231,11 +231,11 @@  struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
 							 name);
 		chan = of_iio_channel_get(np, index);
 		if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
-			break;
-		else if (name && index >= 0) {
+			return chan;
+		if (name && index >= 0) {
 			pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
 				np, name ? name : "", index);
-			return NULL;
+			return chan;
 		}
 
 		/*
@@ -245,10 +245,10 @@  struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
 		 */
 		np = np->parent;
 		if (np && !of_get_property(np, "io-channel-ranges", NULL))
-			return NULL;
+			return chan;
 	}
 
-	return chan;
+	return ERR_PTR(-ENODEV);
 }
 EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name);
 
@@ -267,8 +267,8 @@  static struct iio_channel *of_iio_channel_get_all(struct device *dev)
 			break;
 	} while (++nummaps);
 
-	if (nummaps == 0)	/* no error, return NULL to search map table */
-		return NULL;
+	if (nummaps == 0)	/* return -ENODEV to search map table */
+		return ERR_PTR(-ENODEV);
 
 	/* NULL terminated array to save passing size */
 	chans = kcalloc(nummaps + 1, sizeof(*chans), GFP_KERNEL);
@@ -295,7 +295,7 @@  static struct iio_channel *of_iio_channel_get_all(struct device *dev)
 
 static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
 #endif /* CONFIG_OF */
@@ -362,7 +362,7 @@  struct iio_channel *iio_channel_get(struct device *dev,
 	if (dev) {
 		channel = of_iio_channel_get_by_name(dev->of_node,
 						     channel_name);
-		if (channel != NULL)
+		if (!IS_ERR(channel) || PTR_ERR(channel) == -EPROBE_DEFER)
 			return channel;
 	}
 
@@ -412,8 +412,6 @@  struct iio_channel *devm_of_iio_channel_get_by_name(struct device *dev,
 	channel = of_iio_channel_get_by_name(np, channel_name);
 	if (IS_ERR(channel))
 		return channel;
-	if (!channel)
-		return ERR_PTR(-ENODEV);
 
 	ret = devm_add_action_or_reset(dev, devm_iio_channel_free, channel);
 	if (ret)
@@ -436,7 +434,7 @@  struct iio_channel *iio_channel_get_all(struct device *dev)
 		return ERR_PTR(-EINVAL);
 
 	chans = of_iio_channel_get_all(dev);
-	if (chans)
+	if (!IS_ERR(chans) || PTR_ERR(chans) == -EPROBE_DEFER)
 		return chans;
 
 	name = dev_name(dev);