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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAK8P3a21yPrmp4ik3Ei1BZfeqZNf0wL5NZNF3uXqb4FLRDyUPw@mail.gmail.com>
Date:   Wed, 8 Jan 2020 09:44:36 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     Krzysztof Kozlowski <krzk@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Rich Felker <dalias@...c.org>,
        Jiri Slaby <jirislaby@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        David Airlie <airlied@...ux.ie>,
        Jason Wang <jasowang@...hat.com>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        virtualization@...ts.linux-foundation.org,
        "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
        netdev <netdev@...r.kernel.org>,
        Paul Mackerras <paulus@...ba.org>,
        Linux-Arch <linux-arch@...r.kernel.org>,
        Dave Jiang <dave.jiang@...el.com>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        Helge Deller <deller@....de>,
        Linux-sh list <linux-sh@...r.kernel.org>,
        Alexey Brodkin <abrodkin@...opsys.com>,
        Ben Skeggs <bskeggs@...hat.com>,
        ML nouveau <nouveau@...ts.freedesktop.org>,
        Dave Airlie <airlied@...hat.com>,
        Matt Turner <mattst88@...il.com>,
        arcml <linux-snps-arc@...ts.infradead.org>,
        Nick Kossifidis <mickflemm@...il.com>,
        Allen Hubbe <allenbh@...il.com>,
        alpha <linux-alpha@...r.kernel.org>,
        Ivan Kokshaysky <ink@...assic.park.msu.ru>,
        Thomas Gleixner <tglx@...utronix.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Kalle Valo <kvalo@...eaurora.org>,
        Richard Henderson <rth@...ddle.net>,
        Parisc List <linux-parisc@...r.kernel.org>,
        Vineet Gupta <vgupta@...opsys.com>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Daniel Vetter <daniel@...ll.ch>, Jon Mason <jdmason@...zu.us>,
        linux-ntb@...glegroups.com,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument

On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy <christophe.leroy@....fr> wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven <geert@...ux-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;
}

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.

        Arnd