| Message ID | 20250411023408.185150-1-liurunrun@uniontech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Fix PCI I/O port addressing for MMU-less configurations | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | warning | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On Fri, Apr 11, 2025, at 04:34, Liu Runrun wrote: > This patch addresses the PCI I/O port address handling in RISC-V's > port-mapped I/O emulation routines when the MMU is not enabled. > The changes ensure that: Do you have a system that requires this? I sent a patch the other day to make PCI 'depends on MMU', based on how nothing today uses it. Having a NOMMU system with PCI wounds rather silly, so I hope we don't ever get that. > 1. For non-MMU systems, the PCI I/O port addresses are properly > calculated in marcos inX and outX when PCI_IOBASE is not > defined, this avoids the null pointer calculating warning > from the compiler. This is the wrong way around: the warning tells you that you have failed to configure PCI_IOBASE for the particular hardware, and that you actually get a NULL pointer dereference. The solution is not to shut up the warning but making it not a NULL pointer dereference! Part of the issue is that historically the asm-generic/io.h header includes an incorrect fallback of PCI_IOBASE when the architecture does not provide the correct one. I think only sparc still relies on that, so that fallback definition should be moved into arch/sparc/include/asm/io_{32,64}.h instead. > 2. In asm-generic/io.h, function ioport_map(), casting PCI_IOPORT > to type "long" firstly makes it could compute with variable addr > directly, which avoids the null pointer calculating warning when > PCI_IOPORT is a null pointer in some case. I don't understand that sentence, please rephrase. > The original implementation used `PCI_IOBASE + (addr)` for MMU-enabled > systems, but failed to handle non-MMU cases correctly. This change adds > conditional compilation guards (#ifdef CONFIG_MMU) to differentiate > between MMU and non-MMU environments, providing consistent behavior > for both scenarios. This also looks wrong: what you are distinguishing here is systems with (potentially) I/O port support and those that never have I/O port support. > diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h > index a0e51840b9db..d5181bb02c98 100644 > --- a/arch/riscv/include/asm/io.h > +++ b/arch/riscv/include/asm/io.h > @@ -101,9 +101,15 @@ __io_reads_ins(reads, u32, l, __io_br(), > __io_ar(addr)) > __io_reads_ins(ins, u8, b, __io_pbr(), __io_par(addr)) > __io_reads_ins(ins, u16, w, __io_pbr(), __io_par(addr)) > __io_reads_ins(ins, u32, l, __io_pbr(), __io_par(addr)) > +#ifdef CONFIG_MMU > #define insb(addr, buffer, count) __insb(PCI_IOBASE + (addr), buffer, > count) > #define insw(addr, buffer, count) __insw(PCI_IOBASE + (addr), buffer, > count) > #define insl(addr, buffer, count) __insl(PCI_IOBASE + (addr), buffer, > count) I see that these are defined unconditionally here, which is probably the real mistake. What I think we need instead is to enclose them in "#ifdef CONFIG_HAS_IOPORT" with no "#else" block, and ensure that HAS_IOPORT is only set when building for targets that have a sensible definition of PCI_IOBASE and IO_SPACE_LIMIT. The same approach is used in asm-generic/io.h, we just haven't done it for all architectures yet, since it's not entirely clear which ones can support ISA bridges and legacy PCI devices with port I/O. Ideally we'd go through the individual PCI host driver implementations and only enable HAS_IOPORT for those that are can handle the I/O space window correctly, but leave it out if none of them are selected. Arnd
diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h index a0e51840b9db..d5181bb02c98 100644 --- a/arch/riscv/include/asm/io.h +++ b/arch/riscv/include/asm/io.h @@ -101,9 +101,15 @@ __io_reads_ins(reads, u32, l, __io_br(), __io_ar(addr)) __io_reads_ins(ins, u8, b, __io_pbr(), __io_par(addr)) __io_reads_ins(ins, u16, w, __io_pbr(), __io_par(addr)) __io_reads_ins(ins, u32, l, __io_pbr(), __io_par(addr)) +#ifdef CONFIG_MMU #define insb(addr, buffer, count) __insb(PCI_IOBASE + (addr), buffer, count) #define insw(addr, buffer, count) __insw(PCI_IOBASE + (addr), buffer, count) #define insl(addr, buffer, count) __insl(PCI_IOBASE + (addr), buffer, count) +#else +#define insb(addr, buffer, count) __insb((void __iomem *)(long)addr, buffer, count) +#define insw(addr, buffer, count) __insw((void __iomem *)(long)addr, buffer, count) +#define insl(addr, buffer, count) __insl((void __iomem *)(long)addr, buffer, count) +#endif /* CONFIG_MMU */ __io_writes_outs(writes, u8, b, __io_bw(), __io_aw()) __io_writes_outs(writes, u16, w, __io_bw(), __io_aw()) @@ -115,23 +121,37 @@ __io_writes_outs(writes, u32, l, __io_bw(), __io_aw()) __io_writes_outs(outs, u8, b, __io_pbw(), __io_paw()) __io_writes_outs(outs, u16, w, __io_pbw(), __io_paw()) __io_writes_outs(outs, u32, l, __io_pbw(), __io_paw()) +#ifdef CONFIG_MMU #define outsb(addr, buffer, count) __outsb(PCI_IOBASE + (addr), buffer, count) #define outsw(addr, buffer, count) __outsw(PCI_IOBASE + (addr), buffer, count) #define outsl(addr, buffer, count) __outsl(PCI_IOBASE + (addr), buffer, count) +#else +#define outsb(addr, buffer, count) __outsb((void __iomem *)(long)addr, buffer, count) +#define outsw(addr, buffer, count) __outsw((void __iomem *)(long)addr, buffer, count) +#define outsl(addr, buffer, count) __outsl((void __iomem *)(long)addr, buffer, count) +#endif /* CONFIG_MMU */ #ifdef CONFIG_64BIT __io_reads_ins(reads, u64, q, __io_br(), __io_ar(addr)) #define readsq(addr, buffer, count) __readsq(addr, buffer, count) __io_reads_ins(ins, u64, q, __io_pbr(), __io_par(addr)) +#ifdef CONFIG_MMU #define insq(addr, buffer, count) __insq(PCI_IOBASE + (addr), buffer, count) +#else +#define insq(addr, buffer, count) __insq((void __iomem *)(long)addr, buffer, count) +#endif /* CONFIG_MMU */ __io_writes_outs(writes, u64, q, __io_bw(), __io_aw()) #define writesq(addr, buffer, count) __writesq(addr, buffer, count) __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw()) +#ifdef CONFIG_MMU #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count) -#endif +#else +#define outsq(addr, buffer, count) __outsq((void __iomem *)(long)addr, buffer, count) +#endif /* CONFIG_MMU */ +#endif /* CONFIG_64BIT */ #include <asm-generic/io.h> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 11abad6c87e1..c0409188bb5e 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -1172,7 +1172,8 @@ static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size) static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) { port &= IO_SPACE_LIMIT; - return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; + return (port > MMIO_UPPER_LIMIT) ? + NULL : (void __iomem *)((unsigned long)PCI_IOBASE + port); } #define ARCH_HAS_GENERIC_IOPORT_MAP #endif
This patch addresses the PCI I/O port address handling in RISC-V's port-mapped I/O emulation routines when the MMU is not enabled. The changes ensure that: 1. For non-MMU systems, the PCI I/O port addresses are properly calculated in marcos inX and outX when PCI_IOBASE is not defined, this avoids the null pointer calculating warning from the compiler. 2. In asm-generic/io.h, function ioport_map(), casting PCI_IOPORT to type "long" firstly makes it could compute with variable addr directly, which avoids the null pointer calculating warning when PCI_IOPORT is a null pointer in some case. The original implementation used `PCI_IOBASE + (addr)` for MMU-enabled systems, but failed to handle non-MMU cases correctly. This change adds conditional compilation guards (#ifdef CONFIG_MMU) to differentiate between MMU and non-MMU environments, providing consistent behavior for both scenarios. Fixes: 9cc205e3c17d ("RISC-V: Make port I/O string accessors actually work") Reported-by: WangYuli <wangyuli@uniontech.com> Signed-off-by: Liu Runrun <liurunrun@uniontech.com> --- arch/riscv/include/asm/io.h | 22 +++++++++++++++++++++- include/asm-generic/io.h | 3 ++- 2 files changed, 23 insertions(+), 2 deletions(-)