Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210413115439.1011560-1-schnelle@linux.ibm.com>
In-Reply-To: <20210413115439.1011560-1-schnelle@linux.ibm.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Tue, 13 Apr 2021 14:26:38 +0200
Message-ID: <CAK8P3a1WTZOYpJ2TSjnbytQJWgtfwkQ8bXXdnqCnOn6ugJqN_w@mail.gmail.com>
Subject: Re: [PATCH] asm-generic/io.h: Silence -Wnull-pointer-arithmetic
 warning on PCI_IOBASE
To:     Niklas Schnelle <schnelle@linux.ibm.com>
Cc:     Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        clang-built-linux <clang-built-linux@googlegroups.com>,
        linux-arch <linux-arch@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-s390 <linux-s390@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 13, 2021 at 1:54 PM Niklas Schnelle <schnelle@linux.ibm.com> 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@linux.ibm.com>
> ---
>  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