Subject: Re: [PATCH 7/8] pseries: phyp dump: Unregister and print dump areas.

From: Manish Ahuja <[email-masked]>
Date: Tue, 08 Jan 2008 16:56:45 -0600

Stephen,

>> +	/* Add addr value if not initialized before */
>> +	if (ph->cpu_data.destination_address == 0) {
>> +		ph->cpu_data.destination_address += addr;
> 
> Could be just '=' like further down, right?

Actually the one below should be += as well. Thanks for catching it.

>> +	/* total reserved size - start of scratch area */
>> +	second_addr_range = phdr.cpu_data.destination_address -
>> +				phyp_dump_info->init_reserve_size;
>> +	return sprintf(buf, "CPU:0x%lx-0x%lx: HPTE:0x%lx-0x%lx:"
>> +			    " DUMP:0x%lx-0x%lx, 0x%lx-0x%lx:\n",
>> +	  phdr.cpu_data.destination_address, phdr.cpu_data.length_copied,
>> +	  phdr.hpte_data.destination_address, phdr.hpte_data.length_copied,
>> +	  phdr.kernel_data.destination_address, phdr.kernel_data.length_copied,
>> +	  phyp_dump_info->init_reserve_start, second_addr_range);
> 
> This indentation should be (probably) two tabs.

I kept it one with a few spaces as otherwise it was exceeding 80, I guess, I can just have one per line
and that should take care of that.

> 
>> +	/* re-register the dump area, if old dump was invalid */
>> +	if ((dump_header) && (dump_header->status & DUMP_ERROR_FLAG)) {
>             ^           ^
> Extra parentheses.

Just for clarity.. I would prefer that, if thats okay.

> 
>> +		invalidate_last_dump (&phdr, dump_area_start);
>> +		register_dump_area (&phdr, dump_area_start);
> 
> No spaces after function names.
> 

Yeah, will take that out from here and other files as well.

Thanks,
Manish