| Message ID | 20220307202949.75300-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3,1/1] device property: Allow error pointer to be passed to fwnode APIs | expand |
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Monday, March 7, 2022 9:30 PM > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Rafael J. > Wysocki <rafael.j.wysocki@intel.com>; linux-acpi@vger.kernel.org; > linux-kernel@vger.kernel.org > Cc: Daniel Scally <djrscally@gmail.com>; Heikki Krogerus > <heikki.krogerus@linux.intel.com>; Sakari Ailus > <sakari.ailus@linux.intel.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; Rafael J. Wysocki > <rafael@kernel.org>; Len Brown <lenb@kernel.org>; Sa, Nuno > <Nuno.Sa@analog.com> > 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@analog.com> > Signed-off-by: Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > Tested-by: Nuno Sá <nuno.sa@analog.com> > --- > > 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 <andriy.shevchenko@linux.intel.com> > > 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).
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Tuesday, March 8, 2022 11:10 AM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>; linux- > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; Daniel Scally > <djrscally@gmail.com>; Heikki Krogerus > <heikki.krogerus@linux.intel.com>; Sakari Ailus > <sakari.ailus@linux.intel.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; Rafael J. Wysocki > <rafael@kernel.org>; Len Brown <lenb@kernel.org> > 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 <andriy.shevchenko@linux.intel.com> > > > 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á <nuno.sa@analog.com> > -- > With Best Regards, > Andy Shevchenko >
Hi Andy, This makes secondary handling quite a big nicer, thanks! A few comments below. Apart from that, Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> On Mon, Mar 07, 2022 at 10:29:49PM +0200, Andy Shevchenko wrote: > 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@analog.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Tested-by: Nuno Sá <nuno.sa@analog.com> > --- > > 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? > > 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> Is this intended? linux/property.h already includes 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) It's already bool. I'd instead use: if (ret) > + 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; > > - if (IS_ERR_OR_NULL(ep) && > - !IS_ERR_OR_NULL(parent) && !IS_ERR_OR_NULL(parent->secondary)) > - ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL); > - > - return ep; > + return fwnode_graph_get_next_endpoint(parent->secondary, NULL); > } > EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint); > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h > index 3a532ba66f6c..7defac04f9a3 100644 > --- a/include/linux/fwnode.h > +++ b/include/linux/fwnode.h > @@ -148,12 +148,12 @@ struct fwnode_operations { > int (*add_links)(struct fwnode_handle *fwnode); > }; > > -#define fwnode_has_op(fwnode, op) \ > - ((fwnode) && (fwnode)->ops && (fwnode)->ops->op) > +#define fwnode_has_op(fwnode, op) \ > + (!IS_ERR_OR_NULL(fwnode) && (fwnode)->ops && (fwnode)->ops->op) > + > #define fwnode_call_int_op(fwnode, op, ...) \ > - (fwnode ? (fwnode_has_op(fwnode, op) ? \ > - (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : -ENXIO) : \ > - -EINVAL) > + (fwnode_has_op(fwnode, op) ? \ > + (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : (IS_ERR_OR_NULL(fwnode) ? -EINVAL : -ENXIO)) > > #define fwnode_call_bool_op(fwnode, op, ...) \ > (fwnode_has_op(fwnode, op) ? \
On Tue, Mar 08, 2022 at 01:15:22PM +0200, Sakari Ailus wrote: > Hi Andy, > > This makes secondary handling quite a big nicer, thanks! You are welcome! > A few comments below. Apart from that, > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Thanks! ... > On Mon, Mar 07, 2022 at 10:29:49PM +0200, Andy Shevchenko wrote: > > #include <linux/acpi.h> > > #include <linux/export.h> > > +#include <linux/fwnode.h> > > Is this intended? linux/property.h already includes linux/fwnode.h. > > > #include <linux/kernel.h> > > #include <linux/of.h> > > #include <linux/of_address.h> Yeah, this is a bit messy in the headers. I will drop the inclusion, but in the future it would be good to reshuffle property.h, fwnode.h, and perhaps extract swnode.h. ... > > bool ret; > > > > + if (IS_ERR_OR_NULL(fwnode)) > > + return false; > > + if (ret == true) > > It's already bool. I'd instead use: > > if (ret) Right, will amend this. > > + return ret;
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; - if (IS_ERR_OR_NULL(ep) && - !IS_ERR_OR_NULL(parent) && !IS_ERR_OR_NULL(parent->secondary)) - ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL); - - return ep; + return fwnode_graph_get_next_endpoint(parent->secondary, NULL); } EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint); diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h index 3a532ba66f6c..7defac04f9a3 100644 --- a/include/linux/fwnode.h +++ b/include/linux/fwnode.h @@ -148,12 +148,12 @@ struct fwnode_operations { int (*add_links)(struct fwnode_handle *fwnode); }; -#define fwnode_has_op(fwnode, op) \ - ((fwnode) && (fwnode)->ops && (fwnode)->ops->op) +#define fwnode_has_op(fwnode, op) \ + (!IS_ERR_OR_NULL(fwnode) && (fwnode)->ops && (fwnode)->ops->op) + #define fwnode_call_int_op(fwnode, op, ...) \ - (fwnode ? (fwnode_has_op(fwnode, op) ? \ - (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : -ENXIO) : \ - -EINVAL) + (fwnode_has_op(fwnode, op) ? \ + (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : (IS_ERR_OR_NULL(fwnode) ? -EINVAL : -ENXIO)) #define fwnode_call_bool_op(fwnode, op, ...) \ (fwnode_has_op(fwnode, op) ? \
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@analog.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Tested-by: Nuno Sá <nuno.sa@analog.com> --- 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? drivers/base/property.c | 76 +++++++++++++++++++++++------------------ include/linux/fwnode.h | 10 +++--- 2 files changed, 48 insertions(+), 38 deletions(-)