On Tue, 2021-04-13 at 14:26 +0200, Arnd Bergmann wrote: > On Tue, Apr 13, 2021 at 1:54 PM Niklas Schnelle <schnelle@xxxxxxxxxxxxx> wrote: > > When PCI_IOBASE is not defined, it is set to 0 such that it is ignored > > in calls to the readX/writeX primitives. While mathematically obvious > > this triggers clang's -Wnull-pointer-arithmetic warning. > > Indeed, this is an annoying warning. > > > An additional complication is that PCI_IOBASE is explicitly typed as > > "void __iomem *" which causes the type conversion that converts the > > "unsigned long" port/addr parameters to the appropriate pointer type. > > As non pointer types are used by drivers at the callsite since these are > > dealing with I/O port numbers, changing the parameter type would cause > > further warnings in drivers. Instead use "uintptr_t" for PCI_IOBASE > > 0 and explicitly cast to "void __iomem *" when calling readX/writeX. > > > > Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx> > > --- > > include/asm-generic/io.h | 26 +++++++++++++------------- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > index c6af40ce03be..8eb00bdef7ad 100644 > > --- a/include/asm-generic/io.h > > +++ b/include/asm-generic/io.h > > @@ -441,7 +441,7 @@ static inline void writesq(volatile void __iomem *addr, const void *buffer, > > #endif /* CONFIG_64BIT */ > > > > #ifndef PCI_IOBASE > > -#define PCI_IOBASE ((void __iomem *)0) > > +#define PCI_IOBASE ((uintptr_t)0) > > #endif > > > > #ifndef IO_SPACE_LIMIT > > Your patch looks wrong in the way it changes the type of one of the definitions, > but not the type of any of the architecture specific ones. It's also > awkward since > 'void __iomem *' is really the correct type, while 'uintptr_t' is not! Yeah I see your point. The way I justified it for myself is that the above define really only serves to ignore the PCI_IOBASE and the explicit cast in the function makes the actual type more clear since the parameters have the "wrong" type too. I do agree that this still leaves things somewhat awkward. > > I think the real underlying problem is that '0' is a particularly bad > default value, > we should not have used this one in asm-generic, or maybe have left it as > mandatory to be defined by an architecture to a sane value. Note that > architectures that don't actually support I/O ports will cause a NULL > pointer dereference when loading a legacy driver, which is exactly what clang > is warning about here. Architectures that to support I/O ports in PCI typically > map them at a fixed location in the virtual address space and should put that > location here, rather than adding the pointer value to the port resources. > > What we might do instead here would be move the definition into those > architectures that actually define the base to be at address zero, with a > comment explaining why this is a bad location, and then changing the > inb/outb style helpers to either an empty function or a WARN_ONCE(). > > On which architectures do you see the problem? How is the I/O port > actually mapped there? > > Arnd I'm seeing this on s390 which indeed has no I/O port support at all. I'm not sure how many others there are but I feel like us having to define these functions as empty is also kind of awkward. Maybe we could put them into the asm-generic/io.h for the case that PCI_IOBASE is not defined? Then at least every platform not supporting I/O ports would share them.
