Skip to contentSkip to conversation

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

PATCH In review Andy Shevchenko 7 participants 24 messages Apr 8, 2022 – Apr 29, 2022

Subsystems (4)
ACPI Driver core Device tree Software nodes and device properties
lore
Share & tools
lore

Andy Shevchenko

author reviewer
#0 lore raw

PATCH v6 1/5 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]>
Tested-by Michael Walle <[email protected]>

---
v6: added tag (Michael), avoid shadowing error code (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 3560c4419d11..6ecc1398b0ba 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 ret;
 
-	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;
 

@@ -958,14 +971,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 6ab69871b06d..9a81c4410b9f 100644
@@ -153,12 +153,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 v6 2/5 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]>
Reviewed-by Sakari Ailus <[email protected]>

---
v6: added tag (Sakari)

 drivers/base/property.c  | 56 +++++++++++++++++++++-------------------
 include/linux/property.h |  9 +++++--
 2 files changed, 36 insertions(+), 29 deletions(-)
drivers/base/property.c +29 -27
index 6ecc1398b0ba..f0ac31d28798 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
#2 lore raw

Re: [PATCH v6 2/5] device property: Introduce fwnode_for_each_parent_node()

Hi,

Quoted 22 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]>
Reviewed-by Sakari Ailus <[email protected]>

---
v6: added tag (Sakari)

 drivers/base/property.c  | 56 +++++++++++++++++++++-------------------
 include/linux/property.h |  9 +++++--
 2 files changed, 36 insertions(+), 29 deletions(-)
drivers/base/property.c +0 -0
index 6ecc1398b0ba..f0ac31d28798 100644
@@ -596,17 +596,17 @@ EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
  */
 struct device *fwnode_get_next_parent_dev(struct fwnode_handle *fwnode)
 {

drivers/base/property.c EXPORT_SYMBOL_GPL(fwnode_get_next_parent); · line 603

Quoted 10 lines
drivers/base/property.c +2 -5
+       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);

Breadcrumbs in case anyone else ends up at this patch due to a bisect, like I just did. The above should have been changed to "get_dev_from_fwnode(parent);" in this patch. Fix posted at:

https://lore.kernel.org/r/20220429164325.1.I2a3b980ea051e59140227999f0f0ca16f1125768@changeid

author reviewer
#3 lore raw

PATCH v6 3/5 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]>
Reviewed-by Sakari Ailus <[email protected]>

---
v6: added tag (Sakari)

 drivers/base/property.c  | 20 +++++++++-----------
 include/linux/property.h |  3 +--
 2 files changed, 10 insertions(+), 13 deletions(-)
drivers/base/property.c +9 -11
index f0ac31d28798..36401cfe432c 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 v6 4/5 device property: Constify fwnode_handle_get()

In reply to Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

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

---
v6: new patch

 drivers/base/property.c  | 2 +-
 drivers/base/swnode.c    | 2 +-
 drivers/of/property.c    | 2 +-
 include/linux/fwnode.h   | 2 +-
 include/linux/property.h | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)
drivers/base/property.c +1 -1
index 36401cfe432c..1ad4b37cd312 100644
@@ -776,7 +776,7 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node);
  *
  * Returns the fwnode handle.
  */
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 	if (!fwnode_has_op(fwnode, get))
 		return fwnode;
drivers/base/swnode.c +1 -1
index b0bbd1805970..84a11008ffb8 100644
@@ -363,7 +363,7 @@ EXPORT_SYMBOL_GPL(property_entries_free);
 /* -------------------------------------------------------------------------- */
 /* fwnode operations */
 
-static struct fwnode_handle *software_node_get(struct fwnode_handle *fwnode)
+static struct fwnode_handle *software_node_get(const struct fwnode_handle *fwnode)
 {
 	struct swnode *swnode = to_swnode(fwnode);
drivers/of/property.c +1 -1
index 9a50ad25906e..8d06282af8e4 100644
@@ -858,7 +858,7 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_graph_get_remote_node);
 
-static struct fwnode_handle *of_fwnode_get(struct fwnode_handle *fwnode)
+static struct fwnode_handle *of_fwnode_get(const struct fwnode_handle *fwnode)
 {
 	return of_fwnode_handle(of_node_get(to_of_node(fwnode)));
 }
include/linux/fwnode.h +1 -1
index 9a81c4410b9f..a94bd47192a3 100644
@@ -108,7 +108,7 @@ struct fwnode_reference_args {
  *		zero on success, a negative error code otherwise.
  */
 struct fwnode_operations {
-	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
+	struct fwnode_handle *(*get)(const struct fwnode_handle *fwnode);
 	void (*put)(struct fwnode_handle *fwnode);
 	bool (*device_is_available)(const struct fwnode_handle *fwnode);
 	const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
include/linux/property.h +1 -1
index fc24d45632eb..c631ee7fd161 100644
@@ -121,7 +121,7 @@ struct fwnode_handle *fwnode_get_named_child_node(
 struct fwnode_handle *device_get_named_child_node(struct device *dev,
 						  const char *childname);
 
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
Signature · 1 line
2.35.1
reviewer
#5 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Hi Andy,

Quoted 3 lines · Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

How?

If the fwnode is const, then the struct it contains must be presumed to be const, too.

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

---
v6: new patch

 drivers/base/property.c  | 2 +-
 drivers/base/swnode.c    | 2 +-
 drivers/of/property.c    | 2 +-
 include/linux/fwnode.h   | 2 +-
 include/linux/property.h | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)
drivers/base/property.c +1 -1
index 36401cfe432c..1ad4b37cd312 100644
@@ -776,7 +776,7 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node);
  *
  * Returns the fwnode handle.
  */
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 	if (!fwnode_has_op(fwnode, get))
 		return fwnode;
drivers/base/swnode.c +1 -1
index b0bbd1805970..84a11008ffb8 100644
@@ -363,7 +363,7 @@ EXPORT_SYMBOL_GPL(property_entries_free);
 /* -------------------------------------------------------------------------- */
 /* fwnode operations */
 
-static struct fwnode_handle *software_node_get(struct fwnode_handle *fwnode)
+static struct fwnode_handle *software_node_get(const struct fwnode_handle *fwnode)
 {
 	struct swnode *swnode = to_swnode(fwnode);
drivers/of/property.c +1 -1
index 9a50ad25906e..8d06282af8e4 100644
@@ -858,7 +858,7 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_graph_get_remote_node);
 
-static struct fwnode_handle *of_fwnode_get(struct fwnode_handle *fwnode)
+static struct fwnode_handle *of_fwnode_get(const struct fwnode_handle *fwnode)
 {
 	return of_fwnode_handle(of_node_get(to_of_node(fwnode)));
 }
include/linux/fwnode.h +1 -1
index 9a81c4410b9f..a94bd47192a3 100644
@@ -108,7 +108,7 @@ struct fwnode_reference_args {
  *		zero on success, a negative error code otherwise.
  */
 struct fwnode_operations {
-	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
+	struct fwnode_handle *(*get)(const struct fwnode_handle *fwnode);
 	void (*put)(struct fwnode_handle *fwnode);
 	bool (*device_is_available)(const struct fwnode_handle *fwnode);
 	const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
include/linux/property.h +1 -1
index fc24d45632eb..c631ee7fd161 100644
@@ -121,7 +121,7 @@ struct fwnode_handle *fwnode_get_named_child_node(
 struct fwnode_handle *device_get_named_child_node(struct device *dev,
 						  const char *childname);
 
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
Signature · 1 line
2.35.1
Signature · 1 line
Sakari Ailus
#6 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 9 lines · Sakari Ailus
Quoted 3 lines · Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

How?

If the fwnode is const, then the struct it contains must be presumed to be const, too.

Why? The idea is that we are not updating the fwnode, but the container. The container may or may not be const. It's orthogonal, no?

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

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Sakari Ailus
Quoted 3 lines · Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

Quoted 7 lines
Quoted 4 lines

How?

If the fwnode is const, then the struct it contains must be presumed to be const, too.

Why? The idea is that we are not updating the fwnode, but the container. The container may or may not be const. It's orthogonal, no?

As you wrote: may or may not. The stricter requirement, i.e. const, must be thus followed. I think it would be fine (after adding a comment on what is being done) if you *know* the container struct is not const. But that is not the case here.

Signature · 1 line
Sakari Ailus
#8 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 13 lines · Sakari Ailus
Quoted 12 lines · Andy Shevchenko
Quoted 9 lines · Sakari Ailus
Quoted 3 lines · Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

How?

If the fwnode is const, then the struct it contains must be presumed to be const, too.

Quoted 7 lines
Quoted 2 lines

Why? The idea is that we are not updating the fwnode, but the container. The container may or may not be const. It's orthogonal, no?

As you wrote: may or may not. The stricter requirement, i.e. const, must be thus followed. I think it would be fine (after adding a comment on what is being done) if you *know* the container struct is not const. But that is not the case here.

But even with the original code one may not guarantee that. How the original code works or prevents of using a const container against non-const fwnode pointer?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#9 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 18 lines · Andy Shevchenko
Quoted 16 lines · Sakari Ailus
Quoted 14 lines · Andy Shevchenko
Quoted 9 lines · Sakari Ailus
Quoted 3 lines · Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

How?

If the fwnode is const, then the struct it contains must be presumed to be const, too.

Why? The idea is that we are not updating the fwnode, but the container. The container may or may not be const. It's orthogonal, no?

Quoted 8 lines
Quoted 4 lines

As you wrote: may or may not. The stricter requirement, i.e. const, must be thus followed. I think it would be fine (after adding a comment on what is being done) if you *know* the container struct is not const. But that is not the case here.

But even with the original code one may not guarantee that. How the original code works or prevents of using a const container against non-const fwnode pointer?

I don't think that this is the point here.

If const struct fwnode_handle *fwnode is passed to the ->get() callback, the callback itself (and any function called by it) shouldn't attempt to update the memory pointed to by fwnode. It need not be the memory starting at the fwnode address IIUC, so that would cover the whole object the fwnode is embedded in.

This way the caller of ->get() can assume the immutability of the memory passed to it for read access.

The question is whether or not ->get() needs to update the memory in question. If it doesn't, making fwnode const is correct.

build bot
#10 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

In reply to Andy Shevchenko
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 robh/for-next linus/master v5.18-rc1 next-20220408] [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/20220409-025056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: hexagon-randconfig-r041-20220408 (https://download.01.org/0day-ci/archive/20220409/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c29a51b3a257908aebc01cd7c4655665db317d66)

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/477683439b5ee0954b08970d8c356b4cdaca8bc0
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/20220409-025056 git checkout 477683439b5ee0954b08970d8c356b4cdaca8bc0
# 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:782:10: error: returning 'const struct fwnode_handle *' from a function with result type 'struct fwnode_handle *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
                   return fwnode;
                          ^~~~~~

1 error generated.

vim +782 drivers/base/property.c

613e97218ccbd7 Adam Thomson    2016-06-21  772  
e7887c284969a2 Sakari Ailus    2017-03-28  773  /**
e7887c284969a2 Sakari Ailus    2017-03-28  774   * fwnode_handle_get - Obtain a reference to a device node
e7887c284969a2 Sakari Ailus    2017-03-28  775   * @fwnode: Pointer to the device node to obtain the reference to.
cf89a31ca55272 Sakari Ailus    2017-09-19  776   *
cf89a31ca55272 Sakari Ailus    2017-09-19  777   * Returns the fwnode handle.
e7887c284969a2 Sakari Ailus    2017-03-28  778   */
477683439b5ee0 Andy Shevchenko 2022-04-08  779  struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
e7887c284969a2 Sakari Ailus    2017-03-28  780  {
cf89a31ca55272 Sakari Ailus    2017-09-19  781  	if (!fwnode_has_op(fwnode, get))
cf89a31ca55272 Sakari Ailus    2017-09-19 @782  		return fwnode;
cf89a31ca55272 Sakari Ailus    2017-09-19  783  
cf89a31ca55272 Sakari Ailus    2017-09-19  784  	return fwnode_call_ptr_op(fwnode, get);
e7887c284969a2 Sakari Ailus    2017-03-28  785  }
e7887c284969a2 Sakari Ailus    2017-03-28  786  EXPORT_SYMBOL_GPL(fwnode_handle_get);
e7887c284969a2 Sakari Ailus    2017-03-28  787
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
build bot
#11 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

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 robh/for-next linus/master v5.18-rc1 next-20220408] [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/20220409-025056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220409/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0

reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/477683439b5ee0954b08970d8c356b4cdaca8bc0
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/20220409-025056 git checkout 477683439b5ee0954b08970d8c356b4cdaca8bc0
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=um SUBARCH=i386 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_handle_get':

>> drivers/base/property.c:782:24: warning: return discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     782 |                 return fwnode;
         |                        ^~~~~~

vim +/const +782 drivers/base/property.c

613e97218ccbd7f Adam Thomson    2016-06-21  772  
e7887c284969a23 Sakari Ailus    2017-03-28  773  /**
e7887c284969a23 Sakari Ailus    2017-03-28  774   * fwnode_handle_get - Obtain a reference to a device node
e7887c284969a23 Sakari Ailus    2017-03-28  775   * @fwnode: Pointer to the device node to obtain the reference to.
cf89a31ca55272e Sakari Ailus    2017-09-19  776   *
cf89a31ca55272e Sakari Ailus    2017-09-19  777   * Returns the fwnode handle.
e7887c284969a23 Sakari Ailus    2017-03-28  778   */
477683439b5ee09 Andy Shevchenko 2022-04-08  779  struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
e7887c284969a23 Sakari Ailus    2017-03-28  780  {
cf89a31ca55272e Sakari Ailus    2017-09-19  781  	if (!fwnode_has_op(fwnode, get))
cf89a31ca55272e Sakari Ailus    2017-09-19 @782  		return fwnode;
cf89a31ca55272e Sakari Ailus    2017-09-19  783  
cf89a31ca55272e Sakari Ailus    2017-09-19  784  	return fwnode_call_ptr_op(fwnode, get);
e7887c284969a23 Sakari Ailus    2017-03-28  785  }
e7887c284969a23 Sakari Ailus    2017-03-28  786  EXPORT_SYMBOL_GPL(fwnode_handle_get);
e7887c284969a23 Sakari Ailus    2017-03-28  787
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
maintainer
#12 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

In reply to Andy Shevchenko
Quoted 72 lines · Andy Shevchenko

As to_of_node() suggests and the way the code in the OF and software node back ends actually uses the fwnode handle, it may be constified. Do this for good.

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

---
v6: new patch

 drivers/base/property.c  | 2 +-
 drivers/base/swnode.c    | 2 +-
 drivers/of/property.c    | 2 +-
 include/linux/fwnode.h   | 2 +-
 include/linux/property.h | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)
drivers/base/property.c +1 -1
index 36401cfe432c..1ad4b37cd312 100644
@@ -776,7 +776,7 @@ EXPORT_SYMBOL_GPL(device_get_named_child_node);
  *
  * Returns the fwnode handle.
  */
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
        if (!fwnode_has_op(fwnode, get))
                return fwnode;
drivers/base/swnode.c +1 -1
index b0bbd1805970..84a11008ffb8 100644
@@ -363,7 +363,7 @@ EXPORT_SYMBOL_GPL(property_entries_free);
 /* -------------------------------------------------------------------------- */
 /* fwnode operations */
 
-static struct fwnode_handle *software_node_get(struct fwnode_handle *fwnode)
+static struct fwnode_handle *software_node_get(const struct fwnode_handle *fwnode)
 {
        struct swnode *swnode = to_swnode(fwnode);
drivers/of/property.c +1 -1
index 9a50ad25906e..8d06282af8e4 100644
@@ -858,7 +858,7 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
 }
 EXPORT_SYMBOL(of_graph_get_remote_node);
 
-static struct fwnode_handle *of_fwnode_get(struct fwnode_handle *fwnode)
+static struct fwnode_handle *of_fwnode_get(const struct fwnode_handle *fwnode)
 {
        return of_fwnode_handle(of_node_get(to_of_node(fwnode)));
 }
include/linux/fwnode.h +1 -1
index 9a81c4410b9f..a94bd47192a3 100644
@@ -108,7 +108,7 @@ struct fwnode_reference_args {
  *             zero on success, a negative error code otherwise.
  */
 struct fwnode_operations {
-       struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
+       struct fwnode_handle *(*get)(const struct fwnode_handle *fwnode);
        void (*put)(struct fwnode_handle *fwnode);
        bool (*device_is_available)(const struct fwnode_handle *fwnode);
        const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
include/linux/property.h +0 -0
index fc24d45632eb..c631ee7fd161 100644

include/linux/property.h struct fwnode_handle *fwnode_get_named_child_node(

Quoted 9 lines
include/linux/property.h +1 -2
@@ -121,7 +121,7 @@ struct fwnode_handle *fwnode_get_named_child_node(
 struct fwnode_handle *device_get_named_child_node(struct device *dev,
                                                  const char *childname);
 
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
--

Why is 0-day complaining about this one?

author reviewer
#13 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 2 lines · Rafael J. Wysocki

On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 6 lines

drivers/base/property.c EXPORT_SYMBOL_GPL(device_get_named_child_node); · line 782

Quoted 6 lines
drivers/base/property.c +1 -1
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 
        if (!fwnode_has_op(fwnode, get))
                return fwnode;

^^^^, so it needs a casting, but then we have to comment why is so.

Quoted 1 line

Why is 0-day complaining about this one?

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

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Rafael J. Wysocki

On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 8 lines
Quoted 6 lines

drivers/base/property.c EXPORT_SYMBOL_GPL(device_get_named_child_node); · line 782

Quoted 6 lines
drivers/base/property.c +1 -1
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 
        if (!fwnode_has_op(fwnode, get))
                return fwnode;

^^^^, so it needs a casting, but then we have to comment why is so.

Note, it means that the fwnode parameter either invalid or has no given option. It's not a problem to drop casting in the first case, but the second one should be justified and Sakari wants to be sure that the initial container is not const, which seems can't be achieved even with the original code.

Quoted 1 line
Quoted 1 line

Why is 0-day complaining about this one?

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

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 14 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Rafael J. Wysocki

On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
drivers/base/property.c +1 -1
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 
        if (!fwnode_has_op(fwnode, get))
                return fwnode;
Quoted 6 lines
Quoted 1 line

^^^^, so it needs a casting, but then we have to comment why is so.

Note, it means that the fwnode parameter either invalid or has no given option. It's not a problem to drop casting in the first case, but the second one should be justified and Sakari wants to be sure that the initial container is not const, which seems can't be achieved even with the original code.

I wonder if I'm missing something. The fwnode argument originally was not const here.

Signature · 1 line
Sakari Ailus
author reviewer
#16 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 5 lines · Sakari Ailus
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Rafael J. Wysocki

On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines

drivers/base/property.c EXPORT_SYMBOL_GPL(device_get_named_child_node); · line 782

Quoted 6 lines
drivers/base/property.c +1 -1
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 
        if (!fwnode_has_op(fwnode, get))
                return fwnode;

^^^^, so it needs a casting, but then we have to comment why is so.

Quoted 7 lines
Quoted 4 lines

Note, it means that the fwnode parameter either invalid or has no given option. It's not a problem to drop casting in the first case, but the second one should be justified and Sakari wants to be sure that the initial container is not const, which seems can't be achieved even with the original code.

I wonder if I'm missing something. The fwnode argument originally was not const here.

Yes, and our discussion went to the direction of what const qualifier implies here. I assume that the const means that we do not modify the fwnode object, while its container is another story which we have no influence on. You, if I read your messages correctly, insisting that const here implies that the container object is const as well.

Reading current implementation I see now, that with children APIs we have two pointers passed, while with parent APIs only a single one. In children API due to above is easy to use const qualifier for the first argument. Parent APIs missed that and hence have this problem that we can't constify their parameters.

to_of_node() expects const parameter while returns non-const container. Is it a subtle issue there? (I believe it should be consistent then)

This patch and the followed one can be moved without understanding why we need the non-const parameter there.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#17 lore raw

Re: [PATCH v6 4/5] device property: Constify fwnode_handle_get()

Quoted 33 lines · Andy Shevchenko
Quoted 5 lines · Sakari Ailus
Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Rafael J. Wysocki

On Fri, Apr 8, 2022 at 8:49 PM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines

drivers/base/property.c EXPORT_SYMBOL_GPL(device_get_named_child_node); · line 782

Quoted 6 lines
drivers/base/property.c +1 -1
-struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
+struct fwnode_handle *fwnode_handle_get(const struct fwnode_handle *fwnode)
 {
 
        if (!fwnode_has_op(fwnode, get))
                return fwnode;

^^^^, so it needs a casting, but then we have to comment why is so.

Quoted 7 lines
Quoted 4 lines

Note, it means that the fwnode parameter either invalid or has no given option. It's not a problem to drop casting in the first case, but the second one should be justified and Sakari wants to be sure that the initial container is not const, which seems can't be achieved even with the original code.

I wonder if I'm missing something. The fwnode argument originally was not const here.

Yes, and our discussion went to the direction of what const qualifier implies here. I assume that the const means that we do not modify the fwnode object, while its container is another story which we have no influence on. You, if I read your messages correctly, insisting that const here implies that the container object is const as well.

Quoted 8 lines

Reading current implementation I see now, that with children APIs we have two pointers passed, while with parent APIs only a single one. In children API due to above is easy to use const qualifier for the first argument. Parent APIs missed that and hence have this problem that we can't constify their parameters.

to_of_node() expects const parameter while returns non-const container. Is it a subtle issue there? (I believe it should be consistent then)

This is fine AFAICS.

The const parameter means that to_of_node() will not update the memory pointed to by it, which is correct.

The value returned by it may be used by its caller in whatever way they like, because the caller has no obligation to preserve the memory pointed to by it, unless they've also received that pointer with the const qualifier, but then they need to know how it is related to the to_of_node() return value and what to do with it.

IOW, to_of_node() has no information on its caller's obligations with respect to the memory pointed to by its argument, so it is OK for it to return a non-const result. Moreover, if it had done otherwise, it might have created an obligation for the caller that didn't exist before.

Quoted 2 lines

This patch and the followed one can be moved without understanding why we need the non-const parameter there.

I'm not sure what you mean here, sorry.

author reviewer
#18 lore raw

PATCH v6 5/5 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]>
Reviewed-by Sakari Ailus <[email protected]>

---
v6: added tag (Sakari), since previous patch no warnings anymore (LKP)

 drivers/base/property.c  | 7 +++----
 include/linux/property.h | 9 ++++-----
 2 files changed, 7 insertions(+), 9 deletions(-)
drivers/base/property.c +3 -4
index 1ad4b37cd312..f289f582209c 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 c631ee7fd161..e3390401dd63 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
author reviewer
#19 lore raw
In reply to Andy Shevchenko
Quoted 8 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...
Quoted 7 lines

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.

Rafael and Greg, if this okay for you, can the first three patches be applied, so we will have at least the fix in and consider constification a further work?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#20 lore raw
Quoted 15 lines · Andy Shevchenko
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 5 lines
Quoted 1 line

To prevent this, allow an error pointer to be passed to the fwnode APIs.

Rafael and Greg, if this okay for you, can the first three patches be applied, so we will have at least the fix in and consider constification a further work?

Give us a chance, you sent this on friday and are asking about it first thing Monday morning?

Please go and review other patches sent on the list to help us catch up.

greg k-h

author reviewer
#21 lore raw
Quoted 18 lines · Greg Kroah-Hartman
Quoted 17 lines · Andy Shevchenko
Quoted 15 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.

Quoted 8 lines
Quoted 3 lines

Rafael and Greg, if this okay for you, can the first three patches be applied, so we will have at least the fix in and consider constification a further work?

Give us a chance, you sent this on friday and are asking about it first thing Monday morning?

Please go and review other patches sent on the list to help us catch up.

OK! Reviewed (actually commented on) a few patches so far.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#22 lore raw
Quoted 24 lines · Andy Shevchenko
Quoted 22 lines · Greg Kroah-Hartman
Quoted 20 lines · Andy Shevchenko
Quoted 8 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...
Quoted 7 lines

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.

Rafael and Greg, if this okay for you, can the first three patches be applied, so we will have at least the fix in and consider constification a further work?

Quoted 6 lines
Quoted 4 lines

Give us a chance, you sent this on friday and are asking about it first thing Monday morning?

Please go and review other patches sent on the list to help us catch up.

OK! Reviewed (actually commented on) a few patches so far.

I've just queued up the first three patches in the series for 5.19.

Thanks!

author reviewer
#23 lore raw
Quoted 5 lines · Rafael J. Wysocki
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Greg Kroah-Hartman
Quoted 1 line · Andy Shevchenko

On Fri, Apr 08, 2022 at 09:48:40PM +0300, Andy Shevchenko wrote:

...

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

Rafael and Greg, if this okay for you, can the first three patches be applied, so we will have at least the fix in and consider constification a further work?

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Give us a chance, you sent this on friday and are asking about it first thing Monday morning?

Please go and review other patches sent on the list to help us catch up.

OK! Reviewed (actually commented on) a few patches so far.

I've just queued up the first three patches in the series for 5.19.

Thank you!

Signature · 2 lines
With Best Regards,
Andy Shevchenko