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 for Android: free password hash cracker in your pocket

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

Message-Id: <7100d48e-2b11-4d7a-8c5d-48900e8d4916@app.fastmail.com>
Date: Fri, 27 Sep 2024 10:59:43 +0000
From: "Arnd Bergmann" <arnd@...db.de>
To: "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" <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 Fri, Sep 27, 2024, at 08:19, Julian Vetter wrote:
> On 26.09.24 09:14, Arnd Bergmann wrote:
>>> +#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? 

Yes

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

Yes, I think that would be the best way to do it. Alternatively,
arch/um could provide a dummy implementation of these.

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

Sorry for being unclear, I meant only the architectures that
you are already touching.

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

Ideally we should end up with only one copy, but I'd leave the
rest for a future cleanup. In particular, alpha probably still
needs a custom function.

      Arnd