On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy christophe.leroy@c-s.fr wrote:
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven geert@linux-m68k.org wrote: I'll add to this one also changes to ioreadX_rep() and add another patch for volatile for reads and writes. I guess your review will be appreciated once more because of ioreadX_rep()
volatile should really only be used where deemed necessary:
https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.h...
It is said: " ... accessor functions might use volatile on architectures where direct I/O memory access does work. Essentially, each accessor call becomes a little critical section on its own and ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally makes sense, at least for the implementations that do a plain pointer dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr) { return *(const volatile u32 __force *)addr; }
SuperH is another example: 1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count) calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count) calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force *)(a))
Even if interface was not marked as volatile, in fact its implementation was casting to volatile.
The __force-cast that removes the __iomem here also means that the 'volatile' keyword could be dropped from the argument list, as it has no real effect any more, but then there are a few drivers that mark their iomem pointers as either 'volatile void __iomem*' or (worse) 'volatile void *', so we keep it in the argument list to not add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem pointers, but that seems out of scope for what Krzysztof is trying to do. Ideally we would be consistent here though, either using volatile all the time or never.
Indeed. I guess there are no objections around const so let me send v2 for const only.
Best regards, Krzysztof