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@xxxxxx> wrote: > > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit : > > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 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.html > > > > 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 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
