Re: [PATCH v4 1/1] device property: Allow error pointer to be passed to fwnode APIs

[Heikki Krogerus]

On Tue, Mar 08, 2022 at 02:37:12PM +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@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Tested-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Acked-by: Nuno Sá <nuno.sa@xxxxxxxxxx>

This is nice! FWIW:

Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

> ---
> v4: dropped unneeded fwnode.h and ' == true' part (Sakari), added tag (Sakari)
>  drivers/base/property.c | 75 +++++++++++++++++++++++------------------
>  include/linux/fwnode.h  | 10 +++---
>  2 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c0e94cce9c29..4fbdb14cdec4 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -47,12 +47,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)
> +		return ret;
> +
> +	return fwnode_call_bool_op(fwnode->secondary, property_present, propname);
>  }
>  EXPORT_SYMBOL_GPL(fwnode_property_present);
>  
> @@ -232,15 +234,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 +374,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 +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)
> +		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 +704,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 +728,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 +804,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 +997,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) ?			\
> -- 
> 2.34.1

-- 
heikki