| Message ID | 1555660717-18731-1-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kernel/crash: make parse_crashkernel()'s return value more indicant | expand |
On Fri, 19 Apr 2019, Pingfan Liu wrote: > At present, both return and crash_size should be checked to guarantee the > success of parse_crashkernel(). > Simplify the way by returning negative if fail, positive if success. In > case of failure, -EINVAL for bad syntax, -1 for the parsing results in > crash_size=0. I'm not entirely sure what you are trying to say here, but '-1' is not an improvement at all. We surely are not short of proper error codes, right? Also I don't see any positive return value > 0. So what is this about: > --- a/arch/ia64/kernel/setup.c > +++ b/arch/ia64/kernel/setup.c > @@ -277,7 +277,7 @@ static void __init setup_crashkernel(unsigned long total, int *n) > > ret = parse_crashkernel(boot_command_line, total, > &size, &base); > - if (ret == 0 && size > 0) { > + if (ret >= 0) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ???? > if (!memory_region_available(crash_base, crash_size)) { > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index 45a8d0b..0b626e2 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -376,7 +376,7 @@ static inline unsigned long fadump_calculate_reserve_size(void) > */ > ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), > &size, &base); > - if (ret == 0 && size > 0) { > + if (ret >= 0) { and this ? > unsigned long max_size; > > if (fw_dump.reserve_bootvar) > diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c > index 63f5a93..9f3e61a 100644 > --- a/arch/powerpc/kernel/machine_kexec.c > +++ b/arch/powerpc/kernel/machine_kexec.c > @@ -122,7 +122,7 @@ void __init reserve_crashkernel(void) > /* use common parsing */ > ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), > &crash_size, &crash_base); > - if (ret == 0 && crash_size > 0) { > + if (ret >= 0) { Again. > crashk_res.start = crash_base; > crashk_res.end = crash_base + crash_size - 1; > } > --- a/arch/sh/kernel/machine_kexec.c > +++ b/arch/sh/kernel/machine_kexec.c > @@ -157,7 +157,7 @@ void __init reserve_crashkernel(void) > > ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), > &crash_size, &crash_base); > - if (ret == 0 && crash_size > 0) { > + if (ret >= 0) { And some more. > crashk_res.start = crash_base; > crashk_res.end = crash_base + crash_size - 1; > } > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 3d872a5..62d07d4 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -526,11 +526,11 @@ static void __init reserve_crashkernel(void) > > /* crashkernel=XM */ > ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); > - if (ret != 0 || crash_size <= 0) { > + if (ret == -EINVAL) { Without an explanation why this proceedes on error codes other than EINVAL this is uncomprehensible. Comments exist for a reason. > /* crashkernel=X,high */ > ret = parse_crashkernel_high(boot_command_line, total_mem, > &crash_size, &crash_base); > - if (ret != 0 || crash_size <= 0) > + if (ret < 0) > return; > high = true; > @@ -87,7 +87,7 @@ static int __init parse_crashkernel_mem(char *cmdline, > cur = tmp; > if (size >= system_ram) { > pr_warn("crashkernel: invalid size\n"); > - return -EINVAL; > + return -1; Well, this is incomprehensible as well. The pr_warn() says invalid and then you change the error code to something magic. Thanks, tglx
On Fri, Apr 19, 2019 at 4:19 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Fri, 19 Apr 2019, Pingfan Liu wrote: > > > At present, both return and crash_size should be checked to guarantee the > > success of parse_crashkernel(). > > Simplify the way by returning negative if fail, positive if success. In > > case of failure, -EINVAL for bad syntax, -1 for the parsing results in > > crash_size=0. > > I'm not entirely sure what you are trying to say here, but '-1' is not an > improvement at all. We surely are not short of proper error codes, right? > The different negative return values are only used by x86. The option "crashkernel=X,high", which is only used on x86, causes parse_kernel() to return -EINVAL, then let parse_crashkernel_high() have a try. When parsing crashkernel=size@offset and crashkernel=range1:size1, there are other cases of failure, which is not worth to call parse_crashkernel_high() to have a try. That is "-1" aiming for. First, in parse_crashkernel_mem(), if demanded size is bigger than system ram, this one looks like -ENOMEM, but -ENOMEM normally is used for allocation. Second, in parse_crashkernel_mem(), if system ram is not inside the range listed by "crashkernel=". Third, crashkernel=0MB is given in the option (not in practice, but can not forbid user to do so). All of these cases can be treated as -EINVAL, but hard to define the error codes. > Also I don't see any positive return value > 0. So what is this about: > Yes. 0 is enough for success. I had thought about returning 1 if @offset is specified in crashkernel. But at present, no use case for it. > > --- a/arch/ia64/kernel/setup.c > > +++ b/arch/ia64/kernel/setup.c > > @@ -277,7 +277,7 @@ static void __init setup_crashkernel(unsigned long total, int *n) > > > > ret = parse_crashkernel(boot_command_line, total, > > &size, &base); > > - if (ret == 0 && size > 0) { > > + if (ret >= 0) { > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ???? > > > if (!memory_region_available(crash_base, crash_size)) { > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > > index 45a8d0b..0b626e2 100644 > > --- a/arch/powerpc/kernel/fadump.c > > +++ b/arch/powerpc/kernel/fadump.c > > @@ -376,7 +376,7 @@ static inline unsigned long fadump_calculate_reserve_size(void) > > */ > > ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), > > &size, &base); > > - if (ret == 0 && size > 0) { > > + if (ret >= 0) { > > and this ? > > > unsigned long max_size; > > > > if (fw_dump.reserve_bootvar) > > diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c > > index 63f5a93..9f3e61a 100644 > > --- a/arch/powerpc/kernel/machine_kexec.c > > +++ b/arch/powerpc/kernel/machine_kexec.c > > @@ -122,7 +122,7 @@ void __init reserve_crashkernel(void) > > /* use common parsing */ > > ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), > > &crash_size, &crash_base); > > - if (ret == 0 && crash_size > 0) { > > + if (ret >= 0) { > > Again. > > > crashk_res.start = crash_base; > > crashk_res.end = crash_base + crash_size - 1; > > } > > --- a/arch/sh/kernel/machine_kexec.c > > +++ b/arch/sh/kernel/machine_kexec.c > > @@ -157,7 +157,7 @@ void __init reserve_crashkernel(void) > > > > ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), > > &crash_size, &crash_base); > > - if (ret == 0 && crash_size > 0) { > > + if (ret >= 0) { > > And some more. > > > crashk_res.start = crash_base; > > crashk_res.end = crash_base + crash_size - 1; > > } > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > > index 3d872a5..62d07d4 100644 > > --- a/arch/x86/kernel/setup.c > > +++ b/arch/x86/kernel/setup.c > > @@ -526,11 +526,11 @@ static void __init reserve_crashkernel(void) > > > > /* crashkernel=XM */ > > ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); > > - if (ret != 0 || crash_size <= 0) { > > + if (ret == -EINVAL) { > > Without an explanation why this proceedes on error codes other than EINVAL > this is uncomprehensible. Comments exist for a reason. > As explained above, deciding whether to let parse_crashkernel_high() try. > > /* crashkernel=X,high */ > > ret = parse_crashkernel_high(boot_command_line, total_mem, > > &crash_size, &crash_base); > > - if (ret != 0 || crash_size <= 0) > > + if (ret < 0) > > return; > > high = true; > > > @@ -87,7 +87,7 @@ static int __init parse_crashkernel_mem(char *cmdline, > > cur = tmp; > > if (size >= system_ram) { > > pr_warn("crashkernel: invalid size\n"); > > - return -EINVAL; > > + return -1; > > Well, this is incomprehensible as well. The pr_warn() says invalid and then > you change the error code to something magic. > As explained above, want to know whether worth to let parse_crashkernel_high() try. What about the following alternative method? Treating crash_size=0 as -EINVAL. Then on x86, just call parse_crashkernel_high() blindly to have a try. Thanks for your review. Regards, Pingfan
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 5d78b6a..2feab13 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -997,7 +997,7 @@ static void __init reserve_crashkernel(void) total_mem = get_total_mem(); ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); - if (ret) + if (ret < 0) return; if (crash_base <= 0) { diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 6bc1350..240918c 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -79,7 +79,7 @@ static void __init reserve_crashkernel(void) ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), &crash_size, &crash_base); /* no crashkernel= or invalid value specified */ - if (ret || !crash_size) + if (ret < 0) return; crash_size = PAGE_ALIGN(crash_size); diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c index 583a374..99cae33 100644 --- a/arch/ia64/kernel/setup.c +++ b/arch/ia64/kernel/setup.c @@ -277,7 +277,7 @@ static void __init setup_crashkernel(unsigned long total, int *n) ret = parse_crashkernel(boot_command_line, total, &size, &base); - if (ret == 0 && size > 0) { + if (ret >= 0) { if (!base) { sort_regions(rsvd_region, *n); *n = merge_regions(rsvd_region, *n); diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c index 8d1dc6c..168571b 100644 --- a/arch/mips/kernel/setup.c +++ b/arch/mips/kernel/setup.c @@ -715,7 +715,7 @@ static void __init mips_parse_crashkernel(void) total_mem = get_total_mem(); ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); - if (ret != 0 || crash_size <= 0) + if (ret < 0) return; if (!memory_region_available(crash_base, crash_size)) { diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index 45a8d0b..0b626e2 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -376,7 +376,7 @@ static inline unsigned long fadump_calculate_reserve_size(void) */ ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), &size, &base); - if (ret == 0 && size > 0) { + if (ret >= 0) { unsigned long max_size; if (fw_dump.reserve_bootvar) diff --git a/arch/powerpc/kernel/machine_kexec.c b/arch/powerpc/kernel/machine_kexec.c index 63f5a93..9f3e61a 100644 --- a/arch/powerpc/kernel/machine_kexec.c +++ b/arch/powerpc/kernel/machine_kexec.c @@ -122,7 +122,7 @@ void __init reserve_crashkernel(void) /* use common parsing */ ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), &crash_size, &crash_base); - if (ret == 0 && crash_size > 0) { + if (ret >= 0) { crashk_res.start = crash_base; crashk_res.end = crash_base + crash_size - 1; } diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c index 2c642af..d4bd61b 100644 --- a/arch/s390/kernel/setup.c +++ b/arch/s390/kernel/setup.c @@ -671,7 +671,7 @@ static void __init reserve_crashkernel(void) crash_base = ALIGN(crash_base, KEXEC_CRASH_MEM_ALIGN); crash_size = ALIGN(crash_size, KEXEC_CRASH_MEM_ALIGN); - if (rc || crash_size == 0) + if (rc < 0) return; if (memblock.memory.regions[0].size < crash_size) { diff --git a/arch/sh/kernel/machine_kexec.c b/arch/sh/kernel/machine_kexec.c index 63d63a3..612b21e 100644 --- a/arch/sh/kernel/machine_kexec.c +++ b/arch/sh/kernel/machine_kexec.c @@ -157,7 +157,7 @@ void __init reserve_crashkernel(void) ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(), &crash_size, &crash_base); - if (ret == 0 && crash_size > 0) { + if (ret >= 0) { crashk_res.start = crash_base; crashk_res.end = crash_base + crash_size - 1; } diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 3d872a5..62d07d4 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -526,11 +526,11 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); - if (ret != 0 || crash_size <= 0) { + if (ret == -EINVAL) { /* crashkernel=X,high */ ret = parse_crashkernel_high(boot_command_line, total_mem, &crash_size, &crash_base); - if (ret != 0 || crash_size <= 0) + if (ret < 0) return; high = true; } diff --git a/kernel/crash_core.c b/kernel/crash_core.c index 093c9f9..563d86d 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -87,7 +87,7 @@ static int __init parse_crashkernel_mem(char *cmdline, cur = tmp; if (size >= system_ram) { pr_warn("crashkernel: invalid size\n"); - return -EINVAL; + return -1; } /* match ? */ @@ -108,8 +108,10 @@ static int __init parse_crashkernel_mem(char *cmdline, return -EINVAL; } } - } else + } else { pr_info("crashkernel size resulted in zero bytes\n"); + return -1; + } return 0; } @@ -139,6 +141,8 @@ static int __init parse_crashkernel_simple(char *cmdline, pr_warn("crashkernel: unrecognized char: %c\n", *cur); return -EINVAL; } + if (*crash_size == 0) + return -1; return 0; } @@ -181,6 +185,8 @@ static int __init parse_crashkernel_suffix(char *cmdline, pr_warn("crashkernel: unrecognized char: %c\n", *cur); return -EINVAL; } + if (*crash_size == 0) + return -1; return 0; } @@ -266,6 +272,8 @@ static int __init __parse_crashkernel(char *cmdline, /* * That function is the entry point for command line parsing and should be * called from the arch-specific code. + * On success 0 or 1 if @offset is given. On error -EINVAL if bad syntax, + * or -1 if the parsing results in crash_size=0. */ int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
At present, both return and crash_size should be checked to guarantee the success of parse_crashkernel(). Simplify the way by returning negative if fail, positive if success. In case of failure, -EINVAL for bad syntax, -1 for the parsing results in crash_size=0. Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Paul Burton <paul.burton@mips.com> Cc: James Hogan <jhogan@kernel.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com> Cc: Heiko Carstens <heiko.carstens@de.ibm.com> Cc: Yoshinori Sato <ysato@users.sourceforge.jp> Cc: Rich Felker <dalias@libc.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Julien Thierry <julien.thierry@arm.com> Cc: Palmer Dabbelt <palmer@sifive.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Logan Gunthorpe <logang@deltatee.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Greg Hackmann <ghackmann@android.com> Cc: Stefan Agner <stefan@agner.ch> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: David Hildenbrand <david@redhat.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: Thomas Bogendoerfer <tbogendoerfer@suse.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Hari Bathini <hbathini@linux.ibm.com> Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com> Cc: Yangtao Li <tiny.windzz@gmail.com> Cc: Dave Young <dyoung@redhat.com> Cc: Baoquan He <bhe@redhat.com> Cc: x86@kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-ia64@vger.kernel.org Cc: linux-mips@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-s390@vger.kernel.org Cc: linux-sh@vger.kernel.org --- arch/arm/kernel/setup.c | 2 +- arch/arm64/mm/init.c | 2 +- arch/ia64/kernel/setup.c | 2 +- arch/mips/kernel/setup.c | 2 +- arch/powerpc/kernel/fadump.c | 2 +- arch/powerpc/kernel/machine_kexec.c | 2 +- arch/s390/kernel/setup.c | 2 +- arch/sh/kernel/machine_kexec.c | 2 +- arch/x86/kernel/setup.c | 4 ++-- kernel/crash_core.c | 12 ++++++++++-- 10 files changed, 20 insertions(+), 12 deletions(-)