* [PATCH] powerpc: Remove eieio in _memcpy_fromio @ 2025-01-28 13:57 Julian Vetter 2025-01-28 14:16 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: Julian Vetter @ 2025-01-28 13:57 UTC (permalink / raw) To: Arnd Bergmann, Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao Cc: linuxppc-dev, linux-kernel, Julian Vetter Remove the eieio() calls in _memcpy_fromio, to bring its implementation closer to the one from lib/iomem_copy.c. These eieio() calls don't seem to be necessary, because the _memcpy_toio completely omits them. Also the legacy code from ppc was not doing them. Signed-off-by: Julian Vetter <julian@outer-limits.org> --- arch/powerpc/kernel/io.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c index 6af535905984..81e5d54260a1 100644 --- a/arch/powerpc/kernel/io.c +++ b/arch/powerpc/kernel/io.c @@ -155,21 +155,18 @@ void _memcpy_fromio(void *dest, const volatile void __iomem *src, __asm__ __volatile__ ("sync" : : : "memory"); while(n && (!IO_CHECK_ALIGN(vsrc, 4) || !IO_CHECK_ALIGN(dest, 4))) { *((u8 *)dest) = *((volatile u8 *)vsrc); - eieio(); vsrc++; dest++; n--; } while(n >= 4) { *((u32 *)dest) = *((volatile u32 *)vsrc); - eieio(); vsrc += 4; dest += 4; n -= 4; } while(n) { *((u8 *)dest) = *((volatile u8 *)vsrc); - eieio(); vsrc++; dest++; n--; -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio 2025-01-28 13:57 [PATCH] powerpc: Remove eieio in _memcpy_fromio Julian Vetter @ 2025-01-28 14:16 ` Christophe Leroy 2025-01-28 15:07 ` Julian Vetter 0 siblings, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2025-01-28 14:16 UTC (permalink / raw) To: Julian Vetter, Arnd Bergmann, Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin, Naveen N Rao Cc: linuxppc-dev, linux-kernel Le 28/01/2025 à 14:57, Julian Vetter a écrit : > Remove the eieio() calls in _memcpy_fromio, to bring its implementation > closer to the one from lib/iomem_copy.c. These eieio() calls don't seem > to be necessary, because the _memcpy_toio completely omits them. Also > the legacy code from ppc was not doing them. What do you mean exactly by "legacy code" ? As far as I can see they were already there before commit 68a64357d15a ("[POWERPC] Merge 32 and 64 bits asm-powerpc/io.h"): -static inline void eeh_memcpy_fromio(void *dest, const volatile void __iomem *src, +static inline void eeh_memcpy_fromio(void *dest, const + volatile void __iomem *src, unsigned long n) { - void *vsrc = (void __force *) src; - void *destsave = dest; - unsigned long nsave = n; - - __asm__ __volatile__ ("sync" : : : "memory"); - while(n && (!EEH_CHECK_ALIGN(vsrc, 4) || !EEH_CHECK_ALIGN(dest, 4))) { - *((u8 *)dest) = *((volatile u8 *)vsrc); - __asm__ __volatile__ ("eieio" : : : "memory"); - vsrc++; - dest++; - n--; - } - while(n > 4) { - *((u32 *)dest) = *((volatile u32 *)vsrc); - __asm__ __volatile__ ("eieio" : : : "memory"); - vsrc += 4; - dest += 4; - n -= 4; - } - while(n) { - *((u8 *)dest) = *((volatile u8 *)vsrc); - __asm__ __volatile__ ("eieio" : : : "memory"); - vsrc++; - dest++; - n--; - } - __asm__ __volatile__ ("sync" : : : "memory"); + _memcpy_fromio(dest, src, n); /* Look for ffff's here at dest[n]. Assume that at least 4 bytes * were copied. Check all four bytes. */ - if ((nsave >= 4) && - (EEH_POSSIBLE_ERROR((*((u32 *) destsave+nsave-4)), u32))) { - eeh_check_failure(src, (*((u32 *) destsave+nsave-4))); - } + if (n >= 4 && EEH_POSSIBLE_ERROR(*((u32 *)(dest + n - 4)), u32)) + eeh_check_failure(src, *((u32 *)(dest + n - 4))); } > > Signed-off-by: Julian Vetter <julian@outer-limits.org> > --- > arch/powerpc/kernel/io.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c > index 6af535905984..81e5d54260a1 100644 > --- a/arch/powerpc/kernel/io.c > +++ b/arch/powerpc/kernel/io.c > @@ -155,21 +155,18 @@ void _memcpy_fromio(void *dest, const volatile void __iomem *src, > __asm__ __volatile__ ("sync" : : : "memory"); > while(n && (!IO_CHECK_ALIGN(vsrc, 4) || !IO_CHECK_ALIGN(dest, 4))) { > *((u8 *)dest) = *((volatile u8 *)vsrc); > - eieio(); > vsrc++; > dest++; > n--; > } > while(n >= 4) { > *((u32 *)dest) = *((volatile u32 *)vsrc); > - eieio(); > vsrc += 4; > dest += 4; > n -= 4; > } > while(n) { > *((u8 *)dest) = *((volatile u8 *)vsrc); > - eieio(); > vsrc++; > dest++; > n--; > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio 2025-01-28 14:16 ` Christophe Leroy @ 2025-01-28 15:07 ` Julian Vetter 2025-01-28 15:24 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: Julian Vetter @ 2025-01-28 15:07 UTC (permalink / raw) To: Christophe Leroy, Arnd Bergmann, Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin, Naveen N Rao Cc: linuxppc-dev, linux-kernel On 1/28/25 15:16, Christophe Leroy wrote: > > > Le 28/01/2025 à 14:57, Julian Vetter a écrit : >> Remove the eieio() calls in _memcpy_fromio, to bring its implementation >> closer to the one from lib/iomem_copy.c. These eieio() calls don't seem >> to be necessary, because the _memcpy_toio completely omits them. Also >> the legacy code from ppc was not doing them. > > What do you mean exactly by "legacy code" ? > > As far as I can see they were already there before commit 68a64357d15a > ("[POWERPC] Merge 32 and 64 bits asm-powerpc/io.h"): > With 'ppc' I was refering to 'include/asm-ppc/io.h'. But you're right, when going back a bit, in the 'include/asm-powerpc/io.h' there are two cases, one (eeh_memcpy_fromio) which does the the 'eieio', and a second, i.e., 'iSeries_memcpy_fromio' which does a byte-wise copy. But in the ppc code ('include/asm-ppc/io.h') there is a simple memcpy. I was referring to this one. But my description is not very clear. Sorry for that. > -static inline void eeh_memcpy_fromio(void *dest, const volatile void > __iomem *src, > +static inline void eeh_memcpy_fromio(void *dest, const > + volatile void __iomem *src, > unsigned long n) > { > - void *vsrc = (void __force *) src; > - void *destsave = dest; > - unsigned long nsave = n; > - > - __asm__ __volatile__ ("sync" : : : "memory"); > - while(n && (!EEH_CHECK_ALIGN(vsrc, 4) || !EEH_CHECK_ALIGN(dest, 4))) { > - *((u8 *)dest) = *((volatile u8 *)vsrc); > - __asm__ __volatile__ ("eieio" : : : "memory"); > - vsrc++; > - dest++; > - n--; > - } > - while(n > 4) { > - *((u32 *)dest) = *((volatile u32 *)vsrc); > - __asm__ __volatile__ ("eieio" : : : "memory"); > - vsrc += 4; > - dest += 4; > - n -= 4; > - } > - while(n) { > - *((u8 *)dest) = *((volatile u8 *)vsrc); > - __asm__ __volatile__ ("eieio" : : : "memory"); > - vsrc++; > - dest++; > - n--; > - } > - __asm__ __volatile__ ("sync" : : : "memory"); > + _memcpy_fromio(dest, src, n); > > /* Look for ffff's here at dest[n]. Assume that at least 4 bytes > * were copied. Check all four bytes. > */ > - if ((nsave >= 4) && > - (EEH_POSSIBLE_ERROR((*((u32 *) destsave+nsave-4)), u32))) { > - eeh_check_failure(src, (*((u32 *) destsave+nsave-4))); > - } > + if (n >= 4 && EEH_POSSIBLE_ERROR(*((u32 *)(dest + n - 4)), u32)) > + eeh_check_failure(src, *((u32 *)(dest + n - 4))); > } > > > >> >> Signed-off-by: Julian Vetter <julian@outer-limits.org> >> --- >> arch/powerpc/kernel/io.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c >> index 6af535905984..81e5d54260a1 100644 >> --- a/arch/powerpc/kernel/io.c >> +++ b/arch/powerpc/kernel/io.c >> @@ -155,21 +155,18 @@ void _memcpy_fromio(void *dest, const volatile >> void __iomem *src, >> __asm__ __volatile__ ("sync" : : : "memory"); >> while(n && (!IO_CHECK_ALIGN(vsrc, 4) || !IO_CHECK_ALIGN(dest, >> 4))) { >> *((u8 *)dest) = *((volatile u8 *)vsrc); >> - eieio(); >> vsrc++; >> dest++; >> n--; >> } >> while(n >= 4) { >> *((u32 *)dest) = *((volatile u32 *)vsrc); >> - eieio(); >> vsrc += 4; >> dest += 4; >> n -= 4; >> } >> while(n) { >> *((u8 *)dest) = *((volatile u8 *)vsrc); >> - eieio(); >> vsrc++; >> dest++; >> n--; >> -- >> 2.34.1 >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio 2025-01-28 15:07 ` Julian Vetter @ 2025-01-28 15:24 ` Christophe Leroy 2025-01-28 15:34 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2025-01-28 15:24 UTC (permalink / raw) To: Julian Vetter, Arnd Bergmann, Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin, Naveen N Rao Cc: linuxppc-dev, linux-kernel Le 28/01/2025 à 16:07, Julian Vetter a écrit : > [Vous ne recevez pas souvent de courriers de julian@outer-limits.org. > Découvrez pourquoi ceci est important à https://aka.ms/ > LearnAboutSenderIdentification ] > > On 1/28/25 15:16, Christophe Leroy wrote: >> >> >> Le 28/01/2025 à 14:57, Julian Vetter a écrit : >>> Remove the eieio() calls in _memcpy_fromio, to bring its implementation >>> closer to the one from lib/iomem_copy.c. These eieio() calls don't seem >>> to be necessary, because the _memcpy_toio completely omits them. Also >>> the legacy code from ppc was not doing them. >> >> What do you mean exactly by "legacy code" ? >> >> As far as I can see they were already there before commit 68a64357d15a >> ("[POWERPC] Merge 32 and 64 bits asm-powerpc/io.h"): >> > > With 'ppc' I was refering to 'include/asm-ppc/io.h'. But you're right, > when going back a bit, in the 'include/asm-powerpc/io.h' there are two > cases, one (eeh_memcpy_fromio) which does the the 'eieio', and a second, > i.e., 'iSeries_memcpy_fromio' which does a byte-wise copy. But in the > ppc code ('include/asm-ppc/io.h') there is a simple memcpy. I was > referring to this one. But my description is not very clear. Sorry for > that. But then is your change still valid ? Isn't there some corner case that still need it ? Is it a valid argument that because memcpy_toio() doesn't need it memcpy_fromio() doesn't need it either ? > >> -static inline void eeh_memcpy_fromio(void *dest, const volatile void >> __iomem *src, >> +static inline void eeh_memcpy_fromio(void *dest, const >> + volatile void __iomem *src, >> unsigned long n) >> { >> - void *vsrc = (void __force *) src; >> - void *destsave = dest; >> - unsigned long nsave = n; >> - >> - __asm__ __volatile__ ("sync" : : : "memory"); >> - while(n && (!EEH_CHECK_ALIGN(vsrc, 4) || !EEH_CHECK_ALIGN(dest, >> 4))) { >> - *((u8 *)dest) = *((volatile u8 *)vsrc); >> - __asm__ __volatile__ ("eieio" : : : "memory"); >> - vsrc++; >> - dest++; >> - n--; >> - } >> - while(n > 4) { >> - *((u32 *)dest) = *((volatile u32 *)vsrc); >> - __asm__ __volatile__ ("eieio" : : : "memory"); >> - vsrc += 4; >> - dest += 4; >> - n -= 4; >> - } >> - while(n) { >> - *((u8 *)dest) = *((volatile u8 *)vsrc); >> - __asm__ __volatile__ ("eieio" : : : "memory"); >> - vsrc++; >> - dest++; >> - n--; >> - } >> - __asm__ __volatile__ ("sync" : : : "memory"); >> + _memcpy_fromio(dest, src, n); >> >> /* Look for ffff's here at dest[n]. Assume that at least 4 bytes >> * were copied. Check all four bytes. >> */ >> - if ((nsave >= 4) && >> - (EEH_POSSIBLE_ERROR((*((u32 *) destsave+nsave-4)), u32))) { >> - eeh_check_failure(src, (*((u32 *) destsave+nsave-4))); >> - } >> + if (n >= 4 && EEH_POSSIBLE_ERROR(*((u32 *)(dest + n - 4)), u32)) >> + eeh_check_failure(src, *((u32 *)(dest + n - 4))); >> } >> >> >> >>> >>> Signed-off-by: Julian Vetter <julian@outer-limits.org> >>> --- >>> arch/powerpc/kernel/io.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c >>> index 6af535905984..81e5d54260a1 100644 >>> --- a/arch/powerpc/kernel/io.c >>> +++ b/arch/powerpc/kernel/io.c >>> @@ -155,21 +155,18 @@ void _memcpy_fromio(void *dest, const volatile >>> void __iomem *src, >>> __asm__ __volatile__ ("sync" : : : "memory"); >>> while(n && (!IO_CHECK_ALIGN(vsrc, 4) || !IO_CHECK_ALIGN(dest, >>> 4))) { >>> *((u8 *)dest) = *((volatile u8 *)vsrc); >>> - eieio(); >>> vsrc++; >>> dest++; >>> n--; >>> } >>> while(n >= 4) { >>> *((u32 *)dest) = *((volatile u32 *)vsrc); >>> - eieio(); >>> vsrc += 4; >>> dest += 4; >>> n -= 4; >>> } >>> while(n) { >>> *((u8 *)dest) = *((volatile u8 *)vsrc); >>> - eieio(); >>> vsrc++; >>> dest++; >>> n--; >>> -- >>> 2.34.1 >>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio 2025-01-28 15:24 ` Christophe Leroy @ 2025-01-28 15:34 ` Christophe Leroy 2025-01-28 16:50 ` Arnd Bergmann 0 siblings, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2025-01-28 15:34 UTC (permalink / raw) To: Julian Vetter, Arnd Bergmann, Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin, Naveen N Rao Cc: linuxppc-dev, linux-kernel Le 28/01/2025 à 16:24, Christophe Leroy a écrit : > > > Le 28/01/2025 à 16:07, Julian Vetter a écrit : >> [Vous ne recevez pas souvent de courriers de julian@outer-limits.org. >> Découvrez pourquoi ceci est important à https://aka.ms/ >> LearnAboutSenderIdentification ] >> >> On 1/28/25 15:16, Christophe Leroy wrote: >>> >>> >>> Le 28/01/2025 à 14:57, Julian Vetter a écrit : >>>> Remove the eieio() calls in _memcpy_fromio, to bring its implementation >>>> closer to the one from lib/iomem_copy.c. These eieio() calls don't seem >>>> to be necessary, because the _memcpy_toio completely omits them. Also >>>> the legacy code from ppc was not doing them. >>> >>> What do you mean exactly by "legacy code" ? >>> >>> As far as I can see they were already there before commit 68a64357d15a >>> ("[POWERPC] Merge 32 and 64 bits asm-powerpc/io.h"): >>> >> >> With 'ppc' I was refering to 'include/asm-ppc/io.h'. But you're right, >> when going back a bit, in the 'include/asm-powerpc/io.h' there are two >> cases, one (eeh_memcpy_fromio) which does the the 'eieio', and a second, >> i.e., 'iSeries_memcpy_fromio' which does a byte-wise copy. But in the >> ppc code ('include/asm-ppc/io.h') there is a simple memcpy. I was >> referring to this one. But my description is not very clear. Sorry for >> that. > > But then is your change still valid ? Isn't there some corner case that > still need it ? Is it a valid argument that because memcpy_toio() > doesn't need it memcpy_fromio() doesn't need it either ? I see that _insb(), _insw_ns() and _insl_ns() also have eieio() while _outsb(), _outsw_ns() and _outsl_ns() don't. Why not change those as well if you think eieio() is not needed ? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio 2025-01-28 15:34 ` Christophe Leroy @ 2025-01-28 16:50 ` Arnd Bergmann 0 siblings, 0 replies; 6+ messages in thread From: Arnd Bergmann @ 2025-01-28 16:50 UTC (permalink / raw) To: Christophe Leroy, Julian Vetter, Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin, Naveen N Rao Cc: linuxppc-dev, linux-kernel On Tue, Jan 28, 2025, at 16:34, Christophe Leroy wrote: > Le 28/01/2025 à 16:24, Christophe Leroy a écrit : >> Le 28/01/2025 à 16:07, Julian Vetter a écrit : >>> With 'ppc' I was refering to 'include/asm-ppc/io.h'. But you're right, >>> when going back a bit, in the 'include/asm-powerpc/io.h' there are two >>> cases, one (eeh_memcpy_fromio) which does the the 'eieio', and a second, >>> i.e., 'iSeries_memcpy_fromio' which does a byte-wise copy. But in the >>> ppc code ('include/asm-ppc/io.h') there is a simple memcpy. I was >>> referring to this one. But my description is not very clear. Sorry for >>> that. >> >> But then is your change still valid ? Isn't there some corner case that >> still need it ? Is it a valid argument that because memcpy_toio() >> doesn't need it memcpy_fromio() doesn't need it either ? > > I see that _insb(), _insw_ns() and _insl_ns() also have eieio() while > _outsb(), _outsw_ns() and _outsl_ns() don't. Why not change those as > well if you think eieio() is not needed ? I think that makes sense, even if it's beyond the scope of Julian's work to unify the memcpy/memset I/O helpers across architectures. I looked into the pre-2.6.12 history of arch/powerpc64 to see how the eieio got in there originally and found that at the time the string functions got added, this is what the readl() etc functions did. readl() itself went through a longer set of changes to end up with the current sync/twi/isync version, but the string functions were never updated again during any of the later changes. The bit that needs to be captured in the changelog here is that on all other architectures, strcpy_fromio/strcpy_toio are written to allow prefetching/combining/reordering, while the powerpc version prevents this in strcpy_fromio for apparently only history reasons. Arnd ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-28 19:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-28 13:57 [PATCH] powerpc: Remove eieio in _memcpy_fromio Julian Vetter 2025-01-28 14:16 ` Christophe Leroy 2025-01-28 15:07 ` Julian Vetter 2025-01-28 15:24 ` Christophe Leroy 2025-01-28 15:34 ` Christophe Leroy 2025-01-28 16:50 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).