Re: [PATCH v6 1/5] Consolidate __memcpy_{to,from}io and __memset_io into iomap_copy.c

["Arnd Bergmann" <arnd@arndb.de>]

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