powerpc/pseries: extract of_helpers module

PATCH Superseded Andy Shevchenko 3 participants 11 messages Aug 11, 2015 – Oct 1, 2015 PowerPC

lore

Share & tools

lore

Andy Shevchenko

author

2015-08-11 11:23 UTC #0 lore raw

PATCH v2 1/5 powerpc/pseries: extract of_helpers module

Extract a new module to share the code between other modules.

There is no functional change.

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

---
 arch/powerpc/platforms/pseries/Makefile     |  1 +
 arch/powerpc/platforms/pseries/of_helpers.c | 38 +++++++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/of_helpers.h |  8 ++++++
 arch/powerpc/platforms/pseries/reconfig.c   | 34 ++------------------------
 4 files changed, 49 insertions(+), 32 deletions(-)
 create mode 100644 arch/powerpc/platforms/pseries/of_helpers.c
 create mode 100644 arch/powerpc/platforms/pseries/of_helpers.h

arch/powerpc/platforms/pseries/Makefile +1 -0

index 0348079..2e857c2 100644
@@ -2,6 +2,7 @@ ccflags-$(CONFIG_PPC64)			:= $(NO_MINIMAL_TOC)
 ccflags-$(CONFIG_PPC_PSERIES_DEBUG)	+= -DDEBUG
 
 obj-y			:= lpar.o hvCall.o nvram.o reconfig.o \
+			   of_helpers.o \
 			   setup.o iommu.o event_sources.o ras.o \
 			   firmware.o power.o dlpar.o mobility.o rng.o
 obj-$(CONFIG_SMP)	+= smp.o

arch/powerpc/platforms/pseries/of_helpers.c new file +38 -0

new file mode 100644
index 0000000..1cbd896
@@ -0,0 +1,38 @@
+#include <linux/string.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+#include "of_helpers.h"
+
+/**
+ * pseries_of_derive_parent - basically like dirname(1)
+ * @path:  the full_name of a node to be added to the tree
+ *
+ * Returns the node which should be the parent of the node
+ * described by path.  E.g., for path = "/foo/bar", returns
+ * the node with full_name = "/foo".
+ */
+struct device_node *pseries_of_derive_parent(const char *path)
+{
+	struct device_node *parent = NULL;
+	char *parent_path = "/";
+	size_t parent_path_len = strrchr(path, '/') - path + 1;
+
+	/* reject if path is "/" */
+	if (!strcmp(path, "/"))
+		return ERR_PTR(-EINVAL);
+
+	if (strrchr(path, '/') != path) {
+		parent_path = kmalloc(parent_path_len, GFP_KERNEL);
+		if (!parent_path)
+			return ERR_PTR(-ENOMEM);
+		strlcpy(parent_path, path, parent_path_len);
+	}
+	parent = of_find_node_by_path(parent_path);
+	if (!parent)
+		return ERR_PTR(-EINVAL);
+	if (strcmp(parent_path, "/"))
+		kfree(parent_path);
+	return parent;
+}

arch/powerpc/platforms/pseries/of_helpers.h new file +8 -0

new file mode 100644
index 0000000..bb83d39
@@ -0,0 +1,8 @@
+#ifndef _PSERIES_OF_HELPERS_H
+#define _PSERIES_OF_HELPERS_H
+
+#include <linux/of.h>
+
+struct device_node *pseries_of_derive_parent(const char *path);
+
+#endif /* _PSERIES_OF_HELPERS_H */

arch/powerpc/platforms/pseries/reconfig.c +2 ~~-32~~

index 0f31952..7c7fcc0 100644
@@ -22,37 +22,7 @@
 #include <asm/uaccess.h>
 #include <asm/mmu.h>
 
-/**
- *	derive_parent - basically like dirname(1)
- *	@path:  the full_name of a node to be added to the tree
- *
- *	Returns the node which should be the parent of the node
- *	described by path.  E.g., for path = "/foo/bar", returns
- *	the node with full_name = "/foo".
- */
-static struct device_node *derive_parent(const char *path)
-{
-	struct device_node *parent = NULL;
-	char *parent_path = "/";
-	size_t parent_path_len = strrchr(path, '/') - path + 1;
-
-	/* reject if path is "/" */
-	if (!strcmp(path, "/"))
-		return ERR_PTR(-EINVAL);
-
-	if (strrchr(path, '/') != path) {
-		parent_path = kmalloc(parent_path_len, GFP_KERNEL);
-		if (!parent_path)
-			return ERR_PTR(-ENOMEM);
-		strlcpy(parent_path, path, parent_path_len);
-	}
-	parent = of_find_node_by_path(parent_path);
-	if (!parent)
-		return ERR_PTR(-EINVAL);
-	if (strcmp(parent_path, "/"))
-		kfree(parent_path);
-	return parent;
-}
+#include "of_helpers.h"
 
 static int pSeries_reconfig_add_node(const char *path, struct property *proplist)
 {

@@ -71,7 +41,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
 	of_node_set_flag(np, OF_DYNAMIC);
 	of_node_init(np);
 
-	np->parent = derive_parent(path);
+	np->parent = pseries_of_derive_parent(path);
 	if (IS_ERR(np->parent)) {
 		err = PTR_ERR(np->parent);
 		goto out_err;

Signature · 1 line

2.5.0

Andy Shevchenko

author

2015-08-11 11:23 UTC #1 lore raw

PATCH v2 2/5 powerpc/pseries: fix a potential memory leak

In case we have a full node name like /foo/bar and /foo is not found the parent_path left unfreed. So, free a memory before return to a caller.

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

---
 arch/powerpc/platforms/pseries/of_helpers.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

arch/powerpc/platforms/pseries/of_helpers.c +2 -4

index 1cbd896..2f363e3 100644
@@ -15,7 +15,7 @@
  */
 struct device_node *pseries_of_derive_parent(const char *path)
 {
-	struct device_node *parent = NULL;
+	struct device_node *parent;
 	char *parent_path = "/";
 	size_t parent_path_len = strrchr(path, '/') - path + 1;

@@ -30,9 +30,7 @@ struct device_node *pseries_of_derive_parent(const char *path)
 		strlcpy(parent_path, path, parent_path_len);
 	}
 	parent = of_find_node_by_path(parent_path);
-	if (!parent)
-		return ERR_PTR(-EINVAL);
 	if (strcmp(parent_path, "/"))
 		kfree(parent_path);
-	return parent;
+	return parent ? parent : ERR_PTR(-EINVAL);
 }

Signature · 1 line

2.5.0

Andy Shevchenko

author

2015-08-11 11:23 UTC #2 lore raw

PATCH v2 3/5 powerpc/pseries: replace kmalloc + strlcpy

In reply to Andy Shevchenko

The helper kstrndup() will do the same in one line.

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

---
 arch/powerpc/platforms/pseries/of_helpers.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/powerpc/platforms/pseries/of_helpers.c +1 -2

index 2f363e3..8c6b05a 100644
@@ -24,10 +24,9 @@ struct device_node *pseries_of_derive_parent(const char *path)
 		return ERR_PTR(-EINVAL);
 
 	if (strrchr(path, '/') != path) {
-		parent_path = kmalloc(parent_path_len, GFP_KERNEL);
+		parent_path = kstrndup(path, parent_path_len, GFP_KERNEL);
 		if (!parent_path)
 			return ERR_PTR(-ENOMEM);
-		strlcpy(parent_path, path, parent_path_len);
 	}
 	parent = of_find_node_by_path(parent_path);
 	if (strcmp(parent_path, "/"))

Signature · 1 line

2.5.0

Andy Shevchenko

author

2015-08-11 11:23 UTC #3 lore raw

PATCH v2 4/5 powerpc/pseries: handle nodes without '/'

In reply to Andy Shevchenko

In case we have node without '/' strrchr() returns NULL which might lead to crash. Replace strrchr() by kbasename() and modify condition to avoid such behaviour.

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

---
 arch/powerpc/platforms/pseries/of_helpers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

arch/powerpc/platforms/pseries/of_helpers.c +3 -3

index 8c6b05a..4417afe 100644
@@ -17,14 +17,14 @@ struct device_node *pseries_of_derive_parent(const char *path)
 {
 	struct device_node *parent;
 	char *parent_path = "/";
-	size_t parent_path_len = strrchr(path, '/') - path + 1;
+	const char *tail = kbasename(path);
 
 	/* reject if path is "/" */
 	if (!strcmp(path, "/"))
 		return ERR_PTR(-EINVAL);
 
-	if (strrchr(path, '/') != path) {
-		parent_path = kstrndup(path, parent_path_len, GFP_KERNEL);
+	if (tail > path + 1) {
+		parent_path = kstrndup(path, tail - path, GFP_KERNEL);
 		if (!parent_path)
 			return ERR_PTR(-ENOMEM);
 	}

Signature · 1 line

2.5.0

Andy Shevchenko

author

2015-08-11 11:23 UTC #4 lore raw

PATCH v2 5/5 powerpc/pseries: re-use code from of_helpers module

In reply to Andy Shevchenko

The derive_parent() has similar semantics to what we have in newly introduced of_helpers module. The replacement reduces code base and propagates the actual error code to the caller.

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

---
 arch/powerpc/platforms/pseries/dlpar.c | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

arch/powerpc/platforms/pseries/dlpar.c +5 ~~-26~~

index 47d9cebe..b7f243c 100644
@@ -18,6 +18,8 @@
 #include <linux/cpu.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+
+#include "of_helpers.h"
 #include "offline_states.h"
 #include "pseries.h"

@@ -244,36 +246,13 @@ cc_error:
 	return first_dn;
 }
 
-static struct device_node *derive_parent(const char *path)
-{
-	struct device_node *parent;
-	char *last_slash;
-
-	last_slash = strrchr(path, '/');
-	if (last_slash == path) {
-		parent = of_find_node_by_path("/");
-	} else {
-		char *parent_path;
-		int parent_path_len = last_slash - path + 1;
-		parent_path = kmalloc(parent_path_len, GFP_KERNEL);
-		if (!parent_path)
-			return NULL;
-
-		strlcpy(parent_path, path, parent_path_len);
-		parent = of_find_node_by_path(parent_path);
-		kfree(parent_path);
-	}
-
-	return parent;
-}
-
 int dlpar_attach_node(struct device_node *dn)
 {
 	int rc;
 
-	dn->parent = derive_parent(dn->full_name);
-	if (!dn->parent)
-		return -ENOMEM;
+	dn->parent = pseries_of_derive_parent(dn->full_name);
+	if (IS_ERR(dn->parent))
+		return PTR_ERR(dn_parent);
 
 	rc = of_attach_node(dn);
 	if (rc) {

Signature · 1 line

2.5.0

Andy Shevchenko

author

2015-09-30 16:20 UTC #6 lore raw

Re: [v2,5/5] powerpc/pseries: re-use code from of_helpers module

Quoted 68 lines · Michael Ellerman

Quoted 54 lines · Andy Shevchenko

The derive_parent() has similar semantics to what we have in newly introduced
of_helpers module. The replacement reduces code base and propagates the actual
error code to the caller.

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

---
 arch/powerpc/platforms/pseries/dlpar.c | 31 +++++-----------------
---------
 1 file changed, 5 insertions(+), 26 deletions(-)

arch/powerpc/platforms/pseries/dlpar.c +2 ~~-23~~

index 47d9cebe..b7f243c 100644
@@ -18,6 +18,8 @@
 #include <linux/cpu.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+
+#include "of_helpers.h"
 #include "offline_states.h"
 #include "pseries.h"

@@ -244,36 +246,13 @@ cc_error:
 	return first_dn;
 }
 
-static struct device_node *derive_parent(const char *path)
-{
-	struct device_node *parent;
-	char *last_slash;
-
-	last_slash = strrchr(path, '/');
-	if (last_slash == path) {
-		parent = of_find_node_by_path("/");
-	} else {
-		char *parent_path;
-		int parent_path_len = last_slash - path + 1;
-		parent_path = kmalloc(parent_path_len, 
 GFP_KERNEL);
-		if (!parent_path)
-			return NULL;
-
-		strlcpy(parent_path, path, parent_path_len);
-		parent = of_find_node_by_path(parent_path);
-		kfree(parent_path);
-	}
-
-	return parent;
-}
-

arch/powerpc/platforms/pseries/dlpar.c cc_error: · line 256

Quoted 10 lines

arch/powerpc/platforms/pseries/dlpar.c +3 -3

 int dlpar_attach_node(struct device_node *dn)
 {
 	int rc;
 
-	dn->parent = derive_parent(dn->full_name);
-	if (!dn->parent)
-		return -ENOMEM;
+	dn->parent = pseries_of_derive_parent(dn->full_name);
+	if (IS_ERR(dn->parent))
+		return PTR_ERR(dn_parent);

                                 ^
				 ?

Quoted 4 lines

There are cross compilers on kernel.org, or on Ubuntu you can just:
$ apt-get install gcc-powerpc-linux-gnu
$ make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-gcc

Thanks! I tried today and the above the only problem with the series. Would you like me to resend whole series?

Quoted 2 lines

cheers

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Michael Ellerman

reviewer

2015-10-01 02:56 UTC #7 lore raw

Re: [v2,5/5] powerpc/pseries: re-use code from of_helpers module

Quoted 15 lines · Andy Shevchenko

Quoted 14 lines · Michael Ellerman
arch/powerpc/platforms/pseries/dlpar.c cc_error: · line 255
Quoted 10 lines · Andy Shevchenko
int dlpar_attach_node(struct device_node *dn)
{
arch/powerpc/platforms/pseries/dlpar.c +3 -3
 	int rc;
 
-	dn->parent = derive_parent(dn->full_name);
-	if (!dn->parent)
-		return -ENOMEM;
+	dn->parent = pseries_of_derive_parent(dn->full_name);
+	if (IS_ERR(dn->parent))
+		return PTR_ERR(dn_parent);
                                 ^
				 ?

Quoted 7 lines

Quoted 4 lines
There are cross compilers on kernel.org, or on Ubuntu you can just:
$ apt-get install gcc-powerpc-linux-gnu
$ make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-gcc
Thanks! I tried today and the above the only problem with the series. Would you like me to resend whole series?

I'd like someone to test it. I gather that you haven't because it didn't compile.

I assume you don't have access to hardware to test it on?

cheers

Denis Kirjanov

2015-10-01 07:02 UTC #8 lore raw

Re: [v2,5/5] powerpc/pseries: re-use code from of_helpers module

Quoted 18 lines · Michael Ellerman

Quoted 17 lines · Andy Shevchenko
Quoted 16 lines · Michael Ellerman
arch/powerpc/platforms/pseries/dlpar.c cc_error: · line 255
Quoted 10 lines · Andy Shevchenko
int dlpar_attach_node(struct device_node *dn)
{
arch/powerpc/platforms/pseries/dlpar.c +3 -3
 	int rc;
 
-	dn->parent = derive_parent(dn->full_name);
-	if (!dn->parent)
-		return -ENOMEM;
+	dn->parent = pseries_of_derive_parent(dn->full_name);
+	if (IS_ERR(dn->parent))
+		return PTR_ERR(dn_parent);
                                 ^
				 ?
There are cross compilers on kernel.org, or on Ubuntu you can just:

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
$ apt-get install gcc-powerpc-linux-gnu
$ make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-gcc
Thanks! I tried today and the above the only problem with the series. Would you like me to resend whole series?
I'd like someone to test it. I gather that you haven't because it didn't compile.

Oh yeah, totally untested:

CC arch/powerpc/platforms/pseries/dlpar.o
arch/powerpc/platforms/pseries/dlpar.c: In function 'dlpar_attach_node':
arch/powerpc/platforms/pseries/dlpar.c:255:18: error: 'dn_parent' undeclared (first use in this function)

   return PTR_ERR(dn_parent);
                  ^

arch/powerpc/platforms/pseries/dlpar.c:255:18: note: each undeclared identifier is reported only once for each function it appears in scripts/Makefile.build:258: recipe for target 'arch/powerpc/platforms/pseries/dlpar.o' failed
make[2]: *** [arch/powerpc/platforms/pseries/dlpar.o] Error 1 scripts/Makefile.build:403: recipe for target 'arch/powerpc/platforms/pseries' failed
make[1]: *** [arch/powerpc/platforms/pseries] Error 2 Makefile:941: recipe for target 'arch/powerpc/platforms' failed
make: *** [arch/powerpc/platforms] Error 2

Quoted 11 lines

I assume you don't have access to hardware to test it on?
cheers
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

Andy Shevchenko

author

2015-10-01 08:39 UTC #9 lore raw

Re: [v2,5/5] powerpc/pseries: re-use code from of_helpers module

Quoted 41 lines · Denis Kirjanov

Quoted 23 lines · Michael Ellerman
Quoted 22 lines · Andy Shevchenko
Quoted 20 lines · Michael Ellerman
arch/powerpc/platforms/pseries/dlpar.c cc_error: · line 255
Quoted 11 lines · Andy Shevchenko
int dlpar_attach_node(struct device_node *dn)
{
arch/powerpc/platforms/pseries/dlpar.c +3 -4
 	int rc;
 
-	dn->parent = derive_parent(dn->full_name);
-	if (!dn->parent)
-		return -ENOMEM;
+	dn->parent = pseries_of_derive_parent(dn
->full_name);
+	if (IS_ERR(dn->parent))
+		return PTR_ERR(dn_parent);
                                 ^
				 ?
There are cross compilers on kernel.org, or on Ubuntu you can just:
$ apt-get install gcc-powerpc-linux-gnu
$ make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-gcc
Quoted 7 lines
Quoted 3 lines
Thanks! I tried today and the above the only problem with the series.
Would you like me to resend whole series?
I'd like someone to test it. I gather that you haven't because it didn't
compile.
Oh yeah, totally untested:
CC arch/powerpc/platforms/pseries/dlpar.o
arch/powerpc/platforms/pseries/dlpar.c: In function 'dlpar_attach_node':
arch/powerpc/platforms/pseries/dlpar.c:255:18: error: 'dn_parent' undeclared (first use in this function)
   return PTR_ERR(dn_parent);
                  ^

Quoted 10 lines

arch/powerpc/platforms/pseries/dlpar.c:255:18: note: each undeclared identifier is reported only once for each function it appears in scripts/Makefile.build:258: recipe for target 'arch/powerpc/platforms/pseries/dlpar.o' failed
make[2]: *** [arch/powerpc/platforms/pseries/dlpar.o] Error 1 scripts/Makefile.build:403: recipe for target 'arch/powerpc/platforms/pseries' failed
make[1]: *** [arch/powerpc/platforms/pseries] Error 2 Makefile:941: recipe for target 'arch/powerpc/platforms' failed
make: *** [arch/powerpc/platforms] Error 2

Like I mentioned earlier this is the only one issue to compile the series. Can you fix it and try on real HW?

Quoted 12 lines

Quoted 12 lines
I assume you don't have access to hardware to test it on?
cheers
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Denis Kirjanov

2015-10-01 09:32 UTC #10 lore raw

Re: [v2,5/5] powerpc/pseries: re-use code from of_helpers module

Quoted 53 lines · Andy Shevchenko

Quoted 41 lines · Denis Kirjanov
Quoted 23 lines · Michael Ellerman
Quoted 22 lines · Andy Shevchenko
Quoted 20 lines · Michael Ellerman
arch/powerpc/platforms/pseries/dlpar.c cc_error: · line 255
Quoted 11 lines · Andy Shevchenko
int dlpar_attach_node(struct device_node *dn)
{
arch/powerpc/platforms/pseries/dlpar.c +3 -4
 	int rc;
 
-	dn->parent = derive_parent(dn->full_name);
-	if (!dn->parent)
-		return -ENOMEM;
+	dn->parent = pseries_of_derive_parent(dn
->full_name);
+	if (IS_ERR(dn->parent))
+		return PTR_ERR(dn_parent);
                                 ^
				 ?
There are cross compilers on kernel.org, or on Ubuntu you can just:
$ apt-get install gcc-powerpc-linux-gnu
$ make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu-gcc
Quoted 7 lines
Quoted 3 lines
Thanks! I tried today and the above the only problem with the series.
Would you like me to resend whole series?
I'd like someone to test it. I gather that you haven't because it didn't
compile.
Oh yeah, totally untested:
CC arch/powerpc/platforms/pseries/dlpar.o
arch/powerpc/platforms/pseries/dlpar.c: In function 'dlpar_attach_node':
arch/powerpc/platforms/pseries/dlpar.c:255:18: error: 'dn_parent' undeclared (first use in this function)
   return PTR_ERR(dn_parent);
                  ^
Quoted 10 lines
arch/powerpc/platforms/pseries/dlpar.c:255:18: note: each undeclared identifier is reported only once for each function it appears in scripts/Makefile.build:258: recipe for target 'arch/powerpc/platforms/pseries/dlpar.o' failed
make[2]: *** [arch/powerpc/platforms/pseries/dlpar.o] Error 1 scripts/Makefile.build:403: recipe for target 'arch/powerpc/platforms/pseries' failed
make[1]: *** [arch/powerpc/platforms/pseries] Error 2 Makefile:941: recipe for target 'arch/powerpc/platforms' failed
make: *** [arch/powerpc/platforms] Error 2

Quoted 2 lines

Like I mentioned earlier this is the only one issue to compile the series. Can you fix it and try on real HW?

Please resend the updates series and then I'll test it. Thanks!

Quoted 18 lines

Quoted 5 lines
Quoted 5 lines
I assume you don't have access to hardware to test it on?
Quoted 8 lines
Quoted 8 lines
cheers
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev
--
Andy Shevchenko <[email protected]> Intel Finland Oy