| Message ID | 20220610084545.547700-23-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | make iio inkern interface firmware agnostic | expand |
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.
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á
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);
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á
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 --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);
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(-)