Re: [PATCH] arch/x86/boot/memory.c: Touched up coding-style issues
* 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.
>
> Signed-off-by: Jordan Borgner
> ---
> 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
Re: [PATCH] arch/x86/boot/memory.c: Touched up coding-style issues
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_ 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.
[PATCH] arch/x86/boot/memory.c: Touched up coding-style issues
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
---
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
