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 <[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
@@ -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
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 <[email protected]> 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)