Re: [PATCH v5 1/4] powerpc/mm: refactor radix physical page mapping

[Reza Arbab <arbab@linux.vnet.ibm.com>]

Thanks for your review!

On Tue, Jan 17, 2017 at 12:16:35PM +0530, Balbir Singh wrote:
>On Mon, Jan 16, 2017 at 01:07:43PM -0600, Reza Arbab wrote:
>> --- a/arch/powerpc/mm/pgtable-radix.c
>> +++ b/arch/powerpc/mm/pgtable-radix.c
>> @@ -107,54 +107,66 @@ int radix__map_kernel_page(unsigned long ea, unsigned long pa,
>>  	return 0;
>>  }
>>
>> +static inline void __meminit print_mapping(unsigned long start,
>> +					   unsigned long end,
>> +					   unsigned long size)
>> +{
>> +	if (end <= start)
>> +		return;
>
>Should we pr_err for start > end?

I think that would be overkill. The way this little inline is called, 
start > end is not possible. The real point is not to print anything if 
start == end. Using <= just seemed better in context.

>> +static int __meminit create_physical_mapping(unsigned long start,
>> +					     unsigned long end)
>> +{
>> +	unsigned long addr, mapping_size;
>> +
>> +	start = _ALIGN_UP(start, PAGE_SIZE);
>> +	for (addr = start; addr < end; addr += mapping_size) {
>> +		unsigned long gap, previous_size;
>> +		int rc;
>> +
>> +		gap = end - addr;
>> +		previous_size = mapping_size;
>> +
>> +		if (IS_ALIGNED(addr, PUD_SIZE) && gap >= PUD_SIZE &&
>> +		    mmu_psize_defs[MMU_PAGE_1G].shift)
>> +			mapping_size = PUD_SIZE;
>> +		else if (IS_ALIGNED(addr, PMD_SIZE) && gap >= PMD_SIZE &&
>> +			 mmu_psize_defs[MMU_PAGE_2M].shift)
>> +			mapping_size = PMD_SIZE;
>> +		else
>> +			mapping_size = PAGE_SIZE;
>> +
>> +		if (mapping_size != previous_size) {
>> +			print_mapping(start, addr, previous_size);
>> +			start = addr;
>> +		}
>> +
>> +		rc = radix__map_kernel_page((unsigned long)__va(addr), addr,
>> +					    PAGE_KERNEL_X, mapping_size);
>> +		if (rc)
>> +			return rc;
>
>Should we try a lower page size if map_kernel_page fails for this mapping_size?

The only way map_kernel_page can fail is -ENOMEM. If that's the case,
there's no way we're going to be able to map this range at all. Better 
to fail fast here, I would think.

-- 
Reza Arbab