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

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

Hello Michal,

On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote:
> Hello,
>
> On Wed, 03 May 2017 23:52:52 +0530
> Hari Bathini <hbathini@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@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.


Couple of things that prompted me for v5 are:
   1. Using parse_early_options() limits the kind of parameters
      that can be passed to fadump capture kernel. Passing parameters
      like systemd.unit= & udev.childern.max= has no effect with v4. 
Updating
      boot_command_line parameter, when fadump is active, seems a better
      alternative.

   2. Passing space-separated quoted arguments is not working
      as intended with lilo. Updating bootloader with the below
      entry in /etc/lilo.conf file results in a missing append
      entry in /etc/yaboot.conf file.

        append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr 
crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off 
udev.children-max=2\""

Because lilo doesn't seem to handle quoted arguments as intended,
comma-separated list syntax was the obvious choice. Quoted arguments
for a comma-separated list sounds plausible though, provided the
bootloader can handle it. Will try and address that..


> I also do not see how this addresses
>
> On Wed, 26 Apr 2017 20:32:55 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> Hari Bathini <hbathini@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.
>
>

The addtional comments Michael was asking for was when fadump_append=
is limited to early parsing only. But with this approach, we don't
have such limitation. Though I do agree that there is always scope
to improve the documentation further..

Thanks
Hari