RE: [PATCH] asm-generic/io.h: Silence -Wnull-pointer-arithmetic warning on PCI_IOBASE

[David Laight <David.Laight@ACULAB.COM>]

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.

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

	David

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