[PATCH v5 2/2] powerpc/fadump: update documentation about 'fadump_append=' parameter

[Michal Suchánek]

Hello,

On Wed, 03 May 2017 23:52:52 +0530
Hari Bathini <hbathini at linux.vnet.ibm.com> wrote:

> With the introduction of 'fadump_append=' parameter to pass additional
> parameters to fadump (capture) kernel, update documentation about it.
> 
> Signed-off-by: Hari Bathini <hbathini at linux.vnet.ibm.com>
> ---
> 
> Changes from v4:
> * Based on top of patchset that includes
>   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f
> 
> 
>  Documentation/powerpc/firmware-assisted-dump.txt |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/powerpc/firmware-assisted-dump.txt
> b/Documentation/powerpc/firmware-assisted-dump.txt index
> 8394bc8..6327193 100644 ---
> a/Documentation/powerpc/firmware-assisted-dump.txt +++
> b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7 +162,15
> @@ How to enable firmware-assisted dump (fadump): 
>  1. Set config option CONFIG_FA_DUMP=y and build kernel.
>  2. Boot into linux kernel with 'fadump=on' kernel cmdline option.
> -3. Optionally, user can also set 'crashkernel=' kernel cmdline
> +3. A user can pass additional command line parameters as a comma
> +   separated list through 'fadump_append=' parameter, to be enforced
> +   when fadump is active. For example, if parameters like nr_cpus=1,
> +   numa=off & udev.children-max=2 are to be enforced when fadump is
> +   active, 'fadump_append=nr_cpus=1,numa=off,udev.children-max=2'
> +   can be passed in command line, which will be replaced with
> +   "nr_cpus=1 numa=off udev.children-max=2" when fadump is active.
> +   This helps in reducing memory consumption during dump capture.
> +4. Optionally, user can also set 'crashkernel=' kernel cmdline
>     to specify size of the memory to reserve for boot memory dump
>     preservation.
>  
> 

Writing your own deficient parser for comma separated arguments when
perfectly fine parser for space separated quoted arguments exists in
the kernel and the bootloader does not seem like a good idea to me.

I also do not see how this addresses 

On Wed, 26 Apr 2017 20:32:55 +1000
Michael Ellerman <mpe at ellerman.id.au> wrote:

> Hari Bathini <hbathini at linux.vnet.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/kernel/fadump.c
> > b/arch/powerpc/kernel/fadump.c index 8ff0dd4..87edc7b 100644
> > --- a/arch/powerpc/kernel/fadump.c
> > +++ b/arch/powerpc/kernel/fadump.c
> > @@ -338,6 +343,36 @@ unsigned long __init
> > arch_reserved_kernel_pages(void) return memblock_reserved_size() /
> > PAGE_SIZE; }
> >  
> > +static void __init parse_fadump_append_params(const char *p)
> > +{
> > +	static char fadump_cmdline[COMMAND_LINE_SIZE] __initdata;
> > +	char *token;
> > +
> > +	strlcpy(fadump_cmdline, p, COMMAND_LINE_SIZE);
> > +	token = strchr(fadump_cmdline, ',');
> > +	while (token) {
> > +		*token = ' ';
> > +		token = strchr(token, ',');
> > +	}
> > +
> > +	pr_info("enforcing additional parameters (%s) passed
> > through "
> > +		"'fadump_append=' parameter\n", fadump_cmdline);
> > +	parse_early_options(fadump_cmdline);  
> 
> Using parse_early_options() means it only works for parameters defined
> with early_param() or __setup() doesn't it?
> 
> That might be OK but at least you need to clearly document it.

which was your gripe with v4 of the patchset. Unfortunately the flags
in Documentation/kernel-parameters.txt do not include a flag that
denotes parameters available with parse_early_options - unless it is
the KNL flag and is quite bitrotten.

Thanks

Michal