lkml.org 
[lkml]   [2021]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code
    From
    Date


    On 2021/12/29 0:13, Borislav Petkov wrote:
    > On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
    >> Use parse_crashkernel_high_low() to bring the parsing of
    >> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
    >> are strongly dependent, make code logic clear and more readable.
    >>
    >> Suggested-by: Borislav Petkov <bp@alien8.de>
    >
    > Yeah, doesn't look like something I suggested...
    >
    >> @@ -474,10 +472,9 @@ 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) {
    >> - /* crashkernel=X,high */
    >> - ret = parse_crashkernel_high(boot_command_line, total_mem,
    >> - &crash_size, &crash_base);
    >> - if (ret != 0 || crash_size <= 0)
    >> + /* crashkernel=X,high and possible crashkernel=Y,low */
    >> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
    >
    > So this calls parse_crashkernel() and when that one fails, it calls this
    > new weird parse high/low helper you added.
    >
    > But then all three end up in the same __parse_crashkernel() worker
    > function which seems to do the actual parsing.
    >
    > What I suggested and what would be real clean is if the arches would
    > simply call a *single*
    >
    > parse_crashkernel()
    >
    > function and when that one returns, *all* crashkernel= options would
    > have been parsed properly, low, high, middle crashkernel, whatever...
    > and the caller would know what crash kernel needs to be allocated.
    >
    > Then each arch can do its memory allocations and checks based on that
    > parsed data and decide to allocate or bail.

    However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64
    will also support it. It is not supported on other architectures. So changing parse_crashkernel()
    is not appropriate unless a new function is introduced. But naming this new function isn't easy,
    and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good.
    Of course, we can also consider changing parse_crashkernel() to another name, then use
    parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause
    other architectures to change as well.

    >
    > So it is getting there but it needs more surgery...
    >
    > Thx.
    >

    --
    Regards,
    Zhen Lei

    \
     
     \ /
      Last update: 2021-12-29 03:28    [W:5.049 / U:0.616 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site