[PATCH] powerpc: Use common error handling code in setup_new_fdt()

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c
index e4395f937d63..90c6004c2eec 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c

@@ -302,18 +302,14 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
 		ret = fdt_setprop_u64(fdt, chosen_node,
 				      "linux,initrd-start",
 				      initrd_load_addr);
-		if (ret < 0) {
-			pr_err("Error setting up the new device tree.\n");
-			return -EINVAL;
-		}
+		if (ret < 0)
+			goto report_setup_failure;
 
 		/* initrd-end is the first address after the initrd image. */
 		ret = fdt_setprop_u64(fdt, chosen_node, "linux,initrd-end",
 				      initrd_load_addr + initrd_len);
-		if (ret < 0) {
-			pr_err("Error setting up the new device tree.\n");
-			return -EINVAL;
-		}
+		if (ret < 0)
+			goto report_setup_failure;
 
 		ret = fdt_add_mem_rsv(fdt, initrd_load_addr, initrd_len);
 		if (ret) {

@@ -325,10 +321,8 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
 
 	if (cmdline != NULL) {
 		ret = fdt_setprop_string(fdt, chosen_node, "bootargs", cmdline);
-		if (ret < 0) {
-			pr_err("Error setting up the new device tree.\n");
-			return -EINVAL;
-		}
+		if (ret < 0)
+			goto report_setup_failure;
 	} else {
 		ret = fdt_delprop(fdt, chosen_node, "bootargs");
 		if (ret && ret != -FDT_ERR_NOTFOUND) {

@@ -344,10 +338,12 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
 	}
 
 	ret = fdt_setprop(fdt, chosen_node, "linux,booted-from-kexec", NULL, 0);
-	if (ret) {
-		pr_err("Error setting up the new device tree.\n");
-		return -EINVAL;
-	}
+	if (ret)
+		goto report_setup_failure;
 
 	return 0;
+
+report_setup_failure:
+	pr_err("Error setting up the new device tree.\n");
+	return -EINVAL;
 }

-- 
2.16.2

SF Markus Elfring [off-list ref] writes:

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote:

SF Markus Elfring [off-list ref] writes:

quoted

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

You know that compilers already re-use string constants so this doesn't
actually save memory?  Also we should be preserving the error codes
instead of always returning -EINVAL.

regards,
dan carpenter

On Thu, 2018-03-15 at 14:57 +0300, Dan Carpenter wrote:

On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote:

quoted

SF Markus Elfring [off-list ref] writes:

quoted

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

You know that compilers already re-use string constants so this doesn't
actually save memory?

And modern compilers create their own jump labels
so this doesn't change object code either?

Joe Perches [off-list ref] writes:

On Thu, 2018-03-15 at 14:57 +0300, Dan Carpenter wrote:

quoted

On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote:

quoted

SF Markus Elfring [off-list ref] writes:

quoted

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

You know that compilers already re-use string constants so this doesn't
actually save memory?

And modern compilers create their own jump labels
so this doesn't change object code either?

IMHO it's an improvement to the source code itself. I wasn't thinking
about the object file.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Dan Carpenter [off-list ref] writes:

On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote:

quoted

SF Markus Elfring [off-list ref] writes:

quoted

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

You know that compilers already re-use string constants so this doesn't
actually save memory?

Sure, but it's still clearer to only have the string appear once in the
code.

Also we should be preserving the error codes
instead of always returning -EINVAL.

The error codes come from libfdt code, so they don't necessarily make
sense in the kernel. eg. FDT_ERR_NOSPACE == 3 == ESRCH.

Perhaps we should be trying harder to convert them, but that's a
criticism of the original code not this patch.

cheers

Joe Perches [off-list ref] writes:

On Thu, 2018-03-15 at 14:57 +0300, Dan Carpenter wrote:

quoted

On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote:

quoted

SF Markus Elfring [off-list ref] writes:

quoted

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

You know that compilers already re-use string constants so this doesn't
actually save memory?

And modern compilers create their own jump labels
so this doesn't change object code either?

I must have missed the memo about us only changing source code if it
results in better object code.

cheers

On Fri, Mar 16, 2018 at 09:26:53PM +1100, Michael Ellerman wrote:

Dan Carpenter [off-list ref] writes:

quoted

On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote:

quoted

SF Markus Elfring [off-list ref] writes:

quoted

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
---
 arch/powerpc/kernel/machine_kexec_file_64.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

I liked it. Thanks!

Reviewed-by: Thiago Jung Bauermann <redacted>

You know that compilers already re-use string constants so this doesn't
actually save memory?

Sure, but it's still clearer to only have the string appear once in the
code.

To me the original was better.

quoted

Also we should be preserving the error codes
instead of always returning -EINVAL.

The error codes come from libfdt code, so they don't necessarily make
sense in the kernel. eg. FDT_ERR_NOSPACE == 3 == ESRCH.

Perhaps we should be trying harder to convert them, but that's a
criticism of the original code not this patch.

Ah.  You're right.  I look at the patch in context, sorry.

regards,
dan carpenter

On Sun, 2018-03-11 at 08:16:47 UTC, SF Markus Elfring wrote:

From: Markus Elfring <redacted>
Date: Sun, 11 Mar 2018 09:03:42 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <redacted>
Reviewed-by: Thiago Jung Bauermann <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/baedcdf5054c151a33e34392af7d8c

cheers