device property: Allow error pointer to be passed to fwnode APIs

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]>

Reviewed-by Sakari Ailus <[email protected]>

Acked-by Nuno Sá <[email protected]>

---
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(-)

drivers/base/property.c +42 -33

index c0e94cce9c29..4fbdb14cdec4 100644
@@ -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);
 

include/linux/fwnode.h +5 -5

index 3a532ba66f6c..7defac04f9a3 100644
@@ -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) ?			\

Signature · 1 line

2.34.1

Quoted 14 lines · Andy Shevchenko

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.

Quoted 8 lines

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]>

Reviewed-by Sakari Ailus <[email protected]>

Acked-by Nuno Sá <[email protected]>

This is nice! FWIW:

Reviewed-by Heikki Krogerus <[email protected]>

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]>

Reviewed-by Sakari Ailus <[email protected]>

Acked-by Nuno Sá <[email protected]>

Hi Andy,

Quoted 14 lines

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.

Quoted 8 lines

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]>

Reviewed-by Sakari Ailus <[email protected]>

Acked-by Nuno Sá <[email protected]>

This breaks SFP/phylink (using the lan966x switch) on my board. See below for more details.

[..]

drivers/base/property.c int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, · line 493

Quoted 10 lines

drivers/base/property.c +4 -0

@@ -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))" ?

drivers/base/property.c int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, · line 499

Quoted 12 lines

drivers/base/property.c +3 -6

+             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

On Mon, Mar 14, 2022 at 8:51 PM Michael Walle <[email protected]> wrote:

Quoted 2 lines

This breaks SFP/phylink (using the lan966x switch) on my board. See below for more details.

I'm dropping this commit for the time being.

This breaks SFP/phylink (using the lan966x switch) on my board. See below for more details.