| Message ID | 20250315105907.1275012-6-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | asm-generic: io.h cleanups | expand |
Hi Arnd, On Sat, Mar 15, 2025 at 11:59:06AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > All PIO on MIPS platforms is memory mapped, so there is no benefit in > the lib/iomap.c wrappers that switch between inb/outb and readb/writeb > style accessses. > > In fact, the '#define PIO_RESERVED 0' setting completely disables > the GENERIC_IOMAP functionality, and the '#define PIO_OFFSET > mips_io_port_base' setting is based on a misunderstanding of what the > offset is meant to do. > > MIPS started using GENERIC_IOMAP in 2018 with commit b962aeb02205 ("MIPS: > Use GENERIC_IOMAP") replacing a simple custom implementation of the same > interfaces, but at the time the asm-generic/io.h version was not usable > yet. Since the header is now always included, it's now possible to go > back to the even simpler version. > > Use the normal GENERIC_PCI_IOMAP functionality for all mips platforms > without the hacky GENERIC_IOMAP, and provide a custom pci_iounmap() > for the CONFIG_PCI_DRIVERS_LEGACY case to ensure the I/O port base never > gets unmapped. > > The readsl() prototype needs an extra 'const' keyword to make it > compatible with the generic ioread32_rep() alias. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/mips/Kconfig | 2 +- > arch/mips/include/asm/io.h | 21 ++++++++------------- > arch/mips/lib/iomap-pci.c | 9 +++++++++ > 3 files changed, 18 insertions(+), 14 deletions(-) > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index 1924f2d83932..2a2120a6d852 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -38,7 +38,6 @@ config MIPS > select GENERIC_CMOS_UPDATE > select GENERIC_CPU_AUTOPROBE > select GENERIC_GETTIMEOFDAY > - select GENERIC_IOMAP > select GENERIC_IRQ_PROBE > select GENERIC_IRQ_SHOW > select GENERIC_ISA_DMA if EISA > @@ -47,6 +46,7 @@ config MIPS > select GENERIC_LIB_CMPDI2 > select GENERIC_LIB_LSHRDI3 > select GENERIC_LIB_UCMPDI2 > + select GENERIC_PCI_IOMAP > select GENERIC_SCHED_CLOCK if !CAVIUM_OCTEON_SOC > select GENERIC_SMP_IDLE_THREAD > select GENERIC_IDLE_POLL_SETUP > diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h > index 0bddb568af7c..1fe56d1870a6 100644 > --- a/arch/mips/include/asm/io.h > +++ b/arch/mips/include/asm/io.h > @@ -66,17 +66,6 @@ static inline void set_io_port_base(unsigned long base) > mips_io_port_base = base; > } > > -/* > - * Provide the necessary definitions for generic iomap. We make use of > - * mips_io_port_base for iomap(), but we don't reserve any low addresses for > - * use with I/O ports. > - */ > - > -#define HAVE_ARCH_PIO_SIZE > -#define PIO_OFFSET mips_io_port_base > -#define PIO_MASK IO_SPACE_LIMIT > -#define PIO_RESERVED 0x0UL > - > /* > * Enforce in-order execution of data I/O. In the MIPS architecture > * these are equivalent to corresponding platform-specific memory > @@ -397,8 +386,8 @@ static inline void writes##bwlq(volatile void __iomem *mem, \ > } \ > } \ > \ > -static inline void reads##bwlq(volatile void __iomem *mem, void *addr, \ > - unsigned int count) \ > +static inline void reads##bwlq(const volatile void __iomem *mem, \ > + void *addr, unsigned int count) \ > { \ > volatile type *__addr = addr; \ > \ > @@ -555,6 +544,12 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size); > > void __ioread64_copy(void *to, const void __iomem *from, size_t count); > > +#ifdef CONFIG_PCI_DRIVERS_LEGACY > +struct pci_dev; > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr); > +#define pci_iounmap pci_iounmap > +#endif > + > #include <asm-generic/io.h> > > static inline void *isa_bus_to_virt(unsigned long address) > diff --git a/arch/mips/lib/iomap-pci.c b/arch/mips/lib/iomap-pci.c > index a9cb28813f0b..2f82c776c6d0 100644 > --- a/arch/mips/lib/iomap-pci.c > +++ b/arch/mips/lib/iomap-pci.c > @@ -43,4 +43,13 @@ void __iomem *__pci_ioport_map(struct pci_dev *dev, > return (void __iomem *) (ctrl->io_map_base + port); > } > > +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) > +{ > + struct pci_controller *ctrl = dev->bus->sysdata; > + void __iomem *base = (void __iomem *)ctrl->io_map_base; > + > + if (addr < base || addr > (base + resource_size(ctrl->io_resource))) > + iounmap(addr); > +} > + > #endif /* CONFIG_PCI_DRIVERS_LEGACY */ > -- > 2.39.5 > This change as commit 976bf3aec388 ("mips: drop GENERIC_IOMAP wrapper") in -next introduces new instances of -Wnull-pointer-arithmetic when building certain mips configurations with clang. $ make -skj"$(nproc)" ARCH=mips LLVM=1 mrproper malta_defconfig init/main.o ... In file included from init/main.c:17: In file included from include/linux/module.h:17: In file included from include/linux/kmod.h:9: In file included from include/linux/umh.h:4: In file included from include/linux/gfp.h:7: In file included from include/linux/mmzone.h:22: In file included from include/linux/mm_types.h:5: In file included from include/linux/mm_types_task.h:14: In file included from arch/mips/include/asm/page.h:181: In file included from arch/mips/include/asm/io.h:553: include/asm-generic/io.h:1175:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 1175 | return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; | ~~~~~~~~~~ ^ 1 warning generated. Cheers, Nathan
On Tue, Mar 18, 2025, at 21:39, Nathan Chancellor wrote: > On Sat, Mar 15, 2025 at 11:59:06AM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h >> void __ioread64_copy(void *to, const void __iomem *from, size_t count); >> >> +#ifdef CONFIG_PCI_DRIVERS_LEGACY >> +struct pci_dev; >> +void pci_iounmap(struct pci_dev *dev, void __iomem *addr); >> +#define pci_iounmap pci_iounmap >> +#endif >> + >> #include <asm-generic/io.h> >> >> static inline void *isa_bus_to_virt(unsigned long address) >> diff --git a/arch/mips/lib/iomap-pci.c b/arch/mips/lib/iomap-pci.c >> index a9cb28813f0b..2f82c776c6d0 100644 >> --- a/arch/mips/lib/iomap-pci.c >> +++ b/arch/mips/lib/iomap-pci.c >> @@ -43,4 +43,13 @@ void __iomem *__pci_ioport_map(struct pci_dev *dev, >> return (void __iomem *) (ctrl->io_map_base + port); >> } >> >> +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) >> +{ >> + struct pci_controller *ctrl = dev->bus->sysdata; >> + void __iomem *base = (void __iomem *)ctrl->io_map_base; >> + >> + if (addr < base || addr > (base + resource_size(ctrl->io_resource))) >> + iounmap(addr); >> +} >> + >> #endif /* CONFIG_PCI_DRIVERS_LEGACY */ >> -- >> 2.39.5 >> > > This change as commit 976bf3aec388 ("mips: drop GENERIC_IOMAP wrapper") in > -next introduces new instances of -Wnull-pointer-arithmetic when building > certain mips configurations with clang. > Thanks for the report, I missed that the generic ioport_map() function is missing the PCI_IOBASE macro, we should probably remove that from the asm-generic/io.h header and require architectures to define it themselves, since the NULL fallback is pretty much always wrong. There is also a type mismatch between the MIPS PCI_IOBASE/mips_io_port_base and the one that asm-generic/io.h expects, so I had to add a couple of extra typecasts, which makes it rather ugly, but the change below seems to work. Arnd diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h index 1fe56d1870a6..78c6573f91f2 100644 --- a/arch/mips/include/asm/io.h +++ b/arch/mips/include/asm/io.h @@ -544,12 +544,16 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size); void __ioread64_copy(void *to, const void __iomem *from, size_t count); -#ifdef CONFIG_PCI_DRIVERS_LEGACY +#if defined(CONFIG_PCI) && defined(CONFIG_PCI_DRIVERS_LEGACY) struct pci_dev; void pci_iounmap(struct pci_dev *dev, void __iomem *addr); #define pci_iounmap pci_iounmap #endif +#ifndef PCI_IOBASE +#define PCI_IOBASE ((void __iomem *)mips_io_port_base) +#endif + #include <asm-generic/io.h> static inline void *isa_bus_to_virt(unsigned long address) diff --git a/arch/mips/include/asm/mach-loongson64/spaces.h b/arch/mips/include/asm/mach-loongson64/spaces.h index ce04e998a37b..dbd26db5f2c5 100644 --- a/arch/mips/include/asm/mach-loongson64/spaces.h +++ b/arch/mips/include/asm/mach-loongson64/spaces.h @@ -7,9 +7,10 @@ #endif /* CONFIG_64BIT */ /* Skip 128k to trap NULL pointer dereferences */ -#define PCI_IOBASE _AC(0xc000000000000000 + SZ_128K, UL) +#define PCI_PORT_BASE _AC(0xc000000000000000 + SZ_128K, UL) +#define PCI_IOBASE (void __iomem *)PCI_PORT_BASE #define PCI_IOSIZE SZ_16M -#define MAP_BASE (PCI_IOBASE + PCI_IOSIZE) +#define MAP_BASE (PCI_PORT_BASE + PCI_IOSIZE) #define IO_SPACE_LIMIT (PCI_IOSIZE - 1) diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h index a9f0570d0f04..a63d106c89c6 100644 --- a/arch/mips/include/asm/mach-ralink/spaces.h +++ b/arch/mips/include/asm/mach-ralink/spaces.h @@ -2,7 +2,7 @@ #ifndef __ASM_MACH_RALINK_SPACES_H_ #define __ASM_MACH_RALINK_SPACES_H_ -#define PCI_IOBASE mips_io_port_base +#define PCI_IOBASE (void __iomem *)mips_io_port_base #define PCI_IOSIZE SZ_64K #define IO_SPACE_LIMIT (PCI_IOSIZE - 1) diff --git a/arch/mips/loongson64/init.c b/arch/mips/loongson64/init.c index a35dd7311795..b9f90f33fc9a 100644 --- a/arch/mips/loongson64/init.c +++ b/arch/mips/loongson64/init.c @@ -128,7 +128,7 @@ void __init prom_init(void) } /* init base address of io space */ - set_io_port_base(PCI_IOBASE); + set_io_port_base((unsigned long)PCI_IOBASE); if (loongson_sysconf.early_config) loongson_sysconf.early_config(); @@ -178,7 +178,7 @@ static int __init add_legacy_isa_io(struct fwnode_handle *fwnode, resource_size_ return -EINVAL; } - vaddr = PCI_IOBASE + range->io_start; + vaddr = (unsigned long)PCI_IOBASE + range->io_start; vmap_page_range(vaddr, vaddr + size, hw_start, pgprot_device(PAGE_KERNEL));
On Tue, Mar 18, 2025 at 10:13:35PM +0100, Arnd Bergmann wrote: > Thanks for the report, I missed that the generic ioport_map() function > is missing the PCI_IOBASE macro, we should probably remove that from > the asm-generic/io.h header and require architectures to define it > themselves, since the NULL fallback is pretty much always wrong. > > There is also a type mismatch between the MIPS > PCI_IOBASE/mips_io_port_base and the one that asm-generic/io.h > expects, so I had to add a couple of extra typecasts, which > makes it rather ugly, but the change below seems to work. Thanks, that does make the -Wnull-pointer-arithmetic warnings disappear. That build still fails in next-20250319 (which includes that change) at the end with: $ make -skj"$(nproc)" ARCH=mips CROSS_COMPILE=mips-linux- mrproper malta_defconfig all ERROR: modpost: "pci_iounmap" [drivers/net/wireless/intel/ipw2x00/ipw2100.ko] undefined! which appears related to this original change. Cheers, Nathan
On Wed, Mar 19, 2025, at 18:30, Nathan Chancellor wrote: > On Tue, Mar 18, 2025 at 10:13:35PM +0100, Arnd Bergmann wrote: >> Thanks for the report, I missed that the generic ioport_map() function >> is missing the PCI_IOBASE macro, we should probably remove that from >> the asm-generic/io.h header and require architectures to define it >> themselves, since the NULL fallback is pretty much always wrong. >> >> There is also a type mismatch between the MIPS >> PCI_IOBASE/mips_io_port_base and the one that asm-generic/io.h >> expects, so I had to add a couple of extra typecasts, which >> makes it rather ugly, but the change below seems to work. > > Thanks, that does make the -Wnull-pointer-arithmetic warnings disappear. > That build still fails in next-20250319 (which includes that change) at > the end with: > > $ make -skj"$(nproc)" ARCH=mips CROSS_COMPILE=mips-linux- mrproper > malta_defconfig all > ERROR: modpost: "pci_iounmap" > [drivers/net/wireless/intel/ipw2x00/ipw2100.ko] undefined! > > which appears related to this original change. Right, I had seen and fixed a problem like this in an earlier version, but forgot the EXPORT_SYMBOL on the legacy host version. Fixed it better now. Arnd
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 1924f2d83932..2a2120a6d852 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -38,7 +38,6 @@ config MIPS select GENERIC_CMOS_UPDATE select GENERIC_CPU_AUTOPROBE select GENERIC_GETTIMEOFDAY - select GENERIC_IOMAP select GENERIC_IRQ_PROBE select GENERIC_IRQ_SHOW select GENERIC_ISA_DMA if EISA @@ -47,6 +46,7 @@ config MIPS select GENERIC_LIB_CMPDI2 select GENERIC_LIB_LSHRDI3 select GENERIC_LIB_UCMPDI2 + select GENERIC_PCI_IOMAP select GENERIC_SCHED_CLOCK if !CAVIUM_OCTEON_SOC select GENERIC_SMP_IDLE_THREAD select GENERIC_IDLE_POLL_SETUP diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h index 0bddb568af7c..1fe56d1870a6 100644 --- a/arch/mips/include/asm/io.h +++ b/arch/mips/include/asm/io.h @@ -66,17 +66,6 @@ static inline void set_io_port_base(unsigned long base) mips_io_port_base = base; } -/* - * Provide the necessary definitions for generic iomap. We make use of - * mips_io_port_base for iomap(), but we don't reserve any low addresses for - * use with I/O ports. - */ - -#define HAVE_ARCH_PIO_SIZE -#define PIO_OFFSET mips_io_port_base -#define PIO_MASK IO_SPACE_LIMIT -#define PIO_RESERVED 0x0UL - /* * Enforce in-order execution of data I/O. In the MIPS architecture * these are equivalent to corresponding platform-specific memory @@ -397,8 +386,8 @@ static inline void writes##bwlq(volatile void __iomem *mem, \ } \ } \ \ -static inline void reads##bwlq(volatile void __iomem *mem, void *addr, \ - unsigned int count) \ +static inline void reads##bwlq(const volatile void __iomem *mem, \ + void *addr, unsigned int count) \ { \ volatile type *__addr = addr; \ \ @@ -555,6 +544,12 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size); void __ioread64_copy(void *to, const void __iomem *from, size_t count); +#ifdef CONFIG_PCI_DRIVERS_LEGACY +struct pci_dev; +void pci_iounmap(struct pci_dev *dev, void __iomem *addr); +#define pci_iounmap pci_iounmap +#endif + #include <asm-generic/io.h> static inline void *isa_bus_to_virt(unsigned long address) diff --git a/arch/mips/lib/iomap-pci.c b/arch/mips/lib/iomap-pci.c index a9cb28813f0b..2f82c776c6d0 100644 --- a/arch/mips/lib/iomap-pci.c +++ b/arch/mips/lib/iomap-pci.c @@ -43,4 +43,13 @@ void __iomem *__pci_ioport_map(struct pci_dev *dev, return (void __iomem *) (ctrl->io_map_base + port); } +void pci_iounmap(struct pci_dev *dev, void __iomem *addr) +{ + struct pci_controller *ctrl = dev->bus->sysdata; + void __iomem *base = (void __iomem *)ctrl->io_map_base; + + if (addr < base || addr > (base + resource_size(ctrl->io_resource))) + iounmap(addr); +} + #endif /* CONFIG_PCI_DRIVERS_LEGACY */