Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument

[Krzysztof Kozlowski <krzk@kernel.org>]

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


_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc