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

[Niklas Schnelle]

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.