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

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

[Niklas Schnelle]

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.

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
@@ -461,7 +461,7 @@ static inline u8 _inb(unsigned long addr)
 	u8 val;
 
 	__io_pbr();
-	val = __raw_readb(PCI_IOBASE + addr);
+	val = __raw_readb((void __iomem *)(PCI_IOBASE + addr));
 	__io_par(val);
 	return val;
 }
@@ -474,7 +474,7 @@ static inline u16 _inw(unsigned long addr)
 	u16 val;
 
 	__io_pbr();
-	val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
+	val = __le16_to_cpu((__le16 __force)__raw_readw((void __iomem *)(PCI_IOBASE + addr)));
 	__io_par(val);
 	return val;
 }
@@ -487,7 +487,7 @@ static inline u32 _inl(unsigned long addr)
 	u32 val;
 
 	__io_pbr();
-	val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
+	val = __le32_to_cpu((__le32 __force)__raw_readl((void __iomem *)(PCI_IOBASE + addr)));
 	__io_par(val);
 	return val;
 }
@@ -498,7 +498,7 @@ static inline u32 _inl(unsigned long addr)
 static inline void _outb(u8 value, unsigned long addr)
 {
 	__io_pbw();
-	__raw_writeb(value, PCI_IOBASE + addr);
+	__raw_writeb(value, (void __iomem *)(PCI_IOBASE + addr));
 	__io_paw();
 }
 #endif
@@ -508,7 +508,7 @@ static inline void _outb(u8 value, unsigned long addr)
 static inline void _outw(u16 value, unsigned long addr)
 {
 	__io_pbw();
-	__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
+	__raw_writew((u16 __force)cpu_to_le16(value), (void __iomem *)(PCI_IOBASE + addr));
 	__io_paw();
 }
 #endif
@@ -518,7 +518,7 @@ static inline void _outw(u16 value, unsigned long addr)
 static inline void _outl(u32 value, unsigned long addr)
 {
 	__io_pbw();
-	__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
+	__raw_writel((u32 __force)cpu_to_le32(value), (void __iomem *)(PCI_IOBASE + addr));
 	__io_paw();
 }
 #endif
@@ -606,7 +606,7 @@ static inline void outl_p(u32 value, unsigned long addr)
 #define insb insb
 static inline void insb(unsigned long addr, void *buffer, unsigned int count)
 {
-	readsb(PCI_IOBASE + addr, buffer, count);
+	readsb((void __iomem *)(PCI_IOBASE + addr), buffer, count);
 }
 #endif
 
@@ -614,7 +614,7 @@ static inline void insb(unsigned long addr, void *buffer, unsigned int count)
 #define insw insw
 static inline void insw(unsigned long addr, void *buffer, unsigned int count)
 {
-	readsw(PCI_IOBASE + addr, buffer, count);
+	readsw((void __iomem *)(PCI_IOBASE + addr), buffer, count);
 }
 #endif
 
@@ -622,7 +622,7 @@ static inline void insw(unsigned long addr, void *buffer, unsigned int count)
 #define insl insl
 static inline void insl(unsigned long addr, void *buffer, unsigned int count)
 {
-	readsl(PCI_IOBASE + addr, buffer, count);
+	readsl((void __iomem *)(PCI_IOBASE + addr), buffer, count);
 }
 #endif
 
@@ -631,7 +631,7 @@ static inline void insl(unsigned long addr, void *buffer, unsigned int count)
 static inline void outsb(unsigned long addr, const void *buffer,
 			 unsigned int count)
 {
-	writesb(PCI_IOBASE + addr, buffer, count);
+	writesb((void __iomem *)(PCI_IOBASE + addr), buffer, count);
 }
 #endif
 
@@ -640,7 +640,7 @@ static inline void outsb(unsigned long addr, const void *buffer,
 static inline void outsw(unsigned long addr, const void *buffer,
 			 unsigned int count)
 {
-	writesw(PCI_IOBASE + addr, buffer, count);
+	writesw((void __iomem *)(PCI_IOBASE + addr), buffer, count);
 }
 #endif
 
@@ -649,7 +649,7 @@ static inline void outsw(unsigned long addr, const void *buffer,
 static inline void outsl(unsigned long addr, const void *buffer,
 			 unsigned int count)
 {
-	writesl(PCI_IOBASE + addr, buffer, count);
+	writesl((void __iomem *)(PCI_IOBASE + addr), buffer, count);
 }
 #endif
 
-- 
2.25.1

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

[Arnd Bergmann]

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

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@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!

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.

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

[Arnd Bergmann]

On Tue, Apr 13, 2021 at 2:38 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> On Tue, 2021-04-13 at 14:26 +0200, Arnd Bergmann wrote:
> > 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?
>
> 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.

Yes, I meant doing this in the asm-generic version, something like

#if !defined(inb) && !defined(_inb)
#define _inb _inb
static inline u8 _inb(unsigned long addr)
{
#ifdef PCI_IOBASE
        u8 val;

        __io_pbr();
        val = __raw_readb(PCI_IOBASE + addr);
        __io_par(val);
        return val;
#else
        WARN_ONCE(1, "No I/O port support");
        return 0xff;
#endif
}
#endif

And follow up with a separate patch that moves the
"#define PCI_IOBASE ((void __iomem *)0)" into the architectures
that would currently see it, i.e. those that include asm-generic/io.h
but define neither inb/_inb nor PCI_IOBASE:

$ git grep -l asm-generic/io.h | xargs grep -wL inb | xargs grep -L PCI_IOBASE
arch/arc/include/asm/io.h
arch/csky/include/asm/io.h
arch/h8300/include/asm/io.h
arch/m68k/include/asm/io.h
arch/nds32/include/asm/io.h
arch/nios2/include/asm/io.h
arch/openrisc/include/asm/io.h
arch/s390/include/asm/io.h
arch/sparc/include/asm/io_32.h
arch/um/include/asm/io.h

Out of these, I see that arc, h8300, nds32, nios2, openrisc, and um
never set CONFIG_HAVE_PCI, so these would all be better off
leaving PCI_IOBASE undefined and getting the WARN_ONCE.

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.

         Arnd

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

[David Laight]

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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Arnd Bergmann]

On Tue, Apr 13, 2021 at 3:06 PM David Laight <David.Laight@aculab.com> 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.

As this leaves only two others, I checked those as well:

csky does not actually have a PCI host bridge driver at the moment, so
we don't care about breaking port access on it it, and I would suggest
leaving I/O port access disabled. (Added Guo Ren to Cc for confirmation).

m68k only supports PCI on coldfire M54xx, and this variant does set
a PCI_IOBASE after all. The normal MMU based m68k have no PCI
and do define their out inb/outb/..., so nothing changes for them.

To summarize: only sparc32 needs to set PCI_IOBASE to zero, everyone
else should just WARN_ONCE() or return 0xff/0xffff/0xffffffff.

        Arnd

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

[Guo Ren]

On Tue, Apr 13, 2021 at 9:40 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Apr 13, 2021 at 3:06 PM David Laight <David.Laight@aculab.com> 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.
>
> As this leaves only two others, I checked those as well:
>
> csky does not actually have a PCI host bridge driver at the moment, so
> we don't care about breaking port access on it it, and I would suggest
> leaving I/O port access disabled. (Added Guo Ren to Cc for confirmation).
Yes, we haven't reserved the PCI_IO region in the VM layout.

>
> m68k only supports PCI on coldfire M54xx, and this variant does set
> a PCI_IOBASE after all. The normal MMU based m68k have no PCI
> and do define their out inb/outb/..., so nothing changes for them.
>
> To summarize: only sparc32 needs to set PCI_IOBASE to zero, everyone
> else should just WARN_ONCE() or return 0xff/0xffff/0xffffffff.
>
>         Arnd



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

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

[David Laight]

From: Arnd Bergmann
> Sent: 13 April 2021 14:40
> 
> On Tue, Apr 13, 2021 at 3:06 PM David Laight <David.Laight@aculab.com> 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

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Niklas Schnelle]

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@aculab.com> 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.

> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

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

[David Laight]

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@aculab.com> 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.

Of course, there will be some obscure fallout - there always is.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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@aculab.com> 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)

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

[David Laight]

From: Arnd Bergmann
> Sent: 13 April 2021 13:27
> 
> 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.

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)