* [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 [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 16:50 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
all inboxes | Powered by JetHome®