lkml.org 
[lkml]   [2019]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
Hi Mark,

Thank you for reviewing this work.

> A commit message should provide rationale, rather than just a
> description of the patch. Something like:
>
> | We currently duplicate the logic to enable/disable uaccess via TTBR0,
> | with C functions and assembly macros. This is a maintenenace burden
> | and is liable to lead to subtle bugs, so let's get rid of the assembly
> | macros, and always use the C functions. This requires refactoring
> | some assembly functions to have a C wrapper.

Thank you for suggestion, I will fix my commit log.
>
> [...]
>
> > +static inline int invalidate_icache_range(unsigned long start,
> > + unsigned long end)
> > +{
> > + int rv;
> > +#if ARM64_HAS_CACHE_DIC
> > + rv = arch_invalidate_icache_range(start, end);
> > +#else
> > + uaccess_ttbr0_enable();
> > + rv = arch_invalidate_icache_range(start, end);
> > + uaccess_ttbr0_disable();
> > +#endif
> > + return rv;
> > +}
>
> This ifdeffery is not the same as an alternative_if, and even if it were
> the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
> assembly.
>
> This should be:
>
> static inline int invalidate_icache_range(unsigned long start,
> unsigned long end)
> {
> int ret;
>
> if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
> isb();
> return 0;
> }
>
> uaccess_ttbr0_enable();
> ret = arch_invalidate_icache_range(start, end);
> uaccess_ttbr0_disable();
>
> return ret;
> }

I will fix it, thanks.

>
> The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
> since this is entirely local to the arch code, and even then should only
> be called from the C wrappers.

Sure, I can change it to asm_*, I was using arch_* to be consistent
with __arch_copy_from_user() and friends.

Thank you,
Pasha

\
 
 \ /
  Last update: 2019-11-27 16:11    [W:0.074 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site