Re: [PATCH 1/3] powerpc/64: __ioremap_at clean up in the error case

[Nicholas Piggin <npiggin@gmail.com>]

Christophe Leroy's on June 11, 2019 4:28 pm:
> 
> 
> Le 10/06/2019 à 05:08, Nicholas Piggin a écrit :
>> __ioremap_at error handling is wonky, it requires caller to clean up
>> after it. Implement a helper that does the map and error cleanup and
>> remove the requirement from the caller.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> 
>> This series is a different approach to the problem, using the generic
>> ioremap_page_range directly which reduces added code, and moves
>> the radix specific code into radix files. Thanks to Christophe for
>> pointing out various problems with the previous patch.
>> 
>>   arch/powerpc/mm/pgtable_64.c | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
>> index d2d976ff8a0e..6bd3660388aa 100644
>> --- a/arch/powerpc/mm/pgtable_64.c
>> +++ b/arch/powerpc/mm/pgtable_64.c
>> @@ -108,14 +108,30 @@ unsigned long ioremap_bot;
>>   unsigned long ioremap_bot = IOREMAP_BASE;
>>   #endif
>>   
>> +static int ioremap_range(unsigned long ea, phys_addr_t pa, unsigned long size, pgprot_t prot, int nid)
>> +{
>> +	unsigned long i;
>> +
>> +	for (i = 0; i < size; i += PAGE_SIZE) {
>> +		int err = map_kernel_page(ea + i, pa + i, prot);
> 
> Missing a blank line
> 
>> +		if (err) {
> 
> I'd have done the following to reduce indentation depth
> 
> 		if (!err)
> 			continue

I'll consider it, line lengths were not too bad.

>> +			if (slab_is_available())
>> +				unmap_kernel_range(ea, size);
> 
> Shouldn't it be unmap_kernel_range(ea, i) ?

I guess (i - PAGE_SIZE really), although the old code effectively did
the full range. As a "clean up" it may be better to avoid subtle
change in behaviour and do that in another patch?

Thanks,
Nick