[PATCH 0/3] powerpc/kernel: Simplify rtas_initialize()

This simplifies rtas_initialize(), no functional changes introduced:

   * PATCH[1/3] removes the unnecessary nested if statements.
   * PATCH[2/3] uses of_property_read_u32() instead of converting the
     property's data to target in CPU endian.
   * PATCH[3/3] decreases RTAS device-tree node's refcount in error path.

Gavin Shan (3):
  powerpc/kernel: Remove nested if statements in rtas_initialize()
  powerpc/kernel: Use of_property_read_u32() in rtas_initialize()
  powerpc/kernel: Fix unbalanced refcount on RTAS device node

 arch/powerpc/kernel/rtas.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

-- 
2.7.4

This uses of_property_read_u32() in rtas_initialize() so that we
needn't explicitly care the CPU's endian.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/kernel/rtas.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 9ba0f67..d13b92d 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1145,7 +1145,8 @@ asmlinkage int ppc_rtas(struct rtas_args __user *uargs)
 void __init rtas_initialize(void)
 {
 	unsigned long rtas_region = RTAS_INSTANTIATE_MAX;
-	const __be32 *basep, *entryp, *sizep;
+	u32 base, size, entry;
+	int no_base, no_size, no_entry;
 
 	/* Get RTAS dev node and fill up our "rtas" structure with infos
 	 * about it.

@@ -1154,20 +1155,17 @@ void __init rtas_initialize(void)
 	if (!rtas.dev)
 		return;
 
-	basep = of_get_property(rtas.dev, "linux,rtas-base", NULL);
-	sizep = of_get_property(rtas.dev, "rtas-size", NULL);
-	if (basep == NULL && sizep == NULL) {
+	no_base = of_property_read_u32(rtas.dev, "linux,rtas-base", &base);
+	no_size = of_property_read_u32(rtas.dev, "rtas-size", &size);
+	if (no_base && no_size) {
 		rtas.dev = NULL;
 		return;
 	}
 
-	rtas.base = __be32_to_cpu(*basep);
-	rtas.size = __be32_to_cpu(*sizep);
-	entryp = of_get_property(rtas.dev, "linux,rtas-entry", NULL);
-	if (entryp == NULL) /* Ugh */
-		rtas.entry = rtas.base;
-	else
-		rtas.entry = __be32_to_cpu(*entryp);
+	rtas.base = base;
+	rtas.size = size;
+	no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry);
+	rtas.entry = no_entry ? rtas.base : entry;
 
 	/* If RTAS was found, allocate the RMO buffer for it and look for
 	 * the stop-self token if any

-- 
2.7.4

This removes the unnecessary nested if statements in function
rtas_initialize(), to simplify the code. No functional changes
introduced.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/kernel/rtas.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 112cc3b..9ba0f67 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1145,31 +1145,30 @@ asmlinkage int ppc_rtas(struct rtas_args __user *uargs)
 void __init rtas_initialize(void)
 {
 	unsigned long rtas_region = RTAS_INSTANTIATE_MAX;
+	const __be32 *basep, *entryp, *sizep;
 
 	/* Get RTAS dev node and fill up our "rtas" structure with infos
 	 * about it.
 	 */
 	rtas.dev = of_find_node_by_name(NULL, "rtas");
-	if (rtas.dev) {
-		const __be32 *basep, *entryp, *sizep;
-
-		basep = of_get_property(rtas.dev, "linux,rtas-base", NULL);
-		sizep = of_get_property(rtas.dev, "rtas-size", NULL);
-		if (basep != NULL && sizep != NULL) {
-			rtas.base = __be32_to_cpu(*basep);
-			rtas.size = __be32_to_cpu(*sizep);
-			entryp = of_get_property(rtas.dev,
-					"linux,rtas-entry", NULL);
-			if (entryp == NULL) /* Ugh */
-				rtas.entry = rtas.base;
-			else
-				rtas.entry = __be32_to_cpu(*entryp);
-		} else
-			rtas.dev = NULL;
-	}
 	if (!rtas.dev)
 		return;
 
+	basep = of_get_property(rtas.dev, "linux,rtas-base", NULL);
+	sizep = of_get_property(rtas.dev, "rtas-size", NULL);
+	if (basep == NULL && sizep == NULL) {
+		rtas.dev = NULL;
+		return;
+	}
+
+	rtas.base = __be32_to_cpu(*basep);
+	rtas.size = __be32_to_cpu(*sizep);
+	entryp = of_get_property(rtas.dev, "linux,rtas-entry", NULL);
+	if (entryp == NULL) /* Ugh */
+		rtas.entry = rtas.base;
+	else
+		rtas.entry = __be32_to_cpu(*entryp);
+
 	/* If RTAS was found, allocate the RMO buffer for it and look for
 	 * the stop-self token if any
 	 */

-- 
2.7.4

The RTAS device-tree node's refcount has been increased by one in
the function call of_find_node_by_name(), but it's missed to be
decreased by one in the error path. It leads to unbalanced refcount
on RTAS device-tree node.

This fixes above issue by decreasing RTAS device-tree node's refcount
in error path.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/kernel/rtas.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index d13b92d..0823fe3 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1158,6 +1158,7 @@ void __init rtas_initialize(void)
 	no_base = of_property_read_u32(rtas.dev, "linux,rtas-base", &base);
 	no_size = of_property_read_u32(rtas.dev, "rtas-size", &size);
 	if (no_base && no_size) {
+		of_node_put(rtas.dev);
 		rtas.dev = NULL;
 		return;
 	}

-- 
2.7.4

Gavin Shan [off-list ref] writes:

			...

-		} else

Previously we set rtas.dev to NULL if either basep or sizep was NULL.

But now you set it to NULL only if BOTH basep and sizep are NULL.

Was that intentional? If so you need to mention it in the change log.

The proper negation of:

	if (basep != NULL && sizep != NULL) {
is:
	if (basep == NULL || sizep == NULL) {


cheers

On Mon, Jan 23, 2017 at 08:08:20PM +1100, Michael Ellerman wrote:

Gavin Shan [off-list ref] writes:

quoted

This removes the unnecessary nested if statements in function
rtas_initialize(), to simplify the code. No functional changes
introduced.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/kernel/rtas.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 112cc3b..9ba0f67 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c

@@ -1145,31 +1145,30 @@ asmlinkage int ppc_rtas(struct rtas_args __user *uargs)
 void __init rtas_initialize(void)
 {
 	unsigned long rtas_region = RTAS_INSTANTIATE_MAX;
+	const __be32 *basep, *entryp, *sizep;
 
 	/* Get RTAS dev node and fill up our "rtas" structure with infos
 	 * about it.
 	 */
 	rtas.dev = of_find_node_by_name(NULL, "rtas");
-	if (rtas.dev) {
-		const __be32 *basep, *entryp, *sizep;
-
-		basep = of_get_property(rtas.dev, "linux,rtas-base", NULL);
-		sizep = of_get_property(rtas.dev, "rtas-size", NULL);
-		if (basep != NULL && sizep != NULL) {

		...

quoted

-		} else

Previously we set rtas.dev to NULL if either basep or sizep was NULL.

quoted

-			rtas.dev = NULL;
-	}
 	if (!rtas.dev)
 		return;
 
+	basep = of_get_property(rtas.dev, "linux,rtas-base", NULL);
+	sizep = of_get_property(rtas.dev, "rtas-size", NULL);
+	if (basep == NULL && sizep == NULL) {

But now you set it to NULL only if BOTH basep and sizep are NULL.

Was that intentional? If so you need to mention it in the change log.

quoted

+		rtas.dev = NULL;
+		return;
+	}

The proper negation of:

if (basep != NULL && sizep != NULL) {
is:
if (basep == NULL || sizep == NULL) {

Thanks, Michael. It's not intentional. I'll update in v2.

Thanks,
Gavin