[PATCH] arch/x86/boot/memory.c: Touched up coding-style issues

[PATCH] arch/x86/boot/memory.c: Touched up coding-style issues

[Jordan Borgner]

[-- Attachment #1: 0001-arch-x86-boot-memory.c-Fixed-coding-style-issues.patch --]
[-- Type: text/x-diff, Size: 1769 bytes --]

Added missing parentheses to sizeof() function in detect_memory_e820().

Removed unnecessary braces in detect_memory_e801().

Replaced three if-statements with a ternary if-statement and 
removed an unnecessary integer variable in detect_memory().

This is my first patch I hope it is okay.

Signed-off-by: Jordan Borgner <mail@jordan-borgner.de>
---
 linux-4.19/arch/x86/boot/memory.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/linux-4.19/arch/x86/boot/memory.c b/linux-4.19/arch/x86/boot/memory.c
index d9c28c8..a6124af 100644
--- a/linux-4.19/arch/x86/boot/memory.c
+++ b/linux-4.19/arch/x86/boot/memory.c
@@ -26,7 +26,7 @@ static int detect_memory_e820(void)
 
 	initregs(&ireg);
 	ireg.ax  = 0xe820;
-	ireg.cx  = sizeof buf;
+	ireg.cx  = sizeof(buf);
 	ireg.edx = SMAP;
 	ireg.di  = (size_t)&buf;
 
@@ -88,11 +88,11 @@ static int detect_memory_e801(void)
 		oreg.bx = oreg.dx;
 	}
 
-	if (oreg.ax > 15*1024) {
+	if (oreg.ax > 15*1024)
 		return -1;	/* Bogus! */
-	} else if (oreg.ax == 15*1024) {
+	else if (oreg.ax == 15*1024)
 		boot_params.alt_mem_k = (oreg.bx << 6) + oreg.ax;
-	} else {
+	else
 		/*
 		 * This ignores memory above 16MB if we have a memory
 		 * hole there.  If someone actually finds a machine
@@ -101,7 +101,6 @@ static int detect_memory_e801(void)
 		 * map.
 		 */
 		boot_params.alt_mem_k = oreg.ax;
-	}
 
 	return 0;
 }
@@ -121,16 +120,7 @@ static int detect_memory_88(void)
 
 int detect_memory(void)
 {
-	int err = -1;
-
-	if (detect_memory_e820() > 0)
-		err = 0;
-
-	if (!detect_memory_e801())
-		err = 0;
-
-	if (!detect_memory_88())
-		err = 0;
-
-	return err;
+	return (detect_memory_e820() > 0 ||
+		!detect_memory_e801()    ||
+		!detect_memory_88()) ? 0 : -1;
 }
-- 
2.11.0

Re: [PATCH] arch/x86/boot/memory.c: Touched up coding-style issues

[Joe Perches]

On Sat, 2018-10-27 at 23:32 +0100, Jordan Borgner wrote:
> Added missing parentheses to sizeof() function in detect_memory_e820().
> 
> Removed unnecessary braces in detect_memory_e801().
> 
> Replaced three if-statements with a ternary if-statement and 
> removed an unnecessary integer variable in detect_memory().
> 
> This is my first patch I hope it is okay.

Hello Jordan.

While the first bit is generally OK, given some individual
maintainer preferences, it may not also be applied.

Whitespace only changes without logical fixes/upgrades are
generally discouraged outside of drivers/staging.

This bit below though changes behavior.

> @@ -121,16 +120,7 @@ static int detect_memory_88(void)
>  
>  int detect_memory(void)
>  {
> -       int err = -1;
> -
> -       if (detect_memory_e820() > 0)
> -               err = 0;
> -
> -       if (!detect_memory_e801())
> -               err = 0;
> -
> -       if (!detect_memory_88())
> -               err = 0;
> -
> -       return err;
> +       return (detect_memory_e820() > 0 ||
> +               !detect_memory_e801()    ||
> +               !detect_memory_88()) ? 0 : -1;
>  }


For instance:

If the first detect_memory_e820 > 0 is true,
the other detect_memory_<e801|88> calls will
not be done.

The original will always perform all three tests.


Regardless, please try to make your first patches
to the linux kernel somewhere in drivers/staging
so get comfortable with the general mechanisms and
styles of kernel patching.

Re: [PATCH] arch/x86/boot/memory.c: Touched up coding-style issues

[Ingo Molnar]

* Jordan Borgner <mail@jordan-borgner.de> wrote:

> Added missing parentheses to sizeof() function in detect_memory_e820().
> 
> Removed unnecessary braces in detect_memory_e801().
> 
> Replaced three if-statements with a ternary if-statement and 
> removed an unnecessary integer variable in detect_memory().
> 
> This is my first patch I hope it is okay.
> 
> Signed-off-by: Jordan Borgner <mail@jordan-borgner.de>
> ---
>  linux-4.19/arch/x86/boot/memory.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/linux-4.19/arch/x86/boot/memory.c b/linux-4.19/arch/x86/boot/memory.c
> index d9c28c8..a6124af 100644
> --- a/linux-4.19/arch/x86/boot/memory.c
> +++ b/linux-4.19/arch/x86/boot/memory.c
> @@ -26,7 +26,7 @@ static int detect_memory_e820(void)
>  
>  	initregs(&ireg);
>  	ireg.ax  = 0xe820;
> -	ireg.cx  = sizeof buf;
> +	ireg.cx  = sizeof(buf);
>  	ireg.edx = SMAP;
>  	ireg.di  = (size_t)&buf;

That's legit - could you do a single patch that only changes all the 
'sizeof x' patterns in arch/x86/ to 'sizeof(x)']?

>  
> @@ -88,11 +88,11 @@ static int detect_memory_e801(void)
>  		oreg.bx = oreg.dx;
>  	}
>  
> -	if (oreg.ax > 15*1024) {
> +	if (oreg.ax > 15*1024)
>  		return -1;	/* Bogus! */
> -	} else if (oreg.ax == 15*1024) {
> +	else if (oreg.ax == 15*1024)
>  		boot_params.alt_mem_k = (oreg.bx << 6) + oreg.ax;
> -	} else {
> +	else
>  		/*
>  		 * This ignores memory above 16MB if we have a memory
>  		 * hole there.  If someone actually finds a machine
> @@ -101,7 +101,6 @@ static int detect_memory_e801(void)
>  		 * map.
>  		 */
>  		boot_params.alt_mem_k = oreg.ax;
> -	}

The original code was better - multi-screen-line statements require curly 
braces in general.

>  
>  	return 0;
>  }
> @@ -121,16 +120,7 @@ static int detect_memory_88(void)
>  
>  int detect_memory(void)
>  {
> -	int err = -1;
> -
> -	if (detect_memory_e820() > 0)
> -		err = 0;
> -
> -	if (!detect_memory_e801())
> -		err = 0;
> -
> -	if (!detect_memory_88())
> -		err = 0;
> -
> -	return err;
> +	return (detect_memory_e820() > 0 ||
> +		!detect_memory_e801()    ||
> +		!detect_memory_88()) ? 0 : -1;

Here too I think the original flow of logic was easier to read - more 
compact is not always better.

Also, please investigate whether the return value is actually *used*, and 
if not then please send a separate patch that simplifies the code.

Thanks,

	Ingo