Re: [PATCH v1 13/13] arm64: Rename arm64-internal cache maintenance functions

From: Marc Zyngier <maz@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>, Fuad Tabba <tabba@google.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>
Subject: Re: [PATCH v1 13/13] arm64: Rename arm64-internal cache maintenance functions
Date: Wed, 12 May 2021 10:51:04 +0100	[thread overview]
Message-ID: <d4dc348986083d625f20f63f037b194a@kernel.org> (raw)
In-Reply-To: <20210511154933.GF8933@C02TD0UTHF1T.local>

On 2021-05-11 16:49, Mark Rutland wrote:
> On Tue, May 11, 2021 at 05:09:18PM +0200, Ard Biesheuvel wrote:
>> On Tue, 11 May 2021 at 16:43, Fuad Tabba <tabba@google.com> wrote:
>> >
>> > Although naming across the codebase isn't that consistent, it
>> > tends to follow certain patterns. Moreover, the term "flush"
>> > isn't defined in the Arm Architecture reference manual, and might
>> > be interpreted to mean clean, invalidate, or both for a cache.
>> >
>> > Rename arm64-internal functions to make the naming internally
>> > consistent, as well as making it consistent with the Arm ARM, by
>> > clarifying whether the operation is a clean, invalidate, or both.
>> > Also specify which point the operation applies two, i.e., to the
>> > point of unification (PoU), coherence (PoC), or persistence
>> > (PoP).
>> >
>> > This commit applies the following sed transformation to all files
>> > under arch/arm64:
>> >
>> > "s/\b__flush_cache_range\b/__clean_inval_cache_pou_macro/g;"\
>> > "s/\b__flush_icache_range\b/__clean_inval_cache_pou/g;"\
> 
> For icaches, a "flush" is just an invalidate, so this doesn't need
> "clean".
> 
>> > "s/\binvalidate_icache_range\b/__inval_icache_pou/g;"\
>> > "s/\b__flush_dcache_area\b/__clean_inval_dcache_poc/g;"\
>> > "s/\b__inval_dcache_area\b/__inval_dcache_poc/g;"\
>> > "s/__clean_dcache_area_poc\b/__clean_dcache_poc/g;"\
>> > "s/\b__clean_dcache_area_pop\b/__clean_dcache_pop/g;"\
>> > "s/\b__clean_dcache_area_pou\b/__clean_dcache_pou/g;"\
>> > "s/\b__flush_cache_user_range\b/__clean_inval_cache_user_pou/g;"\
>> > "s/\b__flush_icache_all\b/__clean_inval_all_icache_pou/g;"
> 
> Likewise here.
> 
>> >
>> > Note that __clean_dcache_area_poc is deliberately missing a word
>> > boundary check to match the efistub symbols in image-vars.h.
>> >
>> > No functional change intended.
>> >
>> > Signed-off-by: Fuad Tabba <tabba@google.com>
>> 
>> I am a big fan of this change: code is so much easier to read if the
>> names of subroutines match their intent.
> 
> Likewise!
> 
>> I would suggest, though, that we get rid of all the leading
>> underscores while at it: we often use them when refactoring existing
>> routines into separate pieces (which is where at least some of these
>> came from), but here, they seem to have little value.
> 
> That all makes sense to me; I'd also suggest we make the cache type the
> prefix, e.g.
> 
> * icache_clean_pou

I guess you meant "icache_inval_pou", right, as per your comment above?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel