Re: [RFC PATCH v5 2/9] fadump: Reserve the memory for firmware assisted dump.

["Mahesh J. Salgaonkar" <mahesh@linux.vnet.ibm.com>]

On 11/25/2011 04:32 AM, Paul Mackerras wrote:
> On Tue, Nov 15, 2011 at 08:43:43PM +0530, Mahesh J Salgaonkar wrote:
>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>
>> Reserve the memory during early boot to preserve CPU state data, HPTE region
>> and RMR region data in case of kernel crash. At the time of crash, powerpc
>> firmware will store CPU state data, HPTE region data and move RMR region
>> data to the reserved memory area.
> 
> What is "RMR"?  I don't see anywhere that you explain this acronym.
> Is it the same as the RMA (real mode area)?
> 

Yes, it is the same as the RMA. I think I will replace all RMR/RMO with RMA.

>> +config FA_DUMP
>> +	bool "Firmware-assisted dump"
> 
> Is this new fadump infrastructure intended to supersede the existing
> phyp dump code?  Does it use the same phyp interfaces as phyp dump?
> If so, you should probably remove the phyp dump code and config option
> as the final patch in your series.
> 
>> +/*
>> + * The RMR region will be saved for later dumping when kernel crashes.
>> + * Set this to RMO size.
>> + */
>> +#define RMR_START	0x0
>> +#define RMR_END		(ppc64_rma_size)
> 
> An explanation of "RMR" here, and what the distinction (if any)
> between RMR and RMA/RMO is, would help future readers.
> 

Will change this to RMA_START and RMA_END

>> +	sections = of_get_flat_dt_prop(node, "ibm,configure-kernel-dump-sizes",
>> +					&size);
>> +
>> +	if (!sections)
>> +		return 0;
>> +
>> +	num_sections = size / sizeof(struct dump_section);
>> +
>> +	for (i = 0; i < num_sections; i++) {
>> +		switch (sections[i].dump_section) {
>> +		case FADUMP_CPU_STATE_DATA:
>> +			fw_dump.cpu_state_data_size = sections[i].section_size;
>> +			break;
>> +		case FADUMP_HPTE_REGION:
>> +			fw_dump.hpte_region_size = sections[i].section_size;
>> +			break;
> 
> It's generally better to use of_read_number() or of_read_ulong() to
> parse OF properties, rather than using a structure like this.
> 
>> +	/* divide by 20 to get 5% of value */
>> +	size = memblock_end_of_DRAM();
>> +	do_div(size, 20);
> 
> You could just say size = memblock_end_of_DRAM() / 20 here; no need to
> use do_div, since we won't be using this code on 32-bit platforms.
> 
>> +	if (!fw_dump.fadump_supported) {
>> +		printk(KERN_ERR "Firmware-assisted dump is not supported on"
>> +				" this hardware\n");
> 
> This shouldn't be KERN_ERR; it's not an error to boot a kernel with
> fadump configured in on a machine that doesn't have firmware fadump
> support.  I don't think we really need any message, but if we have one
> it should be KERN_INFO at most.
> 
>> +/* Look for fadump= cmdline option. */
>> +static int __init early_fadump_param(char *p)
>> +{
>> +	if (!p)
>> +		return 1;
>> +
>> +	if (p[0] == '1')
>> +		fw_dump.fadump_enabled = 1;
>> +	else if (p[0] == '0')
>> +		fw_dump.fadump_enabled = 0;
> 
> I think it's usual to allow "on" and "off" as values for this kind of
> option.  There might be a handy little helper function to parse this
> sort of thing (but if there is I don't know what it is called).

Will rework on your suggestions. Thanks for the review.

Thanks,
-Mahesh.