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

From: Mark Rutland <mark.rutland@arm.com>
To: Marc Zyngier <maz@kernel.org>
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 11:00:33 +0100	[thread overview]
Message-ID: <20210512100033.GB88854@C02TD0UTHF1T.local> (raw)
In-Reply-To: <d4dc348986083d625f20f63f037b194a@kernel.org>

On Wed, May 12, 2021 at 10:51:04AM +0100, Marc Zyngier wrote:
> 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?

Yes; whoops!

Mark.

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