Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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
>>>
>>