> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday, March 7, 2022 9:30 PM
> To: Andy Shevchenko <[email protected]>; Rafael J.
> Wysocki <[email protected]>; [email protected];
> [email protected]
> Cc: Daniel Scally <[email protected]>; Heikki Krogerus
> <[email protected]>; Sakari Ailus
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Rafael J. Wysocki
> <[email protected]>; Len Brown <[email protected]>; Sa, Nuno
> <[email protected]>
> Subject: [PATCH v3 1/1] device property: Allow error pointer to be
> passed to fwnode APIs
>
> [External]
>
> Some of the fwnode APIs might return an error pointer instead of
> NULL
> or valid fwnode handle. The result of such API call may be considered
> optional and hence the test for it is usually done in a form of
>
> fwnode = fwnode_find_reference(...);
> if (IS_ERR(fwnode))
> ...error handling...
>
> Nevertheless the resulting fwnode may have bumped the reference
> count
> and hence caller of the above API is obliged to call
> fwnode_handle_put().
> Since fwnode may be not valid either as NULL or error pointer the
> check
> has to be performed there. This approach uglifies the code and adds
> a point of making a mistake, i.e. forgetting about error point case.
>
> To prevent this, allow an error pointer to be passed to the fwnode
> APIs.
>
> Fixes: 83b34afb6b79 ("device property: Introduce
> fwnode_find_reference()")
> Reported-by: Nuno Sá <[email protected]>
> Signed-off-by: Andy Shevchenko
> <[email protected]>
> Tested-by: Nuno Sá <[email protected]>
> ---
>
> v3: dropped test of secondary fwnode (Nuno), added tag (Nuno),
> amended commit message
> v2: adjusted the entire fwnode API (Sakari)
>
> Nuno, can you re-test this with the ltc2983 series to be sure it is still
> okay?
Still works!
> drivers/base/property.c | 76 +++++++++++++++++++++++-------------
> -----
> include/linux/fwnode.h | 10 +++---
> 2 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c0e94cce9c29..635a0e556a4f 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -9,6 +9,7 @@
>
> #include <linux/acpi.h>
> #include <linux/export.h>
> +#include <linux/fwnode.h>
> #include <linux/kernel.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> @@ -47,12 +48,14 @@ bool fwnode_property_present(const struct
> fwnode_handle *fwnode,
> {
> bool ret;
>
> + if (IS_ERR_OR_NULL(fwnode))
> + return false;
> +
> ret = fwnode_call_bool_op(fwnode, property_present,
> propname);
> - if (ret == false && !IS_ERR_OR_NULL(fwnode) &&
> - !IS_ERR_OR_NULL(fwnode->secondary))
> - ret = fwnode_call_bool_op(fwnode->secondary,
> property_present,
> - propname);
> - return ret;
> + if (ret == true)
> + return ret;
> +
> + return fwnode_call_bool_op(fwnode->secondary,
> property_present, propname);
> }
> EXPORT_SYMBOL_GPL(fwnode_property_present);
>
> @@ -232,15 +235,16 @@ static int
> fwnode_property_read_int_array(const struct fwnode_handle
> *fwnode,
> {
> int ret;
>
> + if (IS_ERR_OR_NULL(fwnode))
> + return -EINVAL;
> +
> ret = fwnode_call_int_op(fwnode, property_read_int_array,
> propname,
> elem_size, val, nval);
> - if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> - !IS_ERR_OR_NULL(fwnode->secondary))
> - ret = fwnode_call_int_op(
> - fwnode->secondary, property_read_int_array,
> propname,
> - elem_size, val, nval);
> + if (ret != -EINVAL)
> + return ret;
>
> - return ret;
> + return fwnode_call_int_op(fwnode->secondary,
> property_read_int_array, propname,
> + elem_size, val, nval);
> }
>
> /**
> @@ -371,14 +375,16 @@ int
> fwnode_property_read_string_array(const struct fwnode_handle
> *fwnode,
> {
> int ret;
>
> + if (IS_ERR_OR_NULL(fwnode))
> + return -EINVAL;
> +
> ret = fwnode_call_int_op(fwnode,
> property_read_string_array, propname,
> val, nval);
> - if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) &&
> - !IS_ERR_OR_NULL(fwnode->secondary))
> - ret = fwnode_call_int_op(fwnode->secondary,
> - property_read_string_array,
> propname,
> - val, nval);
> - return ret;
> + if (ret != -EINVAL)
> + return ret;
> +
> + return fwnode_call_int_op(fwnode->secondary,
> property_read_string_array, propname,
> + val, nval);
> }
> EXPORT_SYMBOL_GPL(fwnode_property_read_string_array);
>
> @@ -480,15 +486,16 @@ int
> fwnode_property_get_reference_args(const struct fwnode_handle
> *fwnode,
> {
> int ret;
>
> + if (IS_ERR_OR_NULL(fwnode))
> + return -ENOENT;
> +
> ret = fwnode_call_int_op(fwnode, get_reference_args, prop,
> nargs_prop,
> nargs, index, args);
> + if (ret == 0)
> + return ret;
>
> - if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> - !IS_ERR_OR_NULL(fwnode->secondary))
> - ret = fwnode_call_int_op(fwnode->secondary,
> get_reference_args,
> - prop, nargs_prop, nargs, index,
> args);
> -
> - return ret;
> + return fwnode_call_int_op(fwnode->secondary,
> get_reference_args, prop, nargs_prop,
> + nargs, index, args);
> }
> EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);
>
> @@ -698,7 +705,7 @@ fwnode_get_next_available_child_node(const
> struct fwnode_handle *fwnode,
> {
> struct fwnode_handle *next_child = child;
>
> - if (!fwnode)
> + if (IS_ERR_OR_NULL(fwnode))
> return NULL;
>
> do {
> @@ -722,16 +729,16 @@ struct fwnode_handle
> *device_get_next_child_node(struct device *dev,
> const struct fwnode_handle *fwnode = dev_fwnode(dev);
> struct fwnode_handle *next;
>
> + if (IS_ERR_OR_NULL(fwnode))
> + return NULL;
> +
> /* Try to find a child in primary fwnode */
> next = fwnode_get_next_child_node(fwnode, child);
> if (next)
> return next;
>
> /* When no more children in primary, continue with secondary
> */
> - if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary))
> - next = fwnode_get_next_child_node(fwnode-
> >secondary, child);
> -
> - return next;
> + return fwnode_get_next_child_node(fwnode->secondary,
> child);
> }
> EXPORT_SYMBOL_GPL(device_get_next_child_node);
>
> @@ -798,6 +805,9 @@ EXPORT_SYMBOL_GPL(fwnode_handle_put);
> */
> bool fwnode_device_is_available(const struct fwnode_handle
> *fwnode)
> {
> + if (IS_ERR_OR_NULL(fwnode))
> + return false;
> +
> if (!fwnode_has_op(fwnode, device_is_available))
> return true;
>
> @@ -988,14 +998,14 @@ fwnode_graph_get_next_endpoint(const
> struct fwnode_handle *fwnode,
> parent = fwnode_graph_get_port_parent(prev);
> else
> parent = fwnode;
> + if (IS_ERR_OR_NULL(parent))
> + return NULL;
>
> ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint,
> prev);
> + if (ep)
> + return ep;
I might be missing something but before the check being done was
'if (IS_ERR_OR_NULL(ep)'. Is there anyway for ep to be an error
pointer? Looking at OF, It seems that only NULL or a valid pointer
is being returned. Did not looked at others implementations of
though...
- Nuno Sá
On Tue, Mar 08, 2022 at 09:25:07AM +0000, Sa, Nuno wrote:
> > -----Original Message-----
> > From: Andy Shevchenko <[email protected]>
> > Sent: Monday, March 7, 2022 9:30 PM
...
> > v3: dropped test of secondary fwnode (Nuno), added tag (Nuno),
> > amended commit message
> > v2: adjusted the entire fwnode API (Sakari)
> >
> > Nuno, can you re-test this with the ltc2983 series to be sure it is still
> > okay?
>
> Still works!
Thanks for confirming!
...
> > @@ -988,14 +998,14 @@ fwnode_graph_get_next_endpoint(const
> > struct fwnode_handle *fwnode,
> > parent = fwnode_graph_get_port_parent(prev);
> > else
> > parent = fwnode;
> > + if (IS_ERR_OR_NULL(parent))
> > + return NULL;
(1)
> > ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint,
> > prev);
> > + if (ep)
> > + return ep;
>
> I might be missing something but before the check being done was
> 'if (IS_ERR_OR_NULL(ep)'. Is there anyway for ep to be an error
> pointer? Looking at OF, It seems that only NULL or a valid pointer
> is being returned. Did not looked at others implementations of
> though...
Yes, the IS_ERR() part is redundant there. I was quite confused with
that code while working on this change. So, now it looks much clearer
what's going on and what kind of values are being expected. This also
justifies the choice of returned value in (1).
--
With Best Regards,
Andy Shevchenko
> From: Andy Shevchenko <[email protected]>
> Sent: Tuesday, March 8, 2022 11:10 AM
> To: Sa, Nuno <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>; linux-
> [email protected]; [email protected]; Daniel Scally
> <[email protected]>; Heikki Krogerus
> <[email protected]>; Sakari Ailus
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Rafael J. Wysocki
> <[email protected]>; Len Brown <[email protected]>
> Subject: Re: [PATCH v3 1/1] device property: Allow error pointer to be
> passed to fwnode APIs
>
> [External]
>
> On Tue, Mar 08, 2022 at 09:25:07AM +0000, Sa, Nuno wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko <[email protected]>
> > > Sent: Monday, March 7, 2022 9:30 PM
>
> ...
>
> > > v3: dropped test of secondary fwnode (Nuno), added tag (Nuno),
> > > amended commit message
> > > v2: adjusted the entire fwnode API (Sakari)
> > >
> > > Nuno, can you re-test this with the ltc2983 series to be sure it is still
> > > okay?
> >
> > Still works!
>
> Thanks for confirming!
>
> ...
>
> > > @@ -988,14 +998,14 @@
> fwnode_graph_get_next_endpoint(const
> > > struct fwnode_handle *fwnode,
> > > parent = fwnode_graph_get_port_parent(prev);
> > > else
> > > parent = fwnode;
> > > + if (IS_ERR_OR_NULL(parent))
> > > + return NULL;
>
> (1)
>
> > > ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint,
> > > prev);
> > > + if (ep)
> > > + return ep;
> >
> > I might be missing something but before the check being done was
> > 'if (IS_ERR_OR_NULL(ep)'. Is there anyway for ep to be an error
> > pointer? Looking at OF, It seems that only NULL or a valid pointer
> > is being returned. Did not looked at others implementations of
> > though...
>
> Yes, the IS_ERR() part is redundant there. I was quite confused with
> that code while working on this change. So, now it looks much clearer
> what's going on and what kind of values are being expected. This also
> justifies the choice of returned value in (1).
>
Makes sense to me.
Acked-by: Nuno S? <[email protected]>
> --
> With Best Regards,
> Andy Shevchenko
>