Re: [PATCH v2 03/15] iio: inkern: only return error codes in iio_channel_get_*() APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 11, 2022 at 2:38 PM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
>
> APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all() were
> returning a mix of NULL and 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 9f63cc0921ec
> ("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.
>
> This change also makes some functional changes on how errors were being
> handled. In the original behavior, even if we get an error like '-ENOMEM',
> we still continue with the search. We should only continue to lookup for
> the channel when it makes sense to do so. Hence, the main error handling
> in 'of_iio_channel_get_by_name()' is changed to the following logic:
>
>  * If a channel 'name' is provided and we do find it via
> 'io-channel-names', we should be able to get it. If we get any error,
> we should not proceed with the lookup. Moreover, we should return an error
> so that callers won't proceed with a system lookup.
>  * If a channel 'name' is provided and we cannot find it ('index < 0'),
> 'of_parse_phandle_with_args()' is expected to fail with '-EINVAL'. Hence,
> we should only continue if we get that error.
>  * If a channel 'name' is not provided we should only carry on with the
> search if 'of_parse_phandle_with_args()' returns '-ENOENT'.
>
> Also note that a system channel lookup is only done if the returned
> error code (from 'of_iio_channel_get_by_name()' or
> 'of_iio_channel_get_all()' is -ENODEV.

LGTM (but I might miss something, it's a bit too many conditionals),
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> ---
>  drivers/iio/inkern.c | 54 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 87fd2a0d44f2..c6f1cfe09bd3 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,13 +231,33 @@ 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) {
> -                       pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
> -                               np, name ? name : "", index);
> -                       return NULL;
> +                       return chan;
> +               if (name) {
> +                       if (index >= 0) {
> +                               pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
> +                                      np, name, index);
> +                               /*
> +                                * In this case, we found 'name' in 'io-channel-names'
> +                                * but somehow we still fail so that we should not proceed
> +                                * with any other lookup. Hence, explicitly return -EINVAL
> +                                * (maybe not the better error code) so that the caller
> +                                * won't do a system lookup.
> +                                */
> +                               return ERR_PTR(-EINVAL);
> +                       }
> +                       /* If index < 0, then of_parse_phandle_with_args() fails
> +                        * with -EINVAL which is expected. We should not proceed
> +                        * if we get any other error.
> +                        */
> +                       if (PTR_ERR(chan) != -EINVAL)
> +                               return chan;
> +               } else if (PTR_ERR(chan) != -ENOENT) {
> +                       /*
> +                        * if !name, then we should only proceed the lookup if
> +                        * of_parse_phandle_with_args() returns -ENOENT.
> +                        */
> +                       return chan;
>                 }
> -
>                 /*
>                  * No matching IIO channel found on this node.
>                  * If the parent node has a "io-channel-ranges" property,
> @@ -245,10 +265,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 ERR_PTR(-ENODEV);
>         }
>
> -       return chan;
> +       return ERR_PTR(-ENODEV);
>  }
>  EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name);
>
> @@ -267,8 +287,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 ERR_PTR(-ENODEV);
>
>         /* NULL terminated array to save passing size */
>         chans = kcalloc(nummaps + 1, sizeof(*chans), GFP_KERNEL);
> @@ -295,7 +315,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 +382,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) != -ENODEV)
>                         return channel;
>         }
>
> @@ -412,8 +432,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 +454,11 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>                 return ERR_PTR(-EINVAL);
>
>         chans = of_iio_channel_get_all(dev);
> -       if (chans)
> +       /*
> +        * We only want to carry on if the error is -ENODEV.  Anything else
> +        * should be reported up the stack.
> +        */
> +       if (!IS_ERR(chans) || PTR_ERR(chans) != -ENODEV)
>                 return chans;
>
>         name = dev_name(dev);
> --
> 2.37.0
>


-- 
With Best Regards,
Andy Shevchenko




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux