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

PATCH Superseded Andy Shevchenko 3 participants 6 messages Mar 7, 2022 – Mar 8, 2022

Subsystems (3)

ACPI Driver core Software nodes and device properties

lore

Share & tools

lore

Andy Shevchenko

author reviewer

2022-03-07 20:29 UTC #0 lore raw

PATCH v3 1/1 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]>

---

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

drivers/base/property.c +43 ~~-33~~

index c0e94cce9c29..635a0e556a4f 100644
@@ -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);

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

Sa, Nuno

2022-03-08 09:25 UTC #1 lore raw

Quoted 46 lines

-----Original Message-----
From: Andy Shevchenko <[email protected]>
Sent: Monday, March 7, 2022 9:30 PM
To: Andy Shevchenko <[email protected]>; Rafael J. Wysocki <[email protected]>; [email protected]; [email protected]
Cc: Daniel Scally <[email protected]>; Heikki Krogerus <[email protected]>; Sakari Ailus <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rafael J. Wysocki <[email protected]>; Len Brown <[email protected]>; Sa, Nuno <[email protected]>
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á <[email protected]>
Signed-off-by Andy Shevchenko
<[email protected]>
Tested-by Nuno Sá <[email protected]>

Quoted 8 lines

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

Quoted 174 lines

 drivers/base/property.c | 76 +++++++++++++++++++++++-------------
-----
 include/linux/fwnode.h  | 10 +++---
 2 files changed, 48 insertions(+), 38 deletions(-)

drivers/base/property.c +8 -5

index c0e94cce9c29..635a0e556a4f 100644
@@ -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);

drivers/base/property.c +0 -0

@@ -232,15 +235,16 @@ static int

fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
{

drivers/base/property.c +3 -0

 	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -EINVAL;
+
 	ret = fwnode_call_int_op(fwnode, property_read_int_array,

propname,

drivers/base/property.c +0 -4

 				 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,

drivers/base/property.c +3 -2

-			elem_size, val, nval);
+	if (ret != -EINVAL)
+		return ret;
 
-	return ret;
+	return fwnode_call_int_op(fwnode->secondary,

property_read_int_array, propname,

drivers/base/property.c +1 -0

+				  elem_size, val, nval);
 }
 
 /**

@@ -371,14 +375,16 @@ int

fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
{

drivers/base/property.c +3 -0

 	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -EINVAL;
+
 	ret = fwnode_call_int_op(fwnode,

property_read_string_array, propname,

drivers/base/property.c +0 -4

 				 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,

drivers/base/property.c +4 -2

-					 val, nval);
-	return ret;
+	if (ret != -EINVAL)
+		return ret;
+
+	return fwnode_call_int_op(fwnode->secondary,

property_read_string_array, propname,

drivers/base/property.c +1 -0

+				  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,
{

drivers/base/property.c +3 -0

 	int ret;
 
+	if (IS_ERR_OR_NULL(fwnode))
+		return -ENOENT;
+
 	ret = fwnode_call_int_op(fwnode, get_reference_args, prop,

nargs_prop,

drivers/base/property.c +2 -3

 				 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,

drivers/base/property.c +0 -1

-					 prop, nargs_prop, nargs, index,

args);

drivers/base/property.c +1 -2

-
-	return ret;
+	return fwnode_call_int_op(fwnode->secondary,

get_reference_args, prop, nargs_prop,

drivers/base/property.c +2 -1

+				  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;

drivers/base/property.c +4 -4

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

drivers/base/property.c +3 -0

@@ -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,

drivers/base/property.c fwnode_graph_get_next_endpoint(const · line 1008

Quoted 10 lines

drivers/base/property.c +4 -0

 		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á

Andy Shevchenko

author reviewer

2022-03-08 10:11 UTC #2 lore raw

Quoted 3 lines · Nuno

Quoted 3 lines
-----Original Message-----
From: Andy Shevchenko <[email protected]>
Sent: Monday, March 7, 2022 9:30 PM

...

Quoted 8 lines

Quoted 6 lines
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!

...

Quoted 7 lines

drivers/base/property.c fwnode_graph_get_next_endpoint(const · line 1003

Quoted 7 lines

drivers/base/property.c +2 -0

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

Quoted 10 lines

drivers/base/property.c fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode, · line 1006
Quoted 4 lines
drivers/base/property.c +2 -0
 	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).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Sa, Nuno

Acked-by

2022-03-08 10:32 UTC #3 lore raw

Quoted 57 lines

From: Andy Shevchenko <[email protected]>
Sent: Tuesday, March 8, 2022 11:10 AM
To: Sa, Nuno <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>; linux- [email protected]; [email protected]; Daniel Scally <[email protected]>; Heikki Krogerus <[email protected]>; Sakari Ailus <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rafael J. Wysocki <[email protected]>; Len Brown <[email protected]>
Subject: Re: [PATCH v3 1/1] device property: Allow error pointer to be passed to fwnode APIs
[External]
Quoted 3 lines · Nuno
Quoted 3 lines
-----Original Message-----
From: Andy Shevchenko <[email protected]>
Sent: Monday, March 7, 2022 9:30 PM
...
Quoted 8 lines
Quoted 6 lines
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!
...
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
@@ -988,14 +998,14 @@
fwnode_graph_get_next_endpoint(const
Quoted 6 lines
drivers/base/property.c fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode, · line 1002
Quoted 6 lines
drivers/base/property.c +2 -0
 struct fwnode_handle *fwnode,
 		parent = fwnode_graph_get_port_parent(prev);
 	else
 		parent = fwnode;
+	if (IS_ERR_OR_NULL(parent))
+		return NULL;
(1)
Quoted 10 lines
drivers/base/property.c fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode, · line 1006
Quoted 4 lines
drivers/base/property.c +2 -0
 	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...

Quoted 5 lines

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á <[email protected]>

Quoted 4 lines

--
With Best Regards,
Andy Shevchenko

Sakari Ailus

reviewer Reviewed-by on v3 1/1

2022-03-08 11:15 UTC #4 lore raw

In reply to Andy Shevchenko

Hi Andy,

This makes secondary handling quite a big nicer, thanks! A few comments below. Apart from that,

Reviewed-by Sakari Ailus <[email protected]>

Quoted 36 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.
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]>
---
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(-)
drivers/base/property.c +0 -0
index c0e94cce9c29..635a0e556a4f 100644

drivers/base/property.c line 12

Quoted 4 lines

drivers/base/property.c +1 -0

@@ -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.

Quoted 7 lines

drivers/base/property.c +0 -0

 #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;

drivers/base/property.c bool fwnode_property_present(const struct fwnode_handle *fwnode, · line 55

Quoted 10 lines

drivers/base/property.c +4 -5

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

Quoted 157 lines

drivers/base/property.c +38 ~~-28~~

+		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);

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 · 3 lines

Kind regards,

Sakari Ailus

Andy Shevchenko

author reviewer

2022-03-08 12:23 UTC #5 lore raw

Quoted 3 lines · Sakari Ailus

Hi Andy,
This makes secondary handling quite a big nicer, thanks!

You are welcome!

Quoted 3 lines

A few comments below. Apart from that,
Reviewed-by Sakari Ailus <[email protected]>

Thanks!

...

Quoted 11 lines · Andy Shevchenko

Quoted 4 lines
#include <linux/acpi.h>
#include <linux/export.h>
+#include <linux/fwnode.h>
Is this intended? linux/property.h already includes linux/fwnode.h.
Quoted 3 lines
#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.

...

Quoted 10 lines

drivers/base/property.c bool fwnode_property_present(const struct fwnode_handle *fwnode,
Quoted 6 lines
drivers/base/property.c +3 -0
 	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.

Quoted 1 line

Quoted 1 line
(quoted diff) +1 -0
+		return ret;

Signature · 2 lines

With Best Regards,
Andy Shevchenko