| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6c7770dd1c216410fcff3bf0758a45d5afcb5444.camel@physik.fu-berlin.de>
Date: Sat, 07 Jun 2025 14:08:26 +0200
From: John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
To: Arnd Bergmann <arnd@...nel.org>, linux-arch@...r.kernel.org
Cc: Arnd Bergmann <arnd@...db.de>, Richard Henderson
<richard.henderson@...aro.org>, Matt Turner <mattst88@...il.com>, Geert
Uytterhoeven <geert@...ux-m68k.org>, Greg Ungerer <gerg@...ux-m68k.org>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>, "James E.J. Bottomley"
<James.Bottomley@...senPartnership.com>, Helge Deller <deller@....de>,
Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman
<mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>, Christophe
Leroy <christophe.leroy@...roup.eu>, Naveen N Rao <naveen@...nel.org>,
Yoshinori Sato <ysato@...rs.sourceforge.jp>, Rich Felker
<dalias@...c.org>, Julian Vetter <julian@...er-limits.org>, Bjorn Helgaas
<bhelgaas@...gle.com>, linux-alpha@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-m68k@...ts.linux-m68k.org,
linux-mips@...r.kernel.org, linux-parisc@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-sh@...r.kernel.org
Subject: Re: [PATCH 2/6] sh: remove duplicate ioread/iowrite helpers
Hi Arnd,
On Sat, 2025-03-15 at 11:59 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
>
> The ioread/iowrite functions on sh only do memory mapped I/O like the
> generic verion, and never map onto non-MMIO inb/outb variants, so they
> just add complexity. In particular, the use of asm-generic/iomap.h
> ties the declaration to the x86 implementation.
>
> Remove the custom versions and use the architecture-independent fallback
> code instead. Some of the calling conventions on sh are different here,
> so fix that by adding 'volatile' keywords where required by the generic
> implementation and change the cpg clock driver to no longer depend on
> the interesting choice of return types for ioread8/ioread16/ioread32.
>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> arch/sh/include/asm/io.h | 30 ++------
> arch/sh/kernel/Makefile | 3 -
> arch/sh/kernel/iomap.c | 162 ---------------------------------------
> arch/sh/kernel/ioport.c | 5 --
> arch/sh/lib/io.c | 4 +-
> drivers/sh/clk/cpg.c | 25 +++---
> 6 files changed, 21 insertions(+), 208 deletions(-)
> delete mode 100644 arch/sh/kernel/iomap.c
>
> diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
> index cf5eab840d57..0f663ebec700 100644
> --- a/arch/sh/include/asm/io.h
> +++ b/arch/sh/include/asm/io.h
> @@ -19,7 +19,6 @@
> #include <asm/machvec.h>
> #include <asm/page.h>
> #include <linux/pgtable.h>
> -#include <asm-generic/iomap.h>
>
> #define __IO_PREFIX generic
> #include <asm/io_generic.h>
> @@ -100,7 +99,7 @@ pfx##writes##bwlq(volatile void __iomem *mem, const void *addr, \
> } \
> } \
> \
> -static inline void pfx##reads##bwlq(volatile void __iomem *mem, \
> +static inline void pfx##reads##bwlq(const volatile void __iomem *mem, \
> void *addr, unsigned int count) \
> { \
> volatile type *__addr = addr; \
> @@ -114,37 +113,18 @@ static inline void pfx##reads##bwlq(volatile void __iomem *mem, \
> __BUILD_MEMORY_STRING(__raw_, b, u8)
> __BUILD_MEMORY_STRING(__raw_, w, u16)
>
> -void __raw_writesl(void __iomem *addr, const void *data, int longlen);
> -void __raw_readsl(const void __iomem *addr, void *data, int longlen);
> +void __raw_writesl(void volatile __iomem *addr, const void *data, int longlen);
> +void __raw_readsl(const volatile void __iomem *addr, void *data, int longlen);
>
> __BUILD_MEMORY_STRING(__raw_, q, u64)
>
> #define ioport_map ioport_map
> -#define ioport_unmap ioport_unmap
> #define pci_iounmap pci_iounmap
>
> -#define ioread8 ioread8
> -#define ioread16 ioread16
> -#define ioread16be ioread16be
> -#define ioread32 ioread32
> -#define ioread32be ioread32be
> -
> -#define iowrite8 iowrite8
> -#define iowrite16 iowrite16
> -#define iowrite16be iowrite16be
> -#define iowrite32 iowrite32
> -#define iowrite32be iowrite32be
> -
> -#define ioread8_rep ioread8_rep
> -#define ioread16_rep ioread16_rep
> -#define ioread32_rep ioread32_rep
> -
> -#define iowrite8_rep iowrite8_rep
> -#define iowrite16_rep iowrite16_rep
> -#define iowrite32_rep iowrite32_rep
> -
> #ifdef CONFIG_HAS_IOPORT_MAP
>
> +extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
> +
> /*
> * Slowdown I/O port space accesses for antique hardware.
> */
> diff --git a/arch/sh/kernel/Makefile b/arch/sh/kernel/Makefile
> index ba917008d63e..7b453592adaf 100644
> --- a/arch/sh/kernel/Makefile
> +++ b/arch/sh/kernel/Makefile
> @@ -21,10 +21,7 @@ obj-y := head_32.o debugtraps.o dumpstack.o \
> syscalls_32.o time.o topology.o traps.o \
> traps_32.o unwinder.o
>
> -ifndef CONFIG_GENERIC_IOMAP
> -obj-y += iomap.o
> obj-$(CONFIG_HAS_IOPORT_MAP) += ioport.o
> -endif
>
> obj-y += sys_sh32.o
> obj-y += cpu/
> diff --git a/arch/sh/kernel/iomap.c b/arch/sh/kernel/iomap.c
> deleted file mode 100644
> index 0a0dff4e66de..000000000000
> --- a/arch/sh/kernel/iomap.c
> +++ /dev/null
> @@ -1,162 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * arch/sh/kernel/iomap.c
> - *
> - * Copyright (C) 2000 Niibe Yutaka
> - * Copyright (C) 2005 - 2007 Paul Mundt
> - */
> -#include <linux/module.h>
> -#include <linux/io.h>
> -
> -unsigned int ioread8(const void __iomem *addr)
> -{
> - return readb(addr);
> -}
> -EXPORT_SYMBOL(ioread8);
> -
> -unsigned int ioread16(const void __iomem *addr)
> -{
> - return readw(addr);
> -}
> -EXPORT_SYMBOL(ioread16);
> -
> -unsigned int ioread16be(const void __iomem *addr)
> -{
> - return be16_to_cpu(__raw_readw(addr));
> -}
> -EXPORT_SYMBOL(ioread16be);
> -
> -unsigned int ioread32(const void __iomem *addr)
> -{
> - return readl(addr);
> -}
> -EXPORT_SYMBOL(ioread32);
> -
> -unsigned int ioread32be(const void __iomem *addr)
> -{
> - return be32_to_cpu(__raw_readl(addr));
> -}
> -EXPORT_SYMBOL(ioread32be);
> -
> -void iowrite8(u8 val, void __iomem *addr)
> -{
> - writeb(val, addr);
> -}
> -EXPORT_SYMBOL(iowrite8);
> -
> -void iowrite16(u16 val, void __iomem *addr)
> -{
> - writew(val, addr);
> -}
> -EXPORT_SYMBOL(iowrite16);
> -
> -void iowrite16be(u16 val, void __iomem *addr)
> -{
> - __raw_writew(cpu_to_be16(val), addr);
> -}
> -EXPORT_SYMBOL(iowrite16be);
> -
> -void iowrite32(u32 val, void __iomem *addr)
> -{
> - writel(val, addr);
> -}
> -EXPORT_SYMBOL(iowrite32);
> -
> -void iowrite32be(u32 val, void __iomem *addr)
> -{
> - __raw_writel(cpu_to_be32(val), addr);
> -}
> -EXPORT_SYMBOL(iowrite32be);
> -
> -/*
> - * These are the "repeat MMIO read/write" functions.
> - * Note the "__raw" accesses, since we don't want to
> - * convert to CPU byte order. We write in "IO byte
> - * order" (we also don't have IO barriers).
> - */
> -static inline void mmio_insb(const void __iomem *addr, u8 *dst, int count)
> -{
> - while (--count >= 0) {
> - u8 data = __raw_readb(addr);
> - *dst = data;
> - dst++;
> - }
> -}
> -
> -static inline void mmio_insw(const void __iomem *addr, u16 *dst, int count)
> -{
> - while (--count >= 0) {
> - u16 data = __raw_readw(addr);
> - *dst = data;
> - dst++;
> - }
> -}
> -
> -static inline void mmio_insl(const void __iomem *addr, u32 *dst, int count)
> -{
> - while (--count >= 0) {
> - u32 data = __raw_readl(addr);
> - *dst = data;
> - dst++;
> - }
> -}
> -
> -static inline void mmio_outsb(void __iomem *addr, const u8 *src, int count)
> -{
> - while (--count >= 0) {
> - __raw_writeb(*src, addr);
> - src++;
> - }
> -}
> -
> -static inline void mmio_outsw(void __iomem *addr, const u16 *src, int count)
> -{
> - while (--count >= 0) {
> - __raw_writew(*src, addr);
> - src++;
> - }
> -}
> -
> -static inline void mmio_outsl(void __iomem *addr, const u32 *src, int count)
> -{
> - while (--count >= 0) {
> - __raw_writel(*src, addr);
> - src++;
> - }
> -}
> -
> -void ioread8_rep(const void __iomem *addr, void *dst, unsigned long count)
> -{
> - mmio_insb(addr, dst, count);
> -}
> -EXPORT_SYMBOL(ioread8_rep);
> -
> -void ioread16_rep(const void __iomem *addr, void *dst, unsigned long count)
> -{
> - mmio_insw(addr, dst, count);
> -}
> -EXPORT_SYMBOL(ioread16_rep);
> -
> -void ioread32_rep(const void __iomem *addr, void *dst, unsigned long count)
> -{
> - mmio_insl(addr, dst, count);
> -}
> -EXPORT_SYMBOL(ioread32_rep);
> -
> -void iowrite8_rep(void __iomem *addr, const void *src, unsigned long count)
> -{
> - mmio_outsb(addr, src, count);
> -}
> -EXPORT_SYMBOL(iowrite8_rep);
> -
> -void iowrite16_rep(void __iomem *addr, const void *src, unsigned long count)
> -{
> - mmio_outsw(addr, src, count);
> -}
> -EXPORT_SYMBOL(iowrite16_rep);
> -
> -void iowrite32_rep(void __iomem *addr, const void *src, unsigned long count)
> -{
> - mmio_outsl(addr, src, count);
> -}
> -EXPORT_SYMBOL(iowrite32_rep);
> diff --git a/arch/sh/kernel/ioport.c b/arch/sh/kernel/ioport.c
> index c8aff8a20164..915a3dfd9f02 100644
> --- a/arch/sh/kernel/ioport.c
> +++ b/arch/sh/kernel/ioport.c
> @@ -23,8 +23,3 @@ void __iomem *ioport_map(unsigned long port, unsigned int nr)
> return (void __iomem *)(port + sh_io_port_base);
> }
> EXPORT_SYMBOL(ioport_map);
> -
> -void ioport_unmap(void __iomem *addr)
> -{
> -}
> -EXPORT_SYMBOL(ioport_unmap);
> diff --git a/arch/sh/lib/io.c b/arch/sh/lib/io.c
> index ebcf7c0a7335..dc6345e4c53b 100644
> --- a/arch/sh/lib/io.c
> +++ b/arch/sh/lib/io.c
> @@ -11,7 +11,7 @@
> #include <linux/module.h>
> #include <linux/io.h>
>
> -void __raw_readsl(const void __iomem *addr, void *datap, int len)
> +void __raw_readsl(const volatile void __iomem *addr, void *datap, int len)
> {
> u32 *data;
>
> @@ -60,7 +60,7 @@ void __raw_readsl(const void __iomem *addr, void *datap, int len)
> }
> EXPORT_SYMBOL(__raw_readsl);
>
> -void __raw_writesl(void __iomem *addr, const void *data, int len)
> +void __raw_writesl(volatile void __iomem *addr, const void *data, int len)
> {
> if (likely(len != 0)) {
> int tmp1;
> diff --git a/drivers/sh/clk/cpg.c b/drivers/sh/clk/cpg.c
> index fd72d9088bdc..64ed7d64458a 100644
> --- a/drivers/sh/clk/cpg.c
> +++ b/drivers/sh/clk/cpg.c
> @@ -26,6 +26,19 @@ static unsigned int sh_clk_read(struct clk *clk)
> return ioread32(clk->mapped_reg);
> }
>
> +static unsigned int sh_clk_read_status(struct clk *clk)
> +{
> + void __iomem *mapped_status = (phys_addr_t)clk->status_reg -
> + (phys_addr_t)clk->enable_reg + clk->mapped_reg;
> +
> + if (clk->flags & CLK_ENABLE_REG_8BIT)
> + return ioread8(mapped_status);
> + else if (clk->flags & CLK_ENABLE_REG_16BIT)
> + return ioread16(mapped_status);
> +
> + return ioread32(mapped_status);
> +}
> +
> static void sh_clk_write(int value, struct clk *clk)
> {
> if (clk->flags & CLK_ENABLE_REG_8BIT)
> @@ -40,20 +53,10 @@ static int sh_clk_mstp_enable(struct clk *clk)
> {
> sh_clk_write(sh_clk_read(clk) & ~(1 << clk->enable_bit), clk);
> if (clk->status_reg) {
> - unsigned int (*read)(const void __iomem *addr);
> int i;
> - void __iomem *mapped_status = (phys_addr_t)clk->status_reg -
> - (phys_addr_t)clk->enable_reg + clk->mapped_reg;
> -
> - if (clk->flags & CLK_ENABLE_REG_8BIT)
> - read = ioread8;
> - else if (clk->flags & CLK_ENABLE_REG_16BIT)
> - read = ioread16;
> - else
> - read = ioread32;
>
> for (i = 1000;
> - (read(mapped_status) & (1 << clk->enable_bit)) && i;
> + (sh_clk_read_status(clk) & (1 << clk->enable_bit)) && i;
> i--)
> cpu_relax();
> if (!i) {
Those are quite a number of changes that I would like to test on real hardware
first before merging them into the kernel.
@Geert: Could you test it on your SH-7751 LANDISK board as well?
Thanks,
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Powered by blists - more mailing lists