Re: [PATCH v3 1/2] x86, powerpc: Rename memcpy_mcsafe() to copy_mc_to_{user, kernel}()

Hi Dan, Just a couple of minor things ... Dan Williams <dan.j.williams(a)intel.com&gt; writes: > In reaction to a proposal to introduce a memcpy_mcsafe_fast() > implementation Linus points out that memcpy_mcsafe() is poorly named > relative to communicating the scope of the interface. Specifically what > addresses are valid to pass as source, destination, and what faults / > exceptions are handled. Of particular concern is that even though x86 > might be able to handle the semantics of copy_mc_to_user() with its > common copy_user_generic() implementation other archs likely need / want > an explicit path for this case: ... > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h > index 0969285996cb..dcbbcbf3552c 100644 > --- a/arch/powerpc/include/asm/uaccess.h > +++ b/arch/powerpc/include/asm/uaccess.h > @@ -348,6 +348,32 @@ do { \ > extern unsigned long __copy_tofrom_user(void __user *to, > const void __user *from, unsigned long size); > > +#ifdef CONFIG_ARCH_HAS_COPY_MC > +extern unsigned long __must_check We try not to add extern in headers anymore.

> +copy_mc_generic(void *to, const void *from, unsigned long size); > + > +static inline unsigned long __must_check > +copy_mc_to_kernel(void *to, const void *from, unsigned long size) > +{ > + return copy_mc_generic(to, from, size); > +} > +#define copy_mc_to_kernel copy_mc_to_kernel > + > +static inline unsigned long __must_check > +copy_mc_to_user(void __user *to, const void *from, unsigned long n) > +{ > + if (likely(check_copy_size(from, n, true))) { > + if (access_ok(to, n)) { > + allow_write_to_user(to, n); > + n = copy_mc_generic((void *)to, from, n); > + prevent_write_to_user(to, n); > + } > + } > + > + return n; > +} > +#endif Otherwise that looks fine.

... > diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile > index 0917983a1c78..959817e7567c 100644 > --- a/tools/testing/selftests/powerpc/copyloops/Makefile > +++ b/tools/testing/selftests/powerpc/copyloops/Makefile > @@ -45,9 +45,9 @@ $(OUTPUT)/memcpy_p7_t%: memcpy_power7.S $(EXTRA_SOURCES) > -D SELFTEST_CASE=$(subst memcpy_p7_t,,$(notdir $@)) \ > -o $@ $^ > > -$(OUTPUT)/memcpy_mcsafe_64: memcpy_mcsafe_64.S $(EXTRA_SOURCES) > +$(OUTPUT)/copy_mc: copy_mc.S $(EXTRA_SOURCES) > $(CC) $(CPPFLAGS) $(CFLAGS) \ > - -D COPY_LOOP=test_memcpy_mcsafe \ > + -D COPY_LOOP=test_copy_mc \

This needs a fixup: diff --git a/tools/testing/selftests/powerpc/copyloops/Makefile b/tools/testing/selftests/powerpc/copyloops/Makefile index 959817e7567c..b4eb5c4c6858 100644 --- a/tools/testing/selftests/powerpc/copyloops/Makefile +++ b/tools/testing/selftests/powerpc/copyloops/Makefile @@ -47,7 +47,7 @@ $(OUTPUT)/memcpy_p7_t%: memcpy_power7.S $(EXTRA_SOURCES) $(OUTPUT)/copy_mc: copy_mc.S $(EXTRA_SOURCES) $(CC) $(CPPFLAGS) $(CFLAGS) \ - -D COPY_LOOP=test_copy_mc \ + -D COPY_LOOP=test_copy_mc_generic \ -o $@ $^ $(OUTPUT)/copyuser_64_exc_t%: copyuser_64.S exc_validate.c ../harness.c \ > -o $@ $^ > > $(OUTPUT)/copyuser_64_exc_t%: copyuser_64.S exc_validate.c ../harness.c \ > diff --git a/tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S b/tools/testing/selftests/powerpc/copyloops/copy_mc.S > similarity index 100% > rename from tools/testing/selftests/powerpc/copyloops/memcpy_mcsafe_64.S > rename to tools/testing/selftests/powerpc/copyloops/copy_mc.S This file is a symlink to the file in arch/powerpc/lib, so the name of the link needs updating, as well as the target. Also is there a reason you dropped the "_64"? It would make most sense to keep it I think, as then the file in selftests and the file in arch/ have the same name. If you want to keep the copy_mc.S name it needs the diff below. Though as I said I think it would be better to use copy_mc_64.S.