[PATCH v4 3/3] asm-generic/io.h: warn in inb() and friends with undefined PCI_IOBASE
When PCI_IOBASE is not defined, it is set to 0 such that it is ignored
in calls to the readX/writeX primitives. This triggers clang's
-Wnull-pointer-arithmetic warning and will result in illegal accesses on
platforms that do not support I/O ports.
Make things explicit and silence the warning by letting inb() and
friends fail with WARN_ONCE() and a 0xff... return in case PCI_IOBASE is
not defined.
Signed-off-by: Niklas Schnelle
---
include/asm-generic/io.h | 65 +---
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index c6af40ce03be..6207fb4b9a7d 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -8,6 +8,7 @@
#define __ASM_GENERIC_IO_H
#include /* I/O is all done through memory accesses */
+#include
#include /* for memset() and memcpy() */
#include
@@ -440,10 +441,6 @@ static inline void writesq(volatile void __iomem *addr,
const void *buffer,
#endif
#endif /* CONFIG_64BIT */
-#ifndef PCI_IOBASE
-#define PCI_IOBASE ((void __iomem *)0)
-#endif
-
#ifndef IO_SPACE_LIMIT
#define IO_SPACE_LIMIT 0x
#endif
@@ -458,12 +455,17 @@ static inline void writesq(volatile void __iomem *addr,
const void *buffer,
#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\n");
+ return ~0;
+#endif
}
#endif
@@ -471,12 +473,17 @@ static inline u8 _inb(unsigned long addr)
#define _inw _inw
static inline u16 _inw(unsigned long addr)
{
+#ifdef PCI_IOBASE
u16 val;
__io_pbr();
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
__io_par(val);
return val;
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+ return ~0;
+#endif
}
#endif
@@ -484,12 +491,17 @@ static inline u16 _inw(unsigned long addr)
#define _inl _inl
static inline u32 _inl(unsigned long addr)
{
+#ifdef PCI_IOBASE
u32 val;
__io_pbr();
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
__io_par(val);
return val;
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+ return ~0;
+#endif
}
#endif
@@ -497,9 +509,13 @@ static inline u32 _inl(unsigned long addr)
#define _outb _outb
static inline void _outb(u8 value, unsigned long addr)
{
+#ifdef PCI_IOBASE
__io_pbw();
__raw_writeb(value, PCI_IOBASE + addr);
__io_paw();
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -507,9 +523,13 @@ static inline void _outb(u8 value, unsigned long addr)
#define _outw _outw
static inline void _outw(u16 value, unsigned long addr)
{
+#ifdef PCI_IOBASE
__io_pbw();
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
__io_paw();
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -517,9 +537,13 @@ static inline void _outw(u16 value, unsigned long addr)
#define _outl _outl
static inline void _outl(u32 value, unsigned long addr)
{
+#ifdef PCI_IOBASE
__io_pbw();
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
__io_paw();
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -606,7 +630,11 @@ 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)
{
+#ifdef PCI_IOBASE
readsb(PCI_IOBASE + addr, buffer, count);
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -614,7 +642,11 @@ 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)
{
+#ifdef PCI_IOBASE
readsw(PCI_IOBASE + addr, buffer, count);
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -622,7 +654,11 @@ 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)
{
+#ifdef PCI_IOBASE
readsl(PCI_IOBASE + addr, buffer, count);
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -631,7 +667,11 @@ 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)
{
+#ifdef PCI_IOBASE
writesb(PCI_IOBASE + addr, buffer, count);
+#else
+ WARN_ONCE(1, "No I/O port support\n");
+#endif
}
#endif
@@ -640,7 +680,11 @@ static inline void outsb(unsigned long addr, const void
*buffer,
static inline void outsw(unsigned long addr, const void *buffer,
unsigned int coun
[PATCH v4 2/3] risc-v: Use generic io.h helpers for nommu
From: Niklas Schnelle Without MMU support PCI_IOBASE is left undefined because PCI_IO_END is VMEMMAP_START. Nevertheless the in*()/out*() helper macros are left defined with uses of PCI_IOBASE. At the moment this only compiles because asm-generic/io.h defines PCI_IOBASE as 0 if it is undefined and so at macro expansion the macro is defined. This is suprising at the least and leads to compilation errors when asm-generic/io.h is changed to leave PCI_IOBASE undefined. Instead only define the in*()/out*() helper macros with MMU support and fall back to the asm-generic/io.h helper stubs otherwise. Signed-off-by: Niklas Schnelle --- arch/riscv/include/asm/io.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h index c025a746a148..31a8b98c0f13 100644 --- a/arch/riscv/include/asm/io.h +++ b/arch/riscv/include/asm/io.h @@ -23,12 +23,12 @@ #include /* - * I/O port access constants. + * I/O port access constants. Without MMU support leave PCI_IOBASE undefined + * and fall back to generic stubs for I/O access routines. */ #ifdef CONFIG_MMU #define IO_SPACE_LIMIT (PCI_IO_SIZE - 1) #define PCI_IOBASE ((void __iomem *)PCI_IO_START) -#endif /* CONFIG_MMU */ /* * Emulation routines for the port-mapped IO space used by some PCI drivers. @@ -145,6 +145,7 @@ __io_writes_outs(writes, u64, q, __io_bw(), __io_aw()) __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw()) #define outsq(addr, buffer, count) __outsq((void __iomem *)addr, buffer, count) #endif +#endif /* CONFIG_MMU */ #include -- 2.31.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v4 0/3] asm-generic/io.h: Silence -Wnull-pointer-arithmetic warning on PCI_IOBASE
From: Niklas Schnelle Hi, This is version 4 of my attempt to get rid of a clang -Wnull-pointer-arithmetic warning for the use of PCI_IOBASE in asm-generic/io.h. This was originally found on s390 but should apply to all platforms leaving PCI_IOBASE undefined while making use of the inb() and friends helpers from asm-generic/io.h. This applies cleanly and was compile tested on top of v5.12 for the previously broken ARC, nds32, h8300 and risc-v architecture I did boot test this only on x86_64 and s390x the former implements inb() itself while the latter would emit a WARN_ONCE() but no drivers use inb(). Thanks, Niklas Changes since v3: - Changed the subject of the last patch to better reflect the actual change i.e. the addition of WARN_ONCE() to the helpers not the silencing of the clang warning - Added asm/bug.h to asm-generic/io.h so it doesn't have to be included previously by all arches to be available for the WARN_ONCE() - Added patch for risc-v which defines PCI_IOBASE except when compiled for nommu Changes since v2: - Improved comment for SPARC PCI_IOBASE definition as suggested by David Laight - Added a patch for ARC which is missing the asm/bug.h include for WARN_ONCE() (kernel test robot) - Added ifdefs to ioport_map() and __pci_ioport_map() since apparently at least test configs enable CONFIG_HAS_IOPORT_MAP even on architectures which leave PCI_IOBASE unset (kernel test robot for nds32 and ARC). Changes since v1: - Added patch to explicitly set PCI_IOBASE to 0 on sparc as suggested by Arnd Bergmann - Instead of working around the warning with a uintptr_t PCI_IOBASE make inb() and friends explicitly WARN_ONCE() and return 0xff... (Arnd Bergmann) Niklas Schnelle (3): sparc: explicitly set PCI_IOBASE to 0 risc-v: Use generic io.h helpers for nommu asm-generic/io.h: warn in inb() and friends with undefined PCI_IOBASE arch/riscv/include/asm/io.h | 5 +-- arch/sparc/include/asm/io.h | 8 + include/asm-generic/io.h| 65 ++--- 3 files changed, 72 insertions(+), 6 deletions(-) -- 2.31.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH v4 1/3] sparc: explicitly set PCI_IOBASE to 0
Instead of relying on the fallback in asm-generic/io.h which sets PCI_IOBASE 0 if it is not defined set it explicitly. Link: https://lore.kernel.org/lkml/CAK8P3a3PK9zyeP4ymELtc2ZYnymECoACiigw9Za+pvSJpCk5=g...@mail.gmail.com/ Signed-off-by: Niklas Schnelle --- arch/sparc/include/asm/io.h | 8 1 file changed, 8 insertions(+) diff --git a/arch/sparc/include/asm/io.h b/arch/sparc/include/asm/io.h index 2eefa526b38f..c019e50702c1 100644 --- a/arch/sparc/include/asm/io.h +++ b/arch/sparc/include/asm/io.h @@ -1,6 +1,14 @@ /* SPDX-License-Identifier: GPL-2.0 */ #ifndef ___ASM_SPARC_IO_H #define ___ASM_SPARC_IO_H + +/* + * On LEON PCI addresses below 64k are converted to IO accesses. + * io_remap_xxx() returns a kernel virtual address in the PCI window so + * inb() doesn't need to add an offset. + */ +#define PCI_IOBASE ((void __iomem *)0) + #if defined(__sparc__) && defined(__arch64__) #include #else -- 2.31.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Heads up: gcc miscompiling initramfs zlib decompression code at -O3
On Fri, Apr 30, 2021 at 1:46 PM Vineet Gupta wrote: > > I've hit a mainline gcc 10.2 (also gcc 9.3) bug which triggers at -O3 > causing wrong codegen. I'd be more than happy to just disable CC_OPTIMIZE_FOR_PERFORMANCE_O3 entirely. The advantages are very questionable - with a lot of the optimizations at O3 being about loops, something which the kernel to a close approximation doesn't have. Most kernel loops are "count on one hand" iterations, and loop optimizations generally just make things worse. And we've had problems with -O3 before, because not only are the optimizations a bit esoteric, they are often relatively untested. If you look around at various projects (outside the kernel), -O2 is generally the "default". And that's entirely ignoring the gcc history - where -O3 has often been very buggy indeed. It's gotten much better, but I just don't see the upside of using -O3. In fact, it looks like we already have that depends on ARC for -O3, exactly because nobody really wants to use this. So this bug seems to be entirely ARC-specific, in that only ARC can use -O3 for the kernel already. Linus ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Heads up: gcc miscompiling initramfs zlib decompression code at -O3
On Fri, Apr 30, 2021 at 3:44 PM Vineet Gupta wrote: > > I agree that -O2 is default, but we've had -O3 default for ARC kernel > forever, since last decade seriously. The reason I turned it on back > then was upside of 10% performance improvement on select LMBench numbers > on hardware at the time which for a rookie kernel hacker was yay momemt. > I can revisit this and see if that is still true. It would be interesting if you can actually show 10% improvement, and also pinpoint things in the profile. I (long long long ago) actually used -O6 for kernel builds as a "give me everything you have" (I don't think gcc has actually ever done anything more than O3, but whatever). In fact, you can find Q: What Does gcc -O6 Do? in some kernel FAQ's from those historical times. We eventually gave up on it, because it just generated bigger and slower code, and people got very tired of all the compiler bugs. Linus ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Heads up: gcc miscompiling initramfs zlib decompression code at -O3
Hi,
I've hit a mainline gcc 10.2 (also gcc 9.3) bug which triggers at -O3
causing wrong codegen.
Config needs to have initramfs + gzip compressed.
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_KERNEL_GZIP=y
CONFIG_DECOMPRESS_GZIP=y
CONFIG_INITRAMFS_COMPRESSION_GZIP=y
lib/zlib_inflate/inffast.c
if (dist > 2) {
unsigned short *sfrom;
sfrom = (unsigned short *)(from);
loops = len >> 1;
do
*sout++ = *sfrom++;
^^
while (--loops);
out = (unsigned char *)sout;
from = (unsigned char *)sfrom;
}
...
The gist of issue is that despite use of unsigned short pointers, gcc is
generating wider load/stores (8-byte LDD/STD on arcv2 and 16-byte on
aarch64) causing extraneous bytes to copied into inflated gzip binaries
manifesting later as corrupted fragments in the binaries.
I've opened a gcc bug at:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100363
The workaround is to build lib/zlib_inflate/inffast.c with -O2, although
I reckon not many arches build with -O3 as default. I'll be proposing an
ARC only patch to build this file with -O2, unless people think it needs
to be generalized.
Also problem originally seen on 5.6 kernel, although I confirm it shows
on latest mainline as well.
Unraveling this pretty fun, gory details for those interested at:
https://github.com/foss-for-synopsys-dwc-arc-processors/toolchain/issues/372
Thx,
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: Heads up: gcc miscompiling initramfs zlib decompression code at -O3
On 4/30/21 3:06 PM, Linus Torvalds wrote: > On Fri, Apr 30, 2021 at 1:46 PM Vineet Gupta > wrote: >> >> I've hit a mainline gcc 10.2 (also gcc 9.3) bug which triggers at -O3 >> causing wrong codegen. > > I'd be more than happy to just disable CC_OPTIMIZE_FOR_PERFORMANCE_O3 > entirely. > > The advantages are very questionable - with a lot of the optimizations > at O3 being about loops, something which the kernel to a close > approximation doesn't have. > > Most kernel loops are "count on one hand" iterations, and loop > optimizations generally just make things worse. > > And we've had problems with -O3 before, because not only are the > optimizations a bit esoteric, they are often relatively untested. If > you look around at various projects (outside the kernel), -O2 is > generally the "default". I agree that -O2 is default, but we've had -O3 default for ARC kernel forever, since last decade seriously. The reason I turned it on back then was upside of 10% performance improvement on select LMBench numbers on hardware at the time which for a rookie kernel hacker was yay momemt. I can revisit this and see if that is still true. > And that's entirely ignoring the gcc history - where -O3 has often > been very buggy indeed. It's gotten much better, but I just don't see > the upside of using -O3. > > In fact, it looks like we already have that > > depends on ARC > > for -O3, exactly because nobody really wants to use this. Either that or that people are not brave enough ;-) Perhaps gcc folks would like me to retain this as a testing ground if nothing else. > So this bug seems to be entirely ARC-specific, in that only ARC can > use -O3 for the kernel already. kid in me complaining "that's not fair !" -Vineet ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
