Messages in this thread | | | From | Pavel Tatashin <> | Date | Wed, 27 Nov 2019 10:10:07 -0500 | Subject | Re: [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
| |