Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <3ed30ec5a241c50689fc954103214ce5ed36c463.camel@linux.ibm.com>
Subject: Re: [PATCH] asm-generic/io.h: Silence -Wnull-pointer-arithmetic
 warning on PCI_IOBASE
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     David Laight <David.Laight@ACULAB.COM>,
        "'Arnd Bergmann'" <arnd@arndb.de>
Cc:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        clang-built-linux <clang-built-linux@googlegroups.com>,
        linux-arch <linux-arch@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-s390 <linux-s390@vger.kernel.org>,
        Guo Ren <guoren@kernel.org>
Date:   Wed, 14 Apr 2021 16:02:58 +0200
In-Reply-To: <ac3447d8db2146798b86135e9f49891d@AcuMS.aculab.com>
References: <20210413115439.1011560-1-schnelle@linux.ibm.com>
         <CAK8P3a1WTZOYpJ2TSjnbytQJWgtfwkQ8bXXdnqCnOn6ugJqN_w@mail.gmail.com>
         <84ab737edbe13d390373850bf317920b3a486b87.camel@linux.ibm.com>
         <CAK8P3a2NR2nhEffFQJdMq2Do_g2ji-7p3+iWyzw+aXD6gov05w@mail.gmail.com>
         <11ead5c2c73c42cbbeef32966bc7e5c2@AcuMS.aculab.com>
         <CAK8P3a3PK9zyeP4ymELtc2ZYnymECoACiigw9Za+pvSJpCk5=g@mail.gmail.com>
         <40d4114fa34043d0841b81d09457c415@AcuMS.aculab.com>
         <c6f3c9a70e054e9087f657bf4f142732fd43784c.camel@linux.ibm.com>
         <ac3447d8db2146798b86135e9f49891d@AcuMS.aculab.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 2021-04-14 at 13:50 +0000, David Laight wrote:
> From: Niklas Schnelle
> > Sent: 14 April 2021 13:35
> > 
> > On Tue, 2021-04-13 at 14:12 +0000, David Laight wrote:
> > > From: Arnd Bergmann
> > > > Sent: 13 April 2021 14:40
> > > > 
> > > > On Tue, Apr 13, 2021 at 3:06 PM David Laight <David.Laight@aculab.com> wrote:
> > > > > From: Arnd Bergmann
> > > > > > Sent: 13 April 2021 13:58
> > > > > ...
> > > > > > The remaining ones (csky, m68k, sparc32) need to be inspected
> > > > > > manually to see if they currently support PCI I/O space but in
> > > > > > fact use address zero as the base (with large resources) or they
> > > > > > should also turn the operations into a NOP.
> > > > > 
> > > > > I'd expect sparc32 to use an ASI to access PCI IO space.
> > > > > I can't quite remember whether IO space was supported at all.
> > > > 
> > > > I see this bit in arch/sparc/kernel/leon_pci.c
> > > > 
> > > >  * PCI Memory and Prefetchable Memory is direct-mapped. However I/O Space is
> > > >  * accessed through a Window which is translated to low 64KB in PCI space, the
> > > >  * first 4KB is not used so 60KB is available.
> > > > ...
> > > >         pci_add_resource_offset(&resources, &info->io_space,
> > > >                                 info->io_space.start - 0x1000);
> > > > 
> > > > which means that there is I/O space, which gets accessed through whichever
> > > > method readb() uses. Having the offset equal to the resource means that
> > > > the '(void *)0' start is correct.
> > > 
> > > It must have been the VMEbus (and maybe sBus) sparc that used an ASI.
> > > 
> > > I do remember issues with Solaris of some PCI cards not liking
> > > being assigned a BAR address of zero.
> > > That may be why the low 4k IO space isn't assigned here.
> > > (I've never run Linux on sparc, just SVR4 and Solaris.)
> > > 
> > > I guess setting PCI_IOBASE to zero is safer when you can't trust
> > > drivers not to use inb() instead of readb().
> > > Or whatever io_read() ends up being.
> > > 
> > > 	David
> > 
> > So "I guess setting PCI_IOBASE to zero is safer when you can't trust
> > drivers not to use inb()…" in principle is true on other architectures
> > than sparc too, right? So do you think this means we shouldn't go with
> > Arnd's idea of making inb() just WARN_ONCE() if PCI_IOBASE is not
> > defined or just that for sparc defining it as 0 would be preferred?
> > 
> > As for s390 since we only support a limited number of drivers I think
> > for us such a WARN_ONCE() for inb() would be preferable.
> > 
> > I guess one option would be to let each architecture opt in to leaving
> > PCI_IOBASE undefined but in the first patch push PCI_IOBASE 0 into all
> > drivers that currently don't define it at all _and_ do not define their
> > own inb() etc.
> 
> How much code outside of legacy x86 drivers should be using inb() etc?
> I'm not sure any other (modern) cpu have separate IO instructions.
> 
> Because some PCI(e) resources might be available on memory or IO BARs
> (possible duplicate BAR on some cards) aren't there also ioreadb()
> functions (with addresses as parameters)?
> IIRC on x86 they treat small values as IO ports and large ones
> as memory mapped addresses.
> If PCI IO space is memory mapped then these would be directly equivalent
> to readb() (etc).
> 
> So perhaps inb() should just not be defined at all except on x86?
> (Perhaps except for COMPILE_TEST).
> If it is defined, then maybe it should never be called?
> So a WARN_ONCE() returning ~0 for reads might even be best.

Ok yeah I think that's also what I'd like best.

> 
> Of course, there will be some obscure fallout - there always is.

Let me come up with a patch, then if this decision is wrong it's at
least one of us s390 people breaking someone else's architecture
instead of the usual other way around ;-D

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)