Skip to contentSkip to conversation

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

PATCH Superseded Andy Shevchenko 4 participants 13 messages Apr 6, 2022 – Apr 8, 2022

Subsystems (3)
ACPI Driver core Software nodes and device properties

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Andy Shevchenko

author reviewer
#0 lore raw

PATCH v5 1/4 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]>
Tested-by Nuno Sá <[email protected]>
Acked-by Nuno Sá <[email protected]>
Reviewed-by Sakari Ailus <[email protected]>
Reviewed-by Heikki Krogerus <[email protected]>
Signed-off-by Andy Shevchenko <[email protected]>

---
v5: fixed return value in _get_reference_args() for secondary check (Michael)

 drivers/base/property.c | 89 +++++++++++++++++++++++------------------
 include/linux/fwnode.h  | 10 ++---
 2 files changed, 56 insertions(+), 43 deletions(-)
drivers/base/property.c +51 -38
index c0e94cce9c29..3376d3971a93 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,19 @@ 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);
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return -ENOENT;
 
-	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);
 

@@ -635,12 +644,13 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents);
 struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
 					    unsigned int depth)
 {
-	unsigned int i;
-
 	fwnode_handle_get(fwnode);
 
-	for (i = 0; i < depth && fwnode; i++)
+	do {
+		if (depth-- == 0)
+			break;
 		fwnode = fwnode_get_next_parent(fwnode);
+	} while (fwnode);
 
 	return fwnode;
 }

@@ -659,17 +669,17 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
 bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
 				  struct fwnode_handle *test_child)
 {
-	if (!test_ancestor)
+	if (IS_ERR_OR_NULL(test_ancestor))
 		return false;
 
 	fwnode_handle_get(test_child);
-	while (test_child) {
+	do {
 		if (test_child == test_ancestor) {
 			fwnode_handle_put(test_child);
 			return true;
 		}
 		test_child = fwnode_get_next_parent(test_child);
-	}
+	} while (test_child);
 	return false;
 }
 

@@ -698,7 +708,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 +732,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 +808,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 +1001,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.35.1
author reviewer
#1 lore raw

PATCH v5 2/4 device property: Introduce fwnode_for_each_parent_node()

In a few cases the functionality of fwnode_for_each_parent_node() is already in use. Introduce a common helper macro for it.

It may be used by others as well in the future.

Signed-off-by Andy Shevchenko <[email protected]>

---
v5: new patch

 drivers/base/property.c  | 56 +++++++++++++++++++++-------------------
 include/linux/property.h |  9 +++++--
 2 files changed, 36 insertions(+), 29 deletions(-)
drivers/base/property.c +29 -27
index 3376d3971a93..bca9a28ce271 100644
@@ -596,17 +596,17 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
  */
 struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
 {
+	struct fwnode_handle *parent;
 	struct device *dev;
 
-	fwnode_handle_get(fwnode);
-	do {
-		fwnode = fwnode_get_next_parent(fwnode);
-		if (!fwnode)
-			return NULL;
+	fwnode_for_each_parent_node(fwnode, parent) {
 		dev = get_dev_from_fwnode(fwnode);
-	} while (!dev);
-	fwnode_handle_put(fwnode);
-	return dev;
+		if (dev) {
+			fwnode_handle_put(parent);
+			return dev;
+		}
+	}
+	return NULL;
 }
 
 /**

@@ -617,13 +617,11 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
  */
 unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode)
 {
-	struct fwnode_handle *__fwnode;
-	unsigned int count;
-
-	__fwnode = fwnode_get_parent(fwnode);
+	struct fwnode_handle *parent;
+	unsigned int count = 0;
 
-	for (count = 0; __fwnode; count++)
-		__fwnode = fwnode_get_next_parent(__fwnode);
+	fwnode_for_each_parent_node(fwnode, parent)
+		count++;
 
 	return count;
 }

@@ -644,15 +642,16 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents);
 struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
 					    unsigned int depth)
 {
-	fwnode_handle_get(fwnode);
+	struct fwnode_handle *parent;
 
-	do {
-		if (depth-- == 0)
-			break;
-		fwnode = fwnode_get_next_parent(fwnode);
-	} while (fwnode);
+	if (depth == 0)
+		return fwnode_handle_get(fwnode);
 
-	return fwnode;
+	fwnode_for_each_parent_node(fwnode, parent) {
+		if (--depth == 0)
+			return parent;
+	}
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
 

@@ -669,17 +668,20 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
 bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
 				  struct fwnode_handle *test_child)
 {
+	struct fwnode_handle *parent;
+
 	if (IS_ERR_OR_NULL(test_ancestor))
 		return false;
 
-	fwnode_handle_get(test_child);
-	do {
-		if (test_child == test_ancestor) {
-			fwnode_handle_put(test_child);
+	if (test_child == test_ancestor)
+		return true;
+
+	fwnode_for_each_parent_node(test_child, parent) {
+		if (parent == test_ancestor) {
+			fwnode_handle_put(parent);
 			return true;
 		}
-		test_child = fwnode_get_next_parent(test_child);
-	} while (test_child);
+	}
 	return false;
 }
include/linux/property.h +7 -2
index 4cd4b326941f..15d6863ae962 100644
@@ -83,9 +83,14 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 
 const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
+
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
-struct fwnode_handle *fwnode_get_next_parent(
-	struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
+
+#define fwnode_for_each_parent_node(fwnode, parent)		\
+	for (parent = fwnode_get_parent(fwnode); parent;	\
+	     parent = fwnode_get_next_parent(parent))
+
 struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
 unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
 struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
Signature · 1 line
2.35.1
reviewer Reviewed-by on v5 2/4
#2 lore raw

Re: [PATCH v5 2/4] device property: Introduce fwnode_for_each_parent_node()

Hi Andy,

Quoted 6 lines · Andy Shevchenko

In a few cases the functionality of fwnode_for_each_parent_node() is already in use. Introduce a common helper macro for it.

It may be used by others as well in the future.

Signed-off-by Andy Shevchenko <[email protected]>

Until patch 4:

Reviewed-by Sakari Ailus <[email protected]>
Signature · 1 line
Sakari Ailus
author reviewer
#3 lore raw

PATCH v5 3/4 device property: Drop 'test' prefix in parameters of fwnode_is_ancestor_of()

In reply to Andy Shevchenko

The part 'is' in the function name implies the test against something. Drop unnecessary 'test' prefix in the fwnode_is_ancestor_of() parameters.

No functional change intended.

Signed-off-by Andy Shevchenko <[email protected]>

---
v5: new patch

 drivers/base/property.c  | 20 +++++++++-----------
 include/linux/property.h |  3 +--
 2 files changed, 10 insertions(+), 13 deletions(-)
drivers/base/property.c +9 -11
index bca9a28ce271..839ee7f3ee12 100644
@@ -656,28 +656,26 @@ struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
 EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
 
 /**
- * fwnode_is_ancestor_of - Test if @test_ancestor is ancestor of @test_child
- * @test_ancestor: Firmware which is tested for being an ancestor
- * @test_child: Firmware which is tested for being the child
+ * fwnode_is_ancestor_of - Test if @ancestor is ancestor of @child
+ * @ancestor: Firmware which is tested for being an ancestor
+ * @child: Firmware which is tested for being the child
  *
  * A node is considered an ancestor of itself too.
  *
- * Returns true if @test_ancestor is an ancestor of @test_child.
- * Otherwise, returns false.
+ * Returns true if @ancestor is an ancestor of @child. Otherwise, returns false.
  */
-bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
-				  struct fwnode_handle *test_child)
+bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child)
 {
 	struct fwnode_handle *parent;
 
-	if (IS_ERR_OR_NULL(test_ancestor))
+	if (IS_ERR_OR_NULL(ancestor))
 		return false;
 
-	if (test_child == test_ancestor)
+	if (child == ancestor)
 		return true;
 
-	fwnode_for_each_parent_node(test_child, parent) {
-		if (parent == test_ancestor) {
+	fwnode_for_each_parent_node(child, parent) {
+		if (parent == ancestor) {
 			fwnode_handle_put(parent);
 			return true;
 		}
include/linux/property.h +1 -2
index 15d6863ae962..fc24d45632eb 100644
@@ -95,8 +95,7 @@ struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
 unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
 struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
 					    unsigned int depth);
-bool fwnode_is_ancestor_of(struct fwnode_handle *test_ancestor,
-				  struct fwnode_handle *test_child);
+bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_child_node(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_available_child_node(
Signature · 1 line
2.35.1
author reviewer
#4 lore raw

PATCH v5 4/4 device property: Constify fwnode APIs that uses fwnode_get_next_parent()

In reply to Andy Shevchenko

Due to fwnode_get_next_parent() used to be called against the parameters of some of fwnode APIs those parameters have no const qualifier. However, after switching to fwnode_for_each_parent_node() API now it's possible to constify the parameters. Do it for good.

The affected functions are:

fwnode_get_next_parent_dev()
fwnode_get_nth_parent()
fwnode_is_ancestor_of()

Signed-off-by Andy Shevchenko <[email protected]>

---
v5: new patch

 drivers/base/property.c  | 7 +++----
 include/linux/property.h | 9 ++++-----
 2 files changed, 7 insertions(+), 9 deletions(-)
drivers/base/property.c +3 -4
index 839ee7f3ee12..79e0adfa1e24 100644
@@ -594,7 +594,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
  * The caller of this function is expected to call put_device() on the returned
  * device when they are done.
  */
-struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
+struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode)
 {
 	struct fwnode_handle *parent;
 	struct device *dev;

@@ -639,8 +639,7 @@ EXPORT_SYMBOL_GPL(fwnode_count_parents);
  * The caller is responsible for calling fwnode_handle_put() for the returned
  * node.
  */
-struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
-					    unsigned int depth)
+struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
 {
 	struct fwnode_handle *parent;
 

@@ -664,7 +663,7 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
  *
  * Returns true if @ancestor is an ancestor of @child. Otherwise, returns false.
  */
-bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child)
+bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child)
 {
 	struct fwnode_handle *parent;
include/linux/property.h +4 -5
index fc24d45632eb..119bf7d6a02f 100644
@@ -91,11 +91,10 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode);
 	for (parent = fwnode_get_parent(fwnode); parent;	\
 	     parent = fwnode_get_next_parent(parent))
 
-struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode);
-unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
-struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
-					    unsigned int depth);
-bool fwnode_is_ancestor_of(struct fwnode_handle *ancestor, struct fwnode_handle *child);
+struct device *fwnode_get_next_parent_dev(const struct fwnode_handle *fwnode);
+unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth);
+bool fwnode_is_ancestor_of(const struct fwnode_handle *ancestor, const struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_child_node(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_available_child_node(
Signature · 1 line
2.35.1
build bot
#5 lore raw

Re: [PATCH v5 4/4] device property: Constify fwnode APIs that uses fwnode_get_next_parent()

I love your patch! Yet something to improve:

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on rafael-pm/linux-next linus/master linux/master v5.18-rc1 next-20220406] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: hexagon-randconfig-r032-20220406 (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c4a1b07d0979e7ff20d7d541af666d822d66b566)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511 git checkout d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/base/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/base/property.c:647:28: error: passing 'const struct fwnode_handle *' to parameter of type 'struct fwnode_handle *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
                   return fwnode_handle_get(fwnode);
                                            ^~~~~~
   include/linux/property.h:123:63: note: passing argument to parameter 'fwnode' here
   struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
                                                                 ^

1 error generated.

vim +647 drivers/base/property.c

87e5e95db31a27d Sakari Ailus    2019-10-03  629  
87e5e95db31a27d Sakari Ailus    2019-10-03  630  /**
87e5e95db31a27d Sakari Ailus    2019-10-03  631   * fwnode_get_nth_parent - Return an nth parent of a node
87e5e95db31a27d Sakari Ailus    2019-10-03  632   * @fwnode: The node the parent of which is requested
87e5e95db31a27d Sakari Ailus    2019-10-03  633   * @depth: Distance of the parent from the node
87e5e95db31a27d Sakari Ailus    2019-10-03  634   *
87e5e95db31a27d Sakari Ailus    2019-10-03  635   * Returns the nth parent of a node. If there is no parent at the requested
87e5e95db31a27d Sakari Ailus    2019-10-03  636   * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
87e5e95db31a27d Sakari Ailus    2019-10-03  637   * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
87e5e95db31a27d Sakari Ailus    2019-10-03  638   *
87e5e95db31a27d Sakari Ailus    2019-10-03  639   * The caller is responsible for calling fwnode_handle_put() for the returned
87e5e95db31a27d Sakari Ailus    2019-10-03  640   * node.
87e5e95db31a27d Sakari Ailus    2019-10-03  641   */
d9d353ada8d8c3b Andy Shevchenko 2022-04-06  642  struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
87e5e95db31a27d Sakari Ailus    2019-10-03  643  {
040f806ecab6cd6 Andy Shevchenko 2022-04-06  644  	struct fwnode_handle *parent;
87e5e95db31a27d Sakari Ailus    2019-10-03  645  
040f806ecab6cd6 Andy Shevchenko 2022-04-06  646  	if (depth == 0)
040f806ecab6cd6 Andy Shevchenko 2022-04-06 @647  		return fwnode_handle_get(fwnode);
87e5e95db31a27d Sakari Ailus    2019-10-03  648  
040f806ecab6cd6 Andy Shevchenko 2022-04-06  649  	fwnode_for_each_parent_node(fwnode, parent) {
040f806ecab6cd6 Andy Shevchenko 2022-04-06  650  		if (--depth == 0)
040f806ecab6cd6 Andy Shevchenko 2022-04-06  651  			return parent;
040f806ecab6cd6 Andy Shevchenko 2022-04-06  652  	}
040f806ecab6cd6 Andy Shevchenko 2022-04-06  653  	return NULL;
87e5e95db31a27d Sakari Ailus    2019-10-03  654  }
87e5e95db31a27d Sakari Ailus    2019-10-03  655  EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
87e5e95db31a27d Sakari Ailus    2019-10-03  656
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
build bot
#6 lore raw

Re: [PATCH v5 4/4] device property: Constify fwnode APIs that uses fwnode_get_next_parent()

In reply to Andy Shevchenko
I love your patch! Perhaps something to improve:

Hi Andy,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on rafael-pm/linux-next linus/master linux/master v5.18-rc1 next-20220406] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: powerpc-linux-gcc (GCC) 11.2.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511 git checkout d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/base/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/base/property.c: In function 'fwnode_get_nth_parent':

>> drivers/base/property.c:647:42: warning: passing argument 1 of 'fwnode_handle_get' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     647 |                 return fwnode_handle_get(fwnode);
         |                                          ^~~~~~
   In file included from include/linux/of.h:22,
                    from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from drivers/base/property.c:10:
   include/linux/property.h:123:63: note: expected 'struct fwnode_handle *' but argument is of type 'const struct fwnode_handle *'
     123 | struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
         |                                         ~~~~~~~~~~~~~~~~~~~~~~^~~~~~

vim +647 drivers/base/property.c

87e5e95db31a27 Sakari Ailus    2019-10-03  629  
87e5e95db31a27 Sakari Ailus    2019-10-03  630  /**
87e5e95db31a27 Sakari Ailus    2019-10-03  631   * fwnode_get_nth_parent - Return an nth parent of a node
87e5e95db31a27 Sakari Ailus    2019-10-03  632   * @fwnode: The node the parent of which is requested
87e5e95db31a27 Sakari Ailus    2019-10-03  633   * @depth: Distance of the parent from the node
87e5e95db31a27 Sakari Ailus    2019-10-03  634   *
87e5e95db31a27 Sakari Ailus    2019-10-03  635   * Returns the nth parent of a node. If there is no parent at the requested
87e5e95db31a27 Sakari Ailus    2019-10-03  636   * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
87e5e95db31a27 Sakari Ailus    2019-10-03  637   * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
87e5e95db31a27 Sakari Ailus    2019-10-03  638   *
87e5e95db31a27 Sakari Ailus    2019-10-03  639   * The caller is responsible for calling fwnode_handle_put() for the returned
87e5e95db31a27 Sakari Ailus    2019-10-03  640   * node.
87e5e95db31a27 Sakari Ailus    2019-10-03  641   */
d9d353ada8d8c3 Andy Shevchenko 2022-04-06  642  struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
87e5e95db31a27 Sakari Ailus    2019-10-03  643  {
040f806ecab6cd Andy Shevchenko 2022-04-06  644  	struct fwnode_handle *parent;
87e5e95db31a27 Sakari Ailus    2019-10-03  645  
040f806ecab6cd Andy Shevchenko 2022-04-06  646  	if (depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06 @647  		return fwnode_handle_get(fwnode);
87e5e95db31a27 Sakari Ailus    2019-10-03  648  
040f806ecab6cd Andy Shevchenko 2022-04-06  649  	fwnode_for_each_parent_node(fwnode, parent) {
040f806ecab6cd Andy Shevchenko 2022-04-06  650  		if (--depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06  651  			return parent;
040f806ecab6cd Andy Shevchenko 2022-04-06  652  	}
040f806ecab6cd Andy Shevchenko 2022-04-06  653  	return NULL;
87e5e95db31a27 Sakari Ailus    2019-10-03  654  }
87e5e95db31a27 Sakari Ailus    2019-10-03  655  EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
87e5e95db31a27 Sakari Ailus    2019-10-03  656
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
build bot
#7 lore raw

Re: [PATCH v5 4/4] device property: Constify fwnode APIs that uses fwnode_get_next_parent()

In reply to Andy Shevchenko
I love your patch! Perhaps something to improve:

Hi Andy,

I love your patch! Perhaps something to improve:

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on rafael-pm/linux-next linus/master linux/master v5.18-rc1 next-20220406] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220407/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Andy-Shevchenko/device-property-Allow-error-pointer-to-be-passed-to-fwnode-APIs/20220407-002511
        git checkout d9d353ada8d8c3b1b7f3965ad7fe191bd7dea930
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/base/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)

>> drivers/base/property.c:647:42: sparse: sparse: incorrect type in argument 1 (different modifiers) @@     expected struct fwnode_handle *fwnode @@     got struct fwnode_handle const *fwnode @@

drivers/base/property.c:647:42: sparse: expected struct fwnode_handle *fwnode
drivers/base/property.c:647:42: sparse: got struct fwnode_handle const *fwnode

vim +647 drivers/base/property.c

87e5e95db31a27 Sakari Ailus    2019-10-03  629  
87e5e95db31a27 Sakari Ailus    2019-10-03  630  /**
87e5e95db31a27 Sakari Ailus    2019-10-03  631   * fwnode_get_nth_parent - Return an nth parent of a node
87e5e95db31a27 Sakari Ailus    2019-10-03  632   * @fwnode: The node the parent of which is requested
87e5e95db31a27 Sakari Ailus    2019-10-03  633   * @depth: Distance of the parent from the node
87e5e95db31a27 Sakari Ailus    2019-10-03  634   *
87e5e95db31a27 Sakari Ailus    2019-10-03  635   * Returns the nth parent of a node. If there is no parent at the requested
87e5e95db31a27 Sakari Ailus    2019-10-03  636   * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
87e5e95db31a27 Sakari Ailus    2019-10-03  637   * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
87e5e95db31a27 Sakari Ailus    2019-10-03  638   *
87e5e95db31a27 Sakari Ailus    2019-10-03  639   * The caller is responsible for calling fwnode_handle_put() for the returned
87e5e95db31a27 Sakari Ailus    2019-10-03  640   * node.
87e5e95db31a27 Sakari Ailus    2019-10-03  641   */
d9d353ada8d8c3 Andy Shevchenko 2022-04-06  642  struct fwnode_handle *fwnode_get_nth_parent(const struct fwnode_handle *fwnode, unsigned int depth)
87e5e95db31a27 Sakari Ailus    2019-10-03  643  {
040f806ecab6cd Andy Shevchenko 2022-04-06  644  	struct fwnode_handle *parent;
87e5e95db31a27 Sakari Ailus    2019-10-03  645  
040f806ecab6cd Andy Shevchenko 2022-04-06  646  	if (depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06 @647  		return fwnode_handle_get(fwnode);
87e5e95db31a27 Sakari Ailus    2019-10-03  648  
040f806ecab6cd Andy Shevchenko 2022-04-06  649  	fwnode_for_each_parent_node(fwnode, parent) {
040f806ecab6cd Andy Shevchenko 2022-04-06  650  		if (--depth == 0)
040f806ecab6cd Andy Shevchenko 2022-04-06  651  			return parent;
040f806ecab6cd Andy Shevchenko 2022-04-06  652  	}
040f806ecab6cd Andy Shevchenko 2022-04-06  653  	return NULL;
87e5e95db31a27 Sakari Ailus    2019-10-03  654  }
87e5e95db31a27 Sakari Ailus    2019-10-03  655  EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
87e5e95db31a27 Sakari Ailus    2019-10-03  656
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
Tested-by on v5 1/4
#8 lore raw
In reply to Andy Shevchenko
Quoted 10 lines
drivers/base/property.c +3 -0
@@ -480,15 +485,19 @@ 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,

drivers/base/property.c int fwnode_property_get_reference_args(const · line 499

Quoted 10 lines
drivers/base/property.c +4 -4
 				 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);
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return -ENOENT;

Doesn't this mean you overwrite any return code != 0 with -ENOENT? Is this intended?

In any case:

Tested-by Michael Walle <[email protected]>

-michael

author reviewer
#9 lore raw

On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:

...

Quoted 8 lines
Quoted 8 lines
drivers/base/property.c +5 -0
+	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;
Quoted 9 lines

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

Quoted 6 lines
drivers/base/property.c +2 -4
-	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);
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return -ENOENT;

Doesn't this mean you overwrite any return code != 0 with -ENOENT? Is this intended?

Indeed, it would shadow the error code.
So, it should go with

	if (IS_ERR_OR_NULL(fwnode->secondary))
		return ret;

then.

Quoted 2 lines

In any case:

Tested-by Michael Walle <[email protected]>

Thanks!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author reviewer
#10 lore raw
Quoted 1 line · Andy Shevchenko

On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:

...

Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
drivers/base/property.c +5 -0
+	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;
Quoted 9 lines

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

Quoted 6 lines
drivers/base/property.c +2 -4
-	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);
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return -ENOENT;

Doesn't this mean you overwrite any return code != 0 with -ENOENT? Is this intended?

Quoted 1 line

Indeed, it would shadow the error code.

I was thinking more on this and am not sure about the best approach here. On one hand in the original code this returns the actual error code from the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.

But when we check the secondary fwnode we want to have understanding that it's secondary fwnode which has not been found, but this requires to have a good distinguishing between error codes from the callback.

That said, the error codes convention of ->get_reference_args() simply sucks. Sakari, do you have it on your TODO to fix this mess out, if it's even feasible?

To be on safest side, I will change as suggested in previous mail (see below) so it won't have impact on -EINVAL case.

Quoted 6 lines

So, it should go with

	if (IS_ERR_OR_NULL(fwnode->secondary))
		return ret;

then.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
reviewer
#11 lore raw

Hi Andy,

Quoted 29 lines · Andy Shevchenko
Quoted 1 line · Andy Shevchenko

On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:

...

Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
drivers/base/property.c +5 -0
+	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;
Quoted 9 lines

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

Quoted 6 lines
drivers/base/property.c +2 -4
-	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);
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return -ENOENT;

Doesn't this mean you overwrite any return code != 0 with -ENOENT? Is this intended?

Quoted 1 line

Indeed, it would shadow the error code.

I was thinking more on this and am not sure about the best approach here. On one hand in the original code this returns the actual error code from the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.

Quoted 7 lines

But when we check the secondary fwnode we want to have understanding that it's secondary fwnode which has not been found, but this requires to have a good distinguishing between error codes from the callback.

That said, the error codes convention of ->get_reference_args() simply sucks. Sakari, do you have it on your TODO to fix this mess out, if it's even feasible?

What would you expect to see compared to what it is now?

I guess the error code could be different for a missing property and a property with invalid data, but I'm not sure any caller would be interested in that.

Quoted 16 lines

To be on safest side, I will change as suggested in previous mail (see below) so it won't have impact on -EINVAL case.

Quoted 6 lines

So, it should go with

	if (IS_ERR_OR_NULL(fwnode->secondary))
		return ret;

then.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
Signature · 1 line
Sakari Ailus
author reviewer
#12 lore raw
Quoted 3 lines · Sakari Ailus
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Andy Shevchenko

On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:

...

Quoted 28 lines
Quoted 28 lines
Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
drivers/base/property.c +5 -0
+	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;
Quoted 9 lines

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

Quoted 6 lines
drivers/base/property.c +2 -4
-	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);
+	if (IS_ERR_OR_NULL(fwnode->secondary))
+		return -ENOENT;

Doesn't this mean you overwrite any return code != 0 with -ENOENT? Is this intended?

Quoted 1 line

Indeed, it would shadow the error code.

I was thinking more on this and am not sure about the best approach here. On one hand in the original code this returns the actual error code from the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.

But when we check the secondary fwnode we want to have understanding that it's secondary fwnode which has not been found, but this requires to have a good distinguishing between error codes from the callback.

Quoted 8 lines
Quoted 3 lines

That said, the error codes convention of ->get_reference_args() simply sucks. Sakari, do you have it on your TODO to fix this mess out, if it's even feasible?

What would you expect to see compared to what it is now?

I guess the error code could be different for a missing property and a property with invalid data,

Yes, something like this.

Quoted 2 lines

but I'm not sure any caller would be interested
in that.

Yes, but it would be good for the consistency and working with fwnodes in general. Esp. if we move at some point from primary-secondary to a full linked list of fwnodes.

Quoted 9 lines
Quoted 9 lines

To be on safest side, I will change as suggested in previous mail (see below) so it won't have impact on -EINVAL case.

Quoted 6 lines

So, it should go with

	if (IS_ERR_OR_NULL(fwnode->secondary))
		return ret;

then.

Signature · 2 lines
With Best Regards,
Andy Shevchenko