Messages in this thread | | | From | Michael Walle <> | Subject | Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs | Date | Mon, 14 Mar 2022 20:51:37 +0100 |
| |
Hi Andy,
> 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> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Nuno Sá <nuno.sa@analog.com>
This breaks SFP/phylink (using the lan966x switch) on my board. See below for more details.
[..]
> @@ -480,15 +485,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)
Should this be "if (ret == 0 || IS_ERR_OR_NULL(fwnode->secondary))" ?
> + 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);
What happens before this patch is that sfp_bus_find_fwnode() will call fwnode_property_get_reference_args() and the first calls return -ENOENT which sfp_bus_find_fwnode() will handle in a special way. After your patch, -EINVAL is returned, because fwnode_call_int_op() on fwnode->secondary is always called regardless of the return value of the original fwnode.
-michael
| |