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

[Niklas Schnelle]

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@xxxxxxxxxx> 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)