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

[Jonathan Cameron <jic23@kernel.org>]

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