Subject: Re: [PATCH v2 22/25] powerpc32: move xxxxx_dcache_range() functions inline

From: Christophe Leroy <[email-masked]>
Date: Wed, 7 Oct 2015 14:49:24 +0200

Le 29/09/2015 02:29, Scott Wood a écrit :
> On Tue, Sep 22, 2015 at 06:51:13PM +0200, Christophe Leroy wrote:
>> flush/clean/invalidate _dcache_range() functions are all very
>> similar and are quite short. They are mainly used in __dma_sync()
>> perf_event locate them in the top 3 consumming functions during
>> heavy ethernet activity
>>
>> They are good candidate for inlining, as __dma_sync() does
>> almost nothing but calling them
>>
>> Signed-off-by: Christophe Leroy <[email-masked]>
>> ---
>> New in v2
>>
>>   arch/powerpc/include/asm/cacheflush.h | 55 +++++++++++++++++++++++++++--
>>   arch/powerpc/kernel/misc_32.S         | 65 -----------------------------------
>>   arch/powerpc/kernel/ppc_ksyms.c       |  2 ++
>>   3 files changed, 54 insertions(+), 68 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
>> index 6229e6b..6169604 100644
>> --- a/arch/powerpc/include/asm/cacheflush.h
>> +++ b/arch/powerpc/include/asm/cacheflush.h
>> @@ -47,12 +47,61 @@ static inline void __flush_dcache_icache_phys(unsigned long physaddr)
>>   }
>>   #endif
>>   
>> -extern void flush_dcache_range(unsigned long start, unsigned long stop);
>>   #ifdef CONFIG_PPC32
>> -extern void clean_dcache_range(unsigned long start, unsigned long stop);
>> -extern void invalidate_dcache_range(unsigned long start, unsigned long stop);
>> +/*
>> + * Write any modified data cache blocks out to memory and invalidate them.
>> + * Does not invalidate the corresponding instruction cache blocks.
>> + */
>> +static inline void flush_dcache_range(unsigned long start, unsigned long stop)
>> +{
>> +	void *addr = (void *)(start & ~(L1_CACHE_BYTES - 1));
>> +	unsigned int size = stop - (unsigned long)addr + (L1_CACHE_BYTES - 1);
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < size >> L1_CACHE_SHIFT; i++, addr += L1_CACHE_BYTES)
>> +		dcbf(addr);
>> +	if (i)
>> +		mb();	/* sync */
>> +}
> I know this is 32-bit-specific code, but it's still bad practice to use
> "unsigned int" for addresses or sizes thereof.
>
>
Ok, I can fix size, but what about start and stop ? If I change that, it 
means I also have to fix all caller. Do you expect me to do that ?

And it is very unlykely, but what if for some reason someone wants to 
invalidate the entire user address space which is 3Gbytes size ? A 
signed size would be negative here.

Christophe