linux-kernel - Re: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b9f3f692-d4fa-473b-9bdf-4ea73b22ccde@kalrayinc.com>
Date: Fri, 27 Sep 2024 10:19:31 +0200
From: Julian Vetter <jvetter@...rayinc.com>
To: Arnd Bergmann <arnd@...db.de>, Julian Vetter <jvetter@...rayinc.com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 guoren <guoren@...nel.org>, Huacai Chen <chenhuacai@...nel.org>, WANG Xuerui
 <kernel@...0n.name>, Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-csky@...r.kernel.org, loongarch@...ts.linux.dev, Yann Sionneau
 <ysionneau@...rayinc.com>
Subject: Re: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io
 into iomap_copy.c

On 26.09.24 09:14, Arnd Bergmann wrote:
> On Wed, Sep 25, 2024, at 13:24, Julian Vetter wrote:
>> Various architectures have almost the same implementations for
>> __memcpy_{to,from}io and __memset_io functions. So, consolidate them
>> into the existing lib/iomap_copy.c.
>>
>> Reviewed-by: Yann Sionneau <ysionneau@...rayinc.com>
>> Signed-off-by: Julian Vetter <jvetter@...rayinc.com>
>> ---
>> Signed-off-by: Julian Vetter <jvetter@...rayinc.com>
> 
> You have a duplicated signoff here.

Yes, thank you. I will remove it in the next patch revision.

> 
> 
>> +#ifndef __memcpy_fromio
>> +void __memcpy_fromio(void *to, const volatile void __iomem *from,
>> size_t count);
>> +#endif
>> +
>> +#ifndef __memcpy_toio
>> +void __memcpy_toio(volatile void __iomem *to, const void *from, size_t
>> count);
>> +#endif
>> +
>> +#ifndef __memset_io
>> +void __memset_io(volatile void __iomem *dst, int c, size_t count);
>> +#endif
> 
> I'm not entirely sure about the purpose of the #ifdef here, since
> nothing ever overrides the double-underscore versions, both before
> and after your patches.
> 
> Unless I'm missing something here, I think a more logical
> sequence would be:
> 
> 1. add the definitions in this file without the underscores,

by: "...in this file..." you mean the 'lib/iomap_copy.c' file, right? 
But what if an architecture does not select 'CONFIG_HAS_IOMEM'. Then 
'iomap_copy.c' is not compiled and we don't have an implementation, 
right? I tried to compile with ARCH=um, with some MTD chip driver, like 
the robot did and it indeed fails, because um has 'NO_IOMEM' set. and 
the driver uses memcpy_fromio. I mean it's a strange combination, 
because apparently we try to use IO memory? Is this an invalid 
combination? But shouldn't the driver then 'depends on HAS_IOMEM'?

>     as memcpy_fromio/memcpy_toio/memset_io, with the #ifdef
>     for that name that is always set at this point
> 
Right. I will remove it in my next patch revision.

> 2. replace the default implementation in asm-generic/io.h
>     with extern prototypes, remove the #define from those
> 
Yes, I have done this now.

> 3. convert the other architectures, removing both the
>     implementations and the prototypes.
> 

I have removed the prototypes and have aligned the function arguments in 
m68k, alpha, parisc, and sh, which all have their own implementation, 
but had slightly different function arguments. Btw, I have not removed 
their implementations because some of them seem to have optimized 
implementations (e.g., alpha and m68k), that I didn't want to touch. But 
you're right others (e.g., sh) just do byte wise accesses and have a 
comment "This needs to be optimized." Maybe I should remove these and 
let them use the new version?!

>       Arnd
> 
> 
> 
>