On Tue, Apr 13, 2021 at 1:54 PM Niklas Schnelle <[email protected]> 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 <[email protected]>
> ---
> 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!
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
From: Arnd Bergmann
> Sent: 13 April 2021 13:27
>
> On Tue, Apr 13, 2021 at 1:54 PM Niklas Schnelle <[email protected]> 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.
Since the definitions make no sense when PCI_IOBASE in undefined
maybe the functions should either not be defined, be stubs that
do nothing or stubs that are BUG().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, 2021-04-13 at 14:26 +0200, Arnd Bergmann wrote:
> On Tue, Apr 13, 2021 at 1:54 PM Niklas Schnelle <[email protected]> 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 <[email protected]>
> > ---
> > 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.