> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > Sent: Monday, March 7, 2022 9:30 PM > To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Rafael J. > Wysocki <rafael.j.wysocki@xxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx > Cc: Daniel Scally <djrscally@xxxxxxxxx>; Heikki Krogerus > <heikki.krogerus@xxxxxxxxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman > <gregkh@xxxxxxxxxxxxxxxxxxx>; Rafael J. Wysocki > <rafael@xxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; Sa, Nuno > <Nuno.Sa@xxxxxxxxxx> > 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á <nuno.sa@xxxxxxxxxx> > Signed-off-by: Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> > Tested-by: Nuno Sá <nuno.sa@xxxxxxxxxx> > --- > > 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á