diff mbox series

[v6,07/18] of: dynamic: change type of of_{at,de}tach_node() to void

Message ID 1541431515-25197-8-git-send-email-frowand.list@gmail.com (mailing list archive)
State New, archived
Headers show
Series of: overlay: validation checks, subsequent fixes | expand

Commit Message

Frank Rowand Nov. 5, 2018, 3:25 p.m. UTC 
From: Frank Rowand <frank.rowand@sony.com>

of_attach_node() and of_detach_node() always return zero, so
their return value is meaningless.  Change their type to void
and fix all callers to ignore return value.

Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 arch/powerpc/platforms/pseries/dlpar.c    | 13 ++-----------
 arch/powerpc/platforms/pseries/reconfig.c |  6 +-----
 drivers/of/dynamic.c                      |  9 ++-------
 include/linux/of.h                        |  4 ++--
 4 files changed, 7 insertions(+), 25 deletions(-)

Comments

Michael Ellerman Nov. 7, 2018, 12:08 p.m. UTC | #1 
frowand.list@gmail.com writes:

> From: Frank Rowand <frank.rowand@sony.com>
>
> of_attach_node() and of_detach_node() always return zero, so
> their return value is meaningless.

But should they always return zero?

At least __of_attach_node_sysfs() can fail in several ways.

And there's also this in __of_detach_node() which should probably be
returning an error:

	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
        	return;


Seems to me we should instead be fixing these to propagate errors,
rather than hiding them?

cheers
Frank Rowand Nov. 7, 2018, 2:55 p.m. UTC | #2 
On 11/7/18 4:08 AM, Michael Ellerman wrote:
> frowand.list@gmail.com writes:
> 
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> of_attach_node() and of_detach_node() always return zero, so
>> their return value is meaningless.
> 
> But should they always return zero?
> 
> At least __of_attach_node_sysfs() can fail in several ways.

Sigh.  And of_reconfig_notify() can fail.  And at one point in the
history the return value of of_reconfig_notify() was returned by
of_attach_node() if of_reconfig_notify() failed.


> And there's also this in __of_detach_node() which should probably be
> returning an error:
> 
> 	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
>         	return;
> 
> 
> Seems to me we should instead be fixing these to propagate errors,
> rather than hiding them?

The history of how of_attach_node() stopped propagating errors is
a bit more complex than I want to dig into at the moment.  So I'll
drop this patch from the series and add investigating this onto
my todo list.  I suspect that the result of investigating will be
that error return values should not be ignored in of_attach_node()
and of_detach_node(), but should instead be propagated to the
callers, as you suggest.

-Frank

> 
> cheers
>
Michael Ellerman Nov. 7, 2018, 11:48 p.m. UTC | #3 
Frank Rowand <frowand.list@gmail.com> writes:
> On 11/7/18 4:08 AM, Michael Ellerman wrote:
>> frowand.list@gmail.com writes:
>> 
>>> From: Frank Rowand <frank.rowand@sony.com>
>>>
>>> of_attach_node() and of_detach_node() always return zero, so
>>> their return value is meaningless.
>> 
>> But should they always return zero?
>> 
>> At least __of_attach_node_sysfs() can fail in several ways.
>
> Sigh.  And of_reconfig_notify() can fail.  And at one point in the
> history the return value of of_reconfig_notify() was returned by
> of_attach_node() if of_reconfig_notify() failed.
>
>> And there's also this in __of_detach_node() which should probably be
>> returning an error:
>> 
>> 	if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
>>         	return;
>> 
>> 
>> Seems to me we should instead be fixing these to propagate errors,
>> rather than hiding them?
>
> The history of how of_attach_node() stopped propagating errors is
> a bit more complex than I want to dig into at the moment.  So I'll
> drop this patch from the series and add investigating this onto
> my todo list.  I suspect that the result of investigating will be
> that error return values should not be ignored in of_attach_node()
> and of_detach_node(), but should instead be propagated to the
> callers, as you suggest.

Thanks.

cheers
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 17958043e7f7..7c5a6f8fe9c1 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -242,15 +242,9 @@  struct device_node *dlpar_configure_connector(__be32 drc_index,
 
 int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
 {
-	int rc;
-
 	dn->parent = parent;
 
-	rc = of_attach_node(dn);
-	if (rc) {
-		printk(KERN_ERR "Failed to add device node %pOF\n", dn);
-		return rc;
-	}
+	of_attach_node(dn);
 
 	return 0;
 }
@@ -258,7 +252,6 @@  int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
 int dlpar_detach_node(struct device_node *dn)
 {
 	struct device_node *child;
-	int rc;
 
 	child = of_get_next_child(dn, NULL);
 	while (child) {
@@ -266,9 +259,7 @@  int dlpar_detach_node(struct device_node *dn)
 		child = of_get_next_child(dn, child);
 	}
 
-	rc = of_detach_node(dn);
-	if (rc)
-		return rc;
+	of_detach_node(dn);
 
 	of_node_put(dn);
 
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 0e0208117e77..0b72098da454 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -47,11 +47,7 @@  static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 		goto out_err;
 	}
 
-	err = of_attach_node(np);
-	if (err) {
-		printk(KERN_ERR "Failed to add device node %s\n", path);
-		goto out_err;
-	}
+	of_attach_node(np);
 
 	of_node_put(np->parent);
 
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 146681540487..beea792debb6 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -224,7 +224,7 @@  static void __of_attach_node(struct device_node *np)
 /**
  * of_attach_node() - Plug a device node into the tree and global list.
  */
-int of_attach_node(struct device_node *np)
+void of_attach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
 	unsigned long flags;
@@ -241,8 +241,6 @@  int of_attach_node(struct device_node *np)
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
-
-	return 0;
 }
 
 void __of_detach_node(struct device_node *np)
@@ -273,11 +271,10 @@  void __of_detach_node(struct device_node *np)
 /**
  * of_detach_node() - "Unplug" a node from the device tree.
  */
-int of_detach_node(struct device_node *np)
+void of_detach_node(struct device_node *np)
 {
 	struct of_reconfig_data rd;
 	unsigned long flags;
-	int rc = 0;
 
 	memset(&rd, 0, sizeof(rd));
 	rd.dn = np;
@@ -291,8 +288,6 @@  int of_detach_node(struct device_node *np)
 	mutex_unlock(&of_mutex);
 
 	of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
-
-	return rc;
 }
 EXPORT_SYMBOL_GPL(of_detach_node);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 664cd5573ae2..5dd9ff480397 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -410,8 +410,8 @@  extern int of_alias_get_alias_list(const struct of_device_id *matches,
 #define OF_RECONFIG_REMOVE_PROPERTY	0x0004
 #define OF_RECONFIG_UPDATE_PROPERTY	0x0005
 
-extern int of_attach_node(struct device_node *);
-extern int of_detach_node(struct device_node *);
+extern void of_attach_node(struct device_node *np);
+extern void of_detach_node(struct device_node *np);
 
 #define of_match_ptr(_ptr)	(_ptr)