[RFC PATCH 0/8] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way (Alternative)

[RFC PATCH 0/8] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way (Alternative)

[Christophe Leroy]

From: 

As proposed in the discussion related to your series, here comes an
exemple of how it could be.

I have taken it into ARC and IA64 architectures as an exemple. This is
untested, even not compiled, it is just to illustrated my meaning in the
discussion.

I also added a patch for powerpc architecture, that one in tested with
both pmac32_defconfig and ppc64_le_defconfig.

From my point of view, this different approach provide less churn and
less intellectual disturbance than the way you do it.

Open for discussion.

Baoquan He (5):
  hexagon: mm: Convert to GENERIC_IOREMAP
  openrisc: mm: remove unneeded early ioremap code
  mm: ioremap: allow ARCH to have its own ioremap definition
  arc: mm: Convert to GENERIC_IOREMAP
  ia64: mm: Convert to GENERIC_IOREMAP

Christophe Leroy (3):
  mm/ioremap: Define generic_ioremap_prot() and generic_iounmap()
  mm/ioremap: Consider IOREMAP space in generic ioremap
  powerpc: mm: Convert to GENERIC_IOREMAP

 arch/arc/Kconfig              |  1 +
 arch/arc/include/asm/io.h     |  7 +++---
 arch/arc/mm/ioremap.c         | 46 +++--------------------------------
 arch/hexagon/Kconfig          |  1 +
 arch/hexagon/include/asm/io.h |  9 +++++--
 arch/hexagon/mm/ioremap.c     | 44 ---------------------------------
 arch/ia64/Kconfig             |  1 +
 arch/ia64/include/asm/io.h    | 11 ++++++---
 arch/ia64/mm/ioremap.c        | 45 ++++++----------------------------
 arch/openrisc/mm/ioremap.c    | 22 ++++-------------
 arch/powerpc/Kconfig          |  1 +
 arch/powerpc/include/asm/io.h | 11 ++++++---
 arch/powerpc/mm/ioremap.c     | 26 +-------------------
 arch/powerpc/mm/ioremap_32.c  | 25 ++++++++-----------
 arch/powerpc/mm/ioremap_64.c  | 22 +++++++----------
 include/asm-generic/io.h      |  7 ++++++
 mm/ioremap.c                  | 33 +++++++++++++++++++------
 17 files changed, 98 insertions(+), 214 deletions(-)
 delete mode 100644 arch/hexagon/mm/ioremap.c

-- 
2.37.1

[RFC PATCH 1/8] hexagon: mm: Convert to GENERIC_IOREMAP

[Christophe Leroy]

From: Baoquan He <bhe@redhat.com>

By taking GENERIC_IOREMAP method, the generic ioremap_prot() and
iounmap() are visible and available to arch. Arch only needs to
provide implementation of arch_ioremap() or arch_iounmap() if there's
arch specific handling needed in its ioremap() or iounmap(). This
change will simplify implementation by removing duplicated codes with
generic ioremap() and iounmap(), and has the equivalent functioality.

For hexagon, the current ioremap() and iounmap() are the same as
generic version. After taking GENERIC_IOREMAP way, the old ioremap()
and iounmap() can be completely removed.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Brian Cain <bcain@quicinc.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: linux-hexagon@vger.kernel.org
---
 arch/hexagon/Kconfig          |  1 +
 arch/hexagon/include/asm/io.h |  9 +++++--
 arch/hexagon/mm/ioremap.c     | 44 -----------------------------------
 3 files changed, 8 insertions(+), 46 deletions(-)
 delete mode 100644 arch/hexagon/mm/ioremap.c

diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig
index 54eadf265178..17afffde1a7f 100644
--- a/arch/hexagon/Kconfig
+++ b/arch/hexagon/Kconfig
@@ -25,6 +25,7 @@ config HEXAGON
 	select NEED_SG_DMA_LENGTH
 	select NO_IOPORT_MAP
 	select GENERIC_IOMAP
+	select GENERIC_IOREMAP
 	select GENERIC_SMP_IDLE_THREAD
 	select STACKTRACE_SUPPORT
 	select GENERIC_CLOCKEVENTS_BROADCAST
diff --git a/arch/hexagon/include/asm/io.h b/arch/hexagon/include/asm/io.h
index 46a099de85b7..dcd9cbbf5934 100644
--- a/arch/hexagon/include/asm/io.h
+++ b/arch/hexagon/include/asm/io.h
@@ -170,8 +170,13 @@ static inline void writel(u32 data, volatile void __iomem *addr)
 #define writew_relaxed __raw_writew
 #define writel_relaxed __raw_writel
 
-void __iomem *ioremap(unsigned long phys_addr, unsigned long size);
-#define ioremap_uc(X, Y) ioremap((X), (Y))
+/*
+ * I/O memory mapping functions.
+ */
+#define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_READ | _PAGE_WRITE | \
+		       (__HEXAGON_C_DEV << 6))
+
+#define ioremap_uc(addr, size) ioremap((addr), (size))
 
 
 #define __raw_writel writel
diff --git a/arch/hexagon/mm/ioremap.c b/arch/hexagon/mm/ioremap.c
deleted file mode 100644
index 255c5b1ee1a7..000000000000
--- a/arch/hexagon/mm/ioremap.c
+++ /dev/null
@@ -1,44 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * I/O remap functions for Hexagon
- *
- * Copyright (c) 2010-2011, The Linux Foundation. All rights reserved.
- */
-
-#include <linux/io.h>
-#include <linux/vmalloc.h>
-#include <linux/mm.h>
-
-void __iomem *ioremap(unsigned long phys_addr, unsigned long size)
-{
-	unsigned long last_addr, addr;
-	unsigned long offset = phys_addr & ~PAGE_MASK;
-	struct vm_struct *area;
-
-	pgprot_t prot = __pgprot(_PAGE_PRESENT|_PAGE_READ|_PAGE_WRITE
-					|(__HEXAGON_C_DEV << 6));
-
-	last_addr = phys_addr + size - 1;
-
-	/*  Wrapping not allowed  */
-	if (!size || (last_addr < phys_addr))
-		return NULL;
-
-	/*  Rounds up to next page size, including whole-page offset */
-	size = PAGE_ALIGN(offset + size);
-
-	area = get_vm_area(size, VM_IOREMAP);
-	addr = (unsigned long)area->addr;
-
-	if (ioremap_page_range(addr, addr+size, phys_addr, prot)) {
-		vunmap((void *)addr);
-		return NULL;
-	}
-
-	return (void __iomem *) (offset + addr);
-}
-
-void iounmap(const volatile void __iomem *addr)
-{
-	vunmap((void *) ((unsigned long) addr & PAGE_MASK));
-}
-- 
2.37.1

[RFC PATCH 2/8] openrisc: mm: remove unneeded early ioremap code

[Christophe Leroy]

From: Baoquan He <bhe@redhat.com>

Under arch/openrisc, there isn't any place where ioremap() is called.
It means that there isn't early ioremap handling needed in openrisc,
So the early ioremap handling code in ioremap() of
arch/openrisc/mm/ioremap.c is unnecessary and can be removed.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Cc: Stafford Horne <shorne@gmail.com>
Cc: openrisc@lists.librecores.org
---
 arch/openrisc/mm/ioremap.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
index 8ec0dafecf25..90b59bc53c8c 100644
--- a/arch/openrisc/mm/ioremap.c
+++ b/arch/openrisc/mm/ioremap.c
@@ -22,8 +22,6 @@
 
 extern int mem_init_done;
 
-static unsigned int fixmaps_used __initdata;
-
 /*
  * Remap an arbitrary physical address space into the kernel virtual
  * address space. Needed when the kernel wants to access high addresses
@@ -52,24 +50,14 @@ void __iomem *__ref ioremap(phys_addr_t addr, unsigned long size)
 	p = addr & PAGE_MASK;
 	size = PAGE_ALIGN(last_addr + 1) - p;
 
-	if (likely(mem_init_done)) {
-		area = get_vm_area(size, VM_IOREMAP);
-		if (!area)
-			return NULL;
-		v = (unsigned long)area->addr;
-	} else {
-		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
-			return NULL;
-		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
-		fixmaps_used += (size >> PAGE_SHIFT);
-	}
+	area = get_vm_area(size, VM_IOREMAP);
+	if (!area)
+		return NULL;
+	v = (unsigned long)area->addr;
 
 	if (ioremap_page_range(v, v + size, p,
 			__pgprot(pgprot_val(PAGE_KERNEL) | _PAGE_CI))) {
-		if (likely(mem_init_done))
-			vfree(area->addr);
-		else
-			fixmaps_used -= (size >> PAGE_SHIFT);
+		vfree(area->addr);
 		return NULL;
 	}
 
-- 
2.37.1

[RFC PATCH 3/8] mm/ioremap: Define generic_ioremap_prot() and generic_iounmap()

[Christophe Leroy]

Define a generic version of ioremap_prot() and iounmap() that
architectures can call after they have performed the necessary
alteration to parameters and/or necessary verifications.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 include/asm-generic/io.h |  4 ++++
 mm/ioremap.c             | 26 ++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index a68f8fbf423b..43eb4f62e954 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1073,9 +1073,13 @@ static inline bool iounmap_allowed(void *addr)
 }
 #endif
 
+void __iomem *generic_ioremap_prot(phys_addr_t phys_addr, size_t size,
+                                  pgprot_t prot);
+
 void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 			   unsigned long prot);
 void iounmap(volatile void __iomem *addr);
+void generic_iounmap(volatile void __iomem *addr);
 
 static inline void __iomem *ioremap(phys_addr_t addr, size_t size)
 {
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 8652426282cc..9f34a8f90b58 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -11,8 +11,8 @@
 #include <linux/io.h>
 #include <linux/export.h>
 
-void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
-			   unsigned long prot)
+void __iomem *generic_ioremap_prot(phys_addr_t phys_addr, size_t size,
+				   pgprot_t prot)
 {
 	unsigned long offset, vaddr;
 	phys_addr_t last_addr;
@@ -28,7 +28,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 	phys_addr -= offset;
 	size = PAGE_ALIGN(size + offset);
 
-	if (!ioremap_allowed(phys_addr, size, prot))
+	if (!ioremap_allowed(phys_addr, size, pgprot_val(prot)))
 		return NULL;
 
 	area = get_vm_area_caller(size, VM_IOREMAP,
@@ -38,17 +38,24 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 	vaddr = (unsigned long)area->addr;
 	area->phys_addr = phys_addr;
 
-	if (ioremap_page_range(vaddr, vaddr + size, phys_addr,
-			       __pgprot(prot))) {
+	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
 		free_vm_area(area);
 		return NULL;
 	}
 
 	return (void __iomem *)(vaddr + offset);
 }
+
+#ifndef ioremap_prot
+void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
+			   unsigned long prot)
+{
+	return generic_ioremap_prot(phys_addr, size, __pgprot(prot));
+}
 EXPORT_SYMBOL(ioremap_prot);
+#endif
 
-void iounmap(volatile void __iomem *addr)
+void generic_iounmap(volatile void __iomem *addr)
 {
 	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
 
@@ -58,4 +65,11 @@ void iounmap(volatile void __iomem *addr)
 	if (is_vmalloc_addr(vaddr))
 		vunmap(vaddr);
 }
+
+#ifndef iounmap
+void iounmap(volatile void __iomem *addr)
+{
+	generic_iounmap(addr);
+}
 EXPORT_SYMBOL(iounmap);
+#endif
-- 
2.37.1

[RFC PATCH 4/8] mm: ioremap: allow ARCH to have its own ioremap definition

[Christophe Leroy]

From: Baoquan He <bhe@redhat.com>

Architectures like xtensa, arc, can be converted to GENERIC_IOREMAP,
to take standard ioremap_prot() and ioremap_xxx() way. But they have
ARCH specific handling for ioremap() method, than standard ioremap()
method.

In oder to convert them to take GENERIC_IOREMAP method, allow these
architecutres to have their own ioremap definition.

This is a preparation patch, no functionality change.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arch@vger.kernel.org
Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 include/asm-generic/io.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 43eb4f62e954..b2ed73708a3e 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1081,11 +1081,14 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 void iounmap(volatile void __iomem *addr);
 void generic_iounmap(volatile void __iomem *addr);
 
+#ifndef ioremap
+#define ioremap ioremap
 static inline void __iomem *ioremap(phys_addr_t addr, size_t size)
 {
 	/* _PAGE_IOREMAP needs to be supplied by the architecture */
 	return ioremap_prot(addr, size, _PAGE_IOREMAP);
 }
+#endif
 #endif /* !CONFIG_MMU || CONFIG_GENERIC_IOREMAP */
 
 #ifndef ioremap_wc
-- 
2.37.1

[RFC PATCH 5/8] arc: mm: Convert to GENERIC_IOREMAP

[Christophe Leroy]

From: Baoquan He <bhe@redhat.com>

By taking GENERIC_IOREMAP method, the generic ioremap_prot() and
iounmap() are visible and available to arch. Arch only needs to
provide implementation of arch_ioremap() or arch_iounmap() if there's
arch specific handling needed in its ioremap() or iounmap(). This
change will simplify implementation by removing duplicated codes with
generic ioremap() and iounmap(), and has the equivalent functioality
as before.

Here, add hooks arch_ioremap() and arch_iounmap() for arc's special
operation when ioremap_prot() and iounmap(). Meanwhile define and
implement arc's own ioremap() because arc has some special handling
in ioremap() than standard ioremap().

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Vineet Gupta <vgupta@kernel.org>
Cc: linux-snps-arc@lists.infradead.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/arc/Kconfig          |  1 +
 arch/arc/include/asm/io.h |  7 +++---
 arch/arc/mm/ioremap.c     | 46 +++------------------------------------
 3 files changed, 8 insertions(+), 46 deletions(-)

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 9e3653253ef2..a08d2abfaf61 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -26,6 +26,7 @@ config ARC
 	select GENERIC_PENDING_IRQ if SMP
 	select GENERIC_SCHED_CLOCK
 	select GENERIC_SMP_IDLE_THREAD
+	select GENERIC_IOREMAP
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE if ARC_MMU_V4
diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
index 8f777d6441a5..53b0f1e4f276 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -21,8 +21,8 @@
 #endif
 
 extern void __iomem *ioremap(phys_addr_t paddr, unsigned long size);
-extern void __iomem *ioremap_prot(phys_addr_t paddr, unsigned long size,
-				  unsigned long flags);
+#define ioremap ioremap
+#define ioremap_prot ioremap_prot
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
 	return (void __iomem *)port;
@@ -32,7 +32,8 @@ static inline void ioport_unmap(void __iomem *addr)
 {
 }
 
-extern void iounmap(const void __iomem *addr);
+bool iounmap_allowed(void *addr);
+#define iounmap_allowed iounmap_allowed
 
 /*
  * io{read,write}{16,32}be() macros
diff --git a/arch/arc/mm/ioremap.c b/arch/arc/mm/ioremap.c
index 0ee75aca6e10..02b750abccee 100644
--- a/arch/arc/mm/ioremap.c
+++ b/arch/arc/mm/ioremap.c
@@ -25,13 +25,6 @@ static inline bool arc_uncached_addr_space(phys_addr_t paddr)
 
 void __iomem *ioremap(phys_addr_t paddr, unsigned long size)
 {
-	phys_addr_t end;
-
-	/* Don't allow wraparound or zero size */
-	end = paddr + size - 1;
-	if (!size || (end < paddr))
-		return NULL;
-
 	/*
 	 * If the region is h/w uncached, MMU mapping can be elided as optim
 	 * The cast to u32 is fine as this region can only be inside 4GB
@@ -54,52 +47,19 @@ EXPORT_SYMBOL(ioremap);
 void __iomem *ioremap_prot(phys_addr_t paddr, unsigned long size,
 			   unsigned long flags)
 {
-	unsigned int off;
-	unsigned long vaddr;
-	struct vm_struct *area;
-	phys_addr_t end;
 	pgprot_t prot = __pgprot(flags);
 
-	/* Don't allow wraparound, zero size */
-	end = paddr + size - 1;
-	if ((!size) || (end < paddr))
-		return NULL;
-
 	/* An early platform driver might end up here */
 	if (!slab_is_available())
 		return NULL;
 
 	/* force uncached */
-	prot = pgprot_noncached(prot);
-
-	/* Mappings have to be page-aligned */
-	off = paddr & ~PAGE_MASK;
-	paddr &= PAGE_MASK_PHYS;
-	size = PAGE_ALIGN(end + 1) - paddr;
-
-	/*
-	 * Ok, go for it..
-	 */
-	area = get_vm_area(size, VM_IOREMAP);
-	if (!area)
-		return NULL;
-	area->phys_addr = paddr;
-	vaddr = (unsigned long)area->addr;
-	if (ioremap_page_range(vaddr, vaddr + size, paddr, prot)) {
-		vunmap((void __force *)vaddr);
-		return NULL;
-	}
-	return (void __iomem *)(off + (char __iomem *)vaddr);
+	return generic_ioremap_prot(paddr, size, pgprot_noncached(prot));
 }
 EXPORT_SYMBOL(ioremap_prot);
 
-
-void iounmap(const void __iomem *addr)
+bool iounmap_allowed(void *addr)
 {
 	/* weird double cast to handle phys_addr_t > 32 bits */
-	if (arc_uncached_addr_space((phys_addr_t)(u32)addr))
-		return;
-
-	vfree((void *)(PAGE_MASK & (unsigned long __force)addr));
+	return !arc_uncached_addr_space((phys_addr_t)(u32)addr);
 }
-EXPORT_SYMBOL(iounmap);
-- 
2.37.1

[RFC PATCH 6/8] ia64: mm: Convert to GENERIC_IOREMAP

[Christophe Leroy]

From: Baoquan He <bhe@redhat.com>

By taking GENERIC_IOREMAP method, the generic ioremap_prot() and
iounmap() are visible and available to arch. Arch only needs to
provide implementation of arch_ioremap() or arch_iounmap() if there's
arch specific handling needed in its ioremap() or iounmap(). This
change will simplify implementation by removing duplicated codes with
generic ioremap() and iounmap(), and has the equivalent functioality
as before.

Here add hooks arch_ioremap() and arch_iounmap() for ia64's special
operation when ioremap() and iounmap(), then ioremap_cache() is
converted to use ioremap_prot() from GENERIC_IOREMAP.

The old ioremap_uc() is kept and add its macro definittion.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: linux-ia64@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/ia64/Kconfig          |  1 +
 arch/ia64/include/asm/io.h | 11 ++++++----
 arch/ia64/mm/ioremap.c     | 45 ++++++--------------------------------
 3 files changed, 15 insertions(+), 42 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 26ac8ea15a9e..1ca18be5dc30 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -45,6 +45,7 @@ config IA64
 	select GENERIC_IRQ_LEGACY
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select GENERIC_IOMAP
+	select GENERIC_IOREMAP
 	select GENERIC_SMP_IDLE_THREAD
 	select ARCH_TASK_STRUCT_ON_STACK
 	select ARCH_TASK_STRUCT_ALLOCATOR
diff --git a/arch/ia64/include/asm/io.h b/arch/ia64/include/asm/io.h
index ce66dfc0e719..06e006d82d81 100644
--- a/arch/ia64/include/asm/io.h
+++ b/arch/ia64/include/asm/io.h
@@ -247,18 +247,21 @@ static inline void outsl(unsigned long port, const void *src,
 
 # ifdef __KERNEL__
 
-extern void __iomem * ioremap(unsigned long offset, unsigned long size);
+#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
+
+void __iomem *ioremap_prot(unsigned long phys_addr, unsigned long size,
+			   unsigned long flags);
 extern void __iomem * ioremap_uc(unsigned long offset, unsigned long size);
-extern void iounmap (volatile void __iomem *addr);
 static inline void __iomem * ioremap_cache (unsigned long phys_addr, unsigned long size)
 {
 	return ioremap(phys_addr, size);
 }
-#define ioremap ioremap
+#define ioremap_prot ioremap_prot
 #define ioremap_cache ioremap_cache
 #define ioremap_uc ioremap_uc
-#define iounmap iounmap
 
+bool iounmap_allowed(void *addr);
+#define iounmap_allowed iounmap_allowed
 /*
  * String version of IO memory access ops:
  */
diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
index 55fd3eb753ff..0d43efe528e4 100644
--- a/arch/ia64/mm/ioremap.c
+++ b/arch/ia64/mm/ioremap.c
@@ -29,13 +29,9 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
 	return __ioremap_uc(phys_addr);
 }
 
-void __iomem *
-ioremap (unsigned long phys_addr, unsigned long size)
+void __iomem *ioremap_prot(unsigned long phys_addr, unsigned long size,
+			   unsigned long flags)
 {
-	void __iomem *addr;
-	struct vm_struct *area;
-	unsigned long offset;
-	pgprot_t prot;
 	u64 attr;
 	unsigned long gran_base, gran_size;
 	unsigned long page_base;
@@ -68,36 +64,12 @@ ioremap (unsigned long phys_addr, unsigned long size)
 	 */
 	page_base = phys_addr & PAGE_MASK;
 	size = PAGE_ALIGN(phys_addr + size) - page_base;
-	if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) {
-		prot = PAGE_KERNEL;
-
-		/*
-		 * Mappings have to be page-aligned
-		 */
-		offset = phys_addr & ~PAGE_MASK;
-		phys_addr &= PAGE_MASK;
-
-		/*
-		 * Ok, go for it..
-		 */
-		area = get_vm_area(size, VM_IOREMAP);
-		if (!area)
-			return NULL;
-
-		area->phys_addr = phys_addr;
-		addr = (void __iomem *) area->addr;
-		if (ioremap_page_range((unsigned long) addr,
-				(unsigned long) addr + size, phys_addr, prot)) {
-			vunmap((void __force *) addr);
-			return NULL;
-		}
-
-		return (void __iomem *) (offset + (char __iomem *)addr);
-	}
+	if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB)
+		return generic_ioremap_prot(phys_addr, size, __pgprot(flags));
 
 	return __ioremap_uc(phys_addr);
 }
-EXPORT_SYMBOL(ioremap);
+EXPORT_SYMBOL(ioremap_prot);
 
 void __iomem *
 ioremap_uc(unsigned long phys_addr, unsigned long size)
@@ -114,10 +86,7 @@ early_iounmap (volatile void __iomem *addr, unsigned long size)
 {
 }
 
-void
-iounmap (volatile void __iomem *addr)
+bool iounmap_allowed(void *addr)
 {
-	if (REGION_NUMBER(addr) == RGN_GATE)
-		vunmap((void *) ((unsigned long) addr & PAGE_MASK));
+	return REGION_NUMBER(addr) == RGN_GATE;
 }
-EXPORT_SYMBOL(iounmap);
-- 
2.37.1

[RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Christophe Leroy]

Some architecture have a dedicated space for IOREMAP mappings.

If so, use it in generic_ioremap_pro().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 mm/ioremap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/ioremap.c b/mm/ioremap.c
index 9f34a8f90b58..a2be9cda975b 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -31,8 +31,13 @@ void __iomem *generic_ioremap_prot(phys_addr_t phys_addr, size_t size,
 	if (!ioremap_allowed(phys_addr, size, pgprot_val(prot)))
 		return NULL;
 
+#ifdef IOREMAP_START
+	area = __get_vm_area_caller(size, VM_IOREMAP, IOREMAP_START,
+				    IOREMAP_END, __builtin_return_address(0));
+#else
 	area = get_vm_area_caller(size, VM_IOREMAP,
 			__builtin_return_address(0));
+#endif
 	if (!area)
 		return NULL;
 	vaddr = (unsigned long)area->addr;
@@ -62,7 +67,7 @@ void generic_iounmap(volatile void __iomem *addr)
 	if (!iounmap_allowed(vaddr))
 		return;
 
-	if (is_vmalloc_addr(vaddr))
+	if (is_ioremap_addr(vaddr))
 		vunmap(vaddr);
 }
 
-- 
2.37.1

[RFC PATCH 8/8] powerpc: mm: Convert to GENERIC_IOREMAP

[Christophe Leroy]

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig          |  1 +
 arch/powerpc/include/asm/io.h | 11 ++++++++---
 arch/powerpc/mm/ioremap.c     | 26 +-------------------------
 arch/powerpc/mm/ioremap_32.c  | 25 ++++++++++---------------
 arch/powerpc/mm/ioremap_64.c  | 22 +++++++++-------------
 5 files changed, 29 insertions(+), 56 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 81c9f895d690..c8704933ae5a 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -179,6 +179,7 @@ config PPC
 	select GENERIC_CPU_VULNERABILITIES	if PPC_BARRIER_NOSPEC
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_GETTIMEOFDAY
+	select GENERIC_IOREMAP
 	select GENERIC_IRQ_SHOW
 	select GENERIC_IRQ_SHOW_LEVEL
 	select GENERIC_PCI_IOMAP		if PCI
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index fc112a91d0c2..3d38f46ade16 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -859,8 +859,8 @@ static inline void iosync(void)
  *
  */
 extern void __iomem *ioremap(phys_addr_t address, unsigned long size);
-extern void __iomem *ioremap_prot(phys_addr_t address, unsigned long size,
-				  unsigned long flags);
+#define ioremap ioremap
+#define ioremap_prot ioremap_prot
 extern void __iomem *ioremap_wc(phys_addr_t address, unsigned long size);
 #define ioremap_wc ioremap_wc
 
@@ -874,7 +874,12 @@ void __iomem *ioremap_coherent(phys_addr_t address, unsigned long size);
 #define ioremap_cache(addr, size) \
 	ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
 
-extern void iounmap(volatile void __iomem *addr);
+#ifdef CONFIG_PPC_INDIRECT_MMIO
+#define iounmap iounmap
+#endif
+
+bool iounmap_allowed(void *addr);
+#define iounmap_allowed iounmap_allowed
 
 void __iomem *ioremap_phb(phys_addr_t paddr, unsigned long size);
 
diff --git a/arch/powerpc/mm/ioremap.c b/arch/powerpc/mm/ioremap.c
index 4f12504fb405..705e8e8ffde4 100644
--- a/arch/powerpc/mm/ioremap.c
+++ b/arch/powerpc/mm/ioremap.c
@@ -41,7 +41,7 @@ void __iomem *ioremap_coherent(phys_addr_t addr, unsigned long size)
 	return __ioremap_caller(addr, size, prot, caller);
 }
 
-void __iomem *ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long flags)
+void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long flags)
 {
 	pte_t pte = __pte(flags);
 	void *caller = __builtin_return_address(0);
@@ -74,27 +74,3 @@ int early_ioremap_range(unsigned long ea, phys_addr_t pa,
 
 	return 0;
 }
-
-void __iomem *do_ioremap(phys_addr_t pa, phys_addr_t offset, unsigned long size,
-			 pgprot_t prot, void *caller)
-{
-	struct vm_struct *area;
-	int ret;
-	unsigned long va;
-
-	area = __get_vm_area_caller(size, VM_IOREMAP, IOREMAP_START, IOREMAP_END, caller);
-	if (area == NULL)
-		return NULL;
-
-	area->phys_addr = pa;
-	va = (unsigned long)area->addr;
-
-	ret = ioremap_page_range(va, va + size, pa, prot);
-	if (!ret)
-		return (void __iomem *)area->addr + offset;
-
-	vunmap_range(va, va + size);
-	free_vm_area(area);
-
-	return NULL;
-}
diff --git a/arch/powerpc/mm/ioremap_32.c b/arch/powerpc/mm/ioremap_32.c
index 9d13143b8be4..e9a60ddfd6b3 100644
--- a/arch/powerpc/mm/ioremap_32.c
+++ b/arch/powerpc/mm/ioremap_32.c
@@ -21,6 +21,13 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call
 	phys_addr_t p, offset;
 	int err;
 
+	/*
+	 * If the address lies within the first 16 MB, assume it's in ISA
+	 * memory space
+	 */
+	if (addr < SZ_16M)
+		addr += _ISA_MEM_BASE;
+
 	/*
 	 * Choose an address to map it to.
 	 * Once the vmalloc system is running, we use it.
@@ -31,13 +38,6 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call
 	offset = addr & ~PAGE_MASK;
 	size = PAGE_ALIGN(addr + size) - p;
 
-	/*
-	 * If the address lies within the first 16 MB, assume it's in ISA
-	 * memory space
-	 */
-	if (p < 16 * 1024 * 1024)
-		p += _ISA_MEM_BASE;
-
 #ifndef CONFIG_CRASH_DUMP
 	/*
 	 * Don't allow anybody to remap normal RAM that we're using.
@@ -63,7 +63,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call
 		return (void __iomem *)v + offset;
 
 	if (slab_is_available())
-		return do_ioremap(p, offset, size, prot, caller);
+		return generic_ioremap_prot(addr, size, prot);
 
 	/*
 	 * Should check if it is a candidate for a BAT mapping
@@ -78,16 +78,11 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, pgprot_t prot, void *call
 	return (void __iomem *)ioremap_bot + offset;
 }
 
-void iounmap(volatile void __iomem *addr)
+bool iounmap_allowed(void *addr)
 {
 	/*
 	 * If mapped by BATs then there is nothing to do.
 	 * Calling vfree() generates a benign warning.
 	 */
-	if (v_block_mapped((unsigned long)addr))
-		return;
-
-	if (addr > high_memory && (unsigned long)addr < ioremap_bot)
-		vunmap((void *)(PAGE_MASK & (unsigned long)addr));
+	return !v_block_mapped((unsigned long)addr);
 }
-EXPORT_SYMBOL(iounmap);
diff --git a/arch/powerpc/mm/ioremap_64.c b/arch/powerpc/mm/ioremap_64.c
index 3acece00b33e..a319d40253e5 100644
--- a/arch/powerpc/mm/ioremap_64.c
+++ b/arch/powerpc/mm/ioremap_64.c
@@ -29,7 +29,7 @@ void __iomem *__ioremap_caller(phys_addr_t addr, unsigned long size,
 		return NULL;
 
 	if (slab_is_available())
-		return do_ioremap(paligned, offset, size, prot, caller);
+		return generic_ioremap_prot(addr, size, prot);
 
 	pr_warn("ioremap() called early from %pS. Use early_ioremap() instead\n", caller);
 
@@ -47,19 +47,15 @@ void __iomem *__ioremap_caller(phys_addr_t addr, unsigned long size,
  * Unmap an IO region and remove it from vmalloc'd list.
  * Access to IO memory should be serialized by driver.
  */
-void iounmap(volatile void __iomem *token)
+bool iounmap_allowed(void *addr)
 {
-	void *addr;
-
-	if (!slab_is_available())
-		return;
-
-	addr = (void *)((unsigned long __force)PCI_FIX_ADDR(token) & PAGE_MASK);
+	return slab_is_available();
+}
 
-	if ((unsigned long)addr < ioremap_bot) {
-		pr_warn("Attempt to iounmap early bolted mapping at 0x%p\n", addr);
-		return;
-	}
-	vunmap(addr);
+#ifdef CONFIG_PPC_INDIRECT_MMIO
+void iounmap(volatile void __iomem *token)
+{
+	generic_iounmap(PCI_FIX_ADDR(token));
 }
 EXPORT_SYMBOL(iounmap);
+#endif
-- 
2.37.1

Re: [RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Arnd Bergmann]

On Wed, Oct 12, 2022, at 12:09 PM, Christophe Leroy wrote:
> Some architecture have a dedicated space for IOREMAP mappings.
>
> If so, use it in generic_ioremap_pro().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

"Some" means exactly powerpc64, right? It looks like microblaze
and powerpc32 still share some of this code, but effectively
just use the vmalloc area once the slab allocator is up.

Is the special case still useful for powerpc64 or could this be
changed to do it the same as everything else?

    Arnd

Re: [RFC PATCH 2/8] openrisc: mm: remove unneeded early ioremap code

[Stafford Horne]

On Wed, Oct 12, 2022 at 12:09:38PM +0200, Christophe Leroy wrote:
> From: Baoquan He <bhe@redhat.com>
> 
> Under arch/openrisc, there isn't any place where ioremap() is called.
> It means that there isn't early ioremap handling needed in openrisc,
> So the early ioremap handling code in ioremap() of
> arch/openrisc/mm/ioremap.c is unnecessary and can be removed.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Jonas Bonn <jonas@southpole.se>
> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: openrisc@lists.librecores.org

Thanks for picking this up.

Perhaps add this link?

Link: https://lore.kernel.org/linux-mm/YwxfxKrTUtAuejKQ@oscomms1/

But either way.

Acked-by: Stafford Horne <shorne@gmail.com>

> ---
>  arch/openrisc/mm/ioremap.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
> index 8ec0dafecf25..90b59bc53c8c 100644
> --- a/arch/openrisc/mm/ioremap.c
> +++ b/arch/openrisc/mm/ioremap.c
> @@ -22,8 +22,6 @@
>  
>  extern int mem_init_done;
>  
> -static unsigned int fixmaps_used __initdata;
> -
>  /*
>   * Remap an arbitrary physical address space into the kernel virtual
>   * address space. Needed when the kernel wants to access high addresses
> @@ -52,24 +50,14 @@ void __iomem *__ref ioremap(phys_addr_t addr, unsigned long size)
>  	p = addr & PAGE_MASK;
>  	size = PAGE_ALIGN(last_addr + 1) - p;
>  
> -	if (likely(mem_init_done)) {
> -		area = get_vm_area(size, VM_IOREMAP);
> -		if (!area)
> -			return NULL;
> -		v = (unsigned long)area->addr;
> -	} else {
> -		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> -			return NULL;
> -		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
> -		fixmaps_used += (size >> PAGE_SHIFT);
> -	}
> +	area = get_vm_area(size, VM_IOREMAP);
> +	if (!area)
> +		return NULL;
> +	v = (unsigned long)area->addr;
>  
>  	if (ioremap_page_range(v, v + size, p,
>  			__pgprot(pgprot_val(PAGE_KERNEL) | _PAGE_CI))) {
> -		if (likely(mem_init_done))
> -			vfree(area->addr);
> -		else
> -			fixmaps_used -= (size >> PAGE_SHIFT);
> +		vfree(area->addr);
>  		return NULL;
>  	}
>  
> -- 
> 2.37.1
> 

Re: [RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Alexander Gordeev]

On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
> "Some" means exactly powerpc64, right? It looks like microblaze
> and powerpc32 still share some of this code, but effectively
> just use the vmalloc area once the slab allocator is up.
> 
> Is the special case still useful for powerpc64 or could this be
> changed to do it the same as everything else?

Or make it the other way around and set IOREMAP_START/IOREMAP_END
to VMALLOC_START/VMALLOC_END by default?

> 
>     Arnd

Re: [RFC PATCH 3/8] mm/ioremap: Define generic_ioremap_prot() and generic_iounmap()

[Alexander Gordeev]

On Wed, Oct 12, 2022 at 12:09:39PM +0200, Christophe Leroy wrote:
> Define a generic version of ioremap_prot() and iounmap() that
> architectures can call after they have performed the necessary
> alteration to parameters and/or necessary verifications.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

[...]

> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 8652426282cc..9f34a8f90b58 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -11,8 +11,8 @@
>  #include <linux/io.h>
>  #include <linux/export.h>
>  
> -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
> -			   unsigned long prot)
> +void __iomem *generic_ioremap_prot(phys_addr_t phys_addr, size_t size,
> +				   pgprot_t prot)
>  {
>  	unsigned long offset, vaddr;
>  	phys_addr_t last_addr;
> @@ -28,7 +28,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>  	phys_addr -= offset;
>  	size = PAGE_ALIGN(size + offset);
>  
> -	if (!ioremap_allowed(phys_addr, size, prot))
> +	if (!ioremap_allowed(phys_addr, size, pgprot_val(prot)))
>  		return NULL;

It seems to me ioremap_allowed() is not needed anymore.
Whatever is checked here would move to architecture-
specific implementation.

>  	area = get_vm_area_caller(size, VM_IOREMAP,
> @@ -38,17 +38,24 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>  	vaddr = (unsigned long)area->addr;
>  	area->phys_addr = phys_addr;
>  
> -	if (ioremap_page_range(vaddr, vaddr + size, phys_addr,
> -			       __pgprot(prot))) {
> +	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
>  		free_vm_area(area);
>  		return NULL;
>  	}
>  
>  	return (void __iomem *)(vaddr + offset);
>  }
> +
> +#ifndef ioremap_prot

I guess, this is also needed:

#define ioremap_prot ioremap_prot

> +void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
> +			   unsigned long prot)
> +{
> +	return generic_ioremap_prot(phys_addr, size, __pgprot(prot));
> +}
>  EXPORT_SYMBOL(ioremap_prot);
> +#endif
>  
> -void iounmap(volatile void __iomem *addr)
> +void generic_iounmap(volatile void __iomem *addr)
>  {
>  	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
>  
> @@ -58,4 +65,11 @@ void iounmap(volatile void __iomem *addr)
>  	if (is_vmalloc_addr(vaddr))
>  		vunmap(vaddr);
>  }
> +
> +#ifndef iounmap

Same here.

> +void iounmap(volatile void __iomem *addr)
> +{
> +	generic_iounmap(addr);
> +}
>  EXPORT_SYMBOL(iounmap);
> +#endif

Re: [RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Arnd Bergmann]

On Sun, Oct 16, 2022, at 9:54 AM, Alexander Gordeev wrote:
> On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
>> "Some" means exactly powerpc64, right? It looks like microblaze
>> and powerpc32 still share some of this code, but effectively
>> just use the vmalloc area once the slab allocator is up.
>> 
>> Is the special case still useful for powerpc64 or could this be
>> changed to do it the same as everything else?
>
> Or make it the other way around and set IOREMAP_START/IOREMAP_END
> to VMALLOC_START/VMALLOC_END by default?

Sure, if there is a reason for actually making them different.
From the git history, it appears that before commit 3d5134ee8341
("[POWERPC] Rewrite IO allocation & mapping on powerpc64"), the
ioremap() and vmalloc() handling was largely duplicated. Ben
cleaned it up by making most of the implementation shared but left
the separate address spaces.

My guess is that there was no technical reason for this, other
than having no reason to change the behavior at the time.

       Arnd

Re: [RFC PATCH 3/8] mm/ioremap: Define generic_ioremap_prot() and generic_iounmap()

[Christophe Leroy]

Le 16/10/2022 à 10:14, Alexander Gordeev a écrit :
> On Wed, Oct 12, 2022 at 12:09:39PM +0200, Christophe Leroy wrote:
>> Define a generic version of ioremap_prot() and iounmap() that
>> architectures can call after they have performed the necessary
>> alteration to parameters and/or necessary verifications.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> [...]
> 
>> diff --git a/mm/ioremap.c b/mm/ioremap.c
>> index 8652426282cc..9f34a8f90b58 100644
>> --- a/mm/ioremap.c
>> +++ b/mm/ioremap.c
>> @@ -11,8 +11,8 @@
>>   #include <linux/io.h>
>>   #include <linux/export.h>
>>   
>> -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>> -			   unsigned long prot)
>> +void __iomem *generic_ioremap_prot(phys_addr_t phys_addr, size_t size,
>> +				   pgprot_t prot)
>>   {
>>   	unsigned long offset, vaddr;
>>   	phys_addr_t last_addr;
>> @@ -28,7 +28,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>>   	phys_addr -= offset;
>>   	size = PAGE_ALIGN(size + offset);
>>   
>> -	if (!ioremap_allowed(phys_addr, size, prot))
>> +	if (!ioremap_allowed(phys_addr, size, pgprot_val(prot)))
>>   		return NULL;
> 
> It seems to me ioremap_allowed() is not needed anymore.
> Whatever is checked here would move to architecture-
> specific implementation.

Yes can probably be removed as a follow-up. I didn't want to change 
existing implementations at the first place, and see what it looks like 
once all architectures have been looked at.

> 
>>   	area = get_vm_area_caller(size, VM_IOREMAP,
>> @@ -38,17 +38,24 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>>   	vaddr = (unsigned long)area->addr;
>>   	area->phys_addr = phys_addr;
>>   
>> -	if (ioremap_page_range(vaddr, vaddr + size, phys_addr,
>> -			       __pgprot(prot))) {
>> +	if (ioremap_page_range(vaddr, vaddr + size, phys_addr, prot)) {
>>   		free_vm_area(area);
>>   		return NULL;
>>   	}
>>   
>>   	return (void __iomem *)(vaddr + offset);
>>   }
>> +
>> +#ifndef ioremap_prot
> 
> I guess, this is also needed:
> 
> #define ioremap_prot ioremap_prot

Why would it need that ? We are in ioremap.c, any define done here is 
exclusively seen here in this C file. It's not like a header file.

> 
>> +void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>> +			   unsigned long prot)
>> +{
>> +	return generic_ioremap_prot(phys_addr, size, __pgprot(prot));
>> +}
>>   EXPORT_SYMBOL(ioremap_prot);
>> +#endif
>>   
>> -void iounmap(volatile void __iomem *addr)
>> +void generic_iounmap(volatile void __iomem *addr)
>>   {
>>   	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
>>   
>> @@ -58,4 +65,11 @@ void iounmap(volatile void __iomem *addr)
>>   	if (is_vmalloc_addr(vaddr))
>>   		vunmap(vaddr);
>>   }
>> +
>> +#ifndef iounmap
> 
> Same here.

Same, I don't see what it would add.

> 
>> +void iounmap(volatile void __iomem *addr)
>> +{
>> +	generic_iounmap(addr);
>> +}
>>   EXPORT_SYMBOL(iounmap);
>> +#endif

Re: [RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Christophe Leroy]

I'm more focussed on powerpc32

+ Adding linuxppc-dev, someone else might help.

Le 16/10/2022 à 13:51, Arnd Bergmann a écrit :
> On Sun, Oct 16, 2022, at 9:54 AM, Alexander Gordeev wrote:
>> On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
>>> "Some" means exactly powerpc64, right? It looks like microblaze
>>> and powerpc32 still share some of this code, but effectively
>>> just use the vmalloc area once the slab allocator is up.
>>>
>>> Is the special case still useful for powerpc64 or could this be
>>> changed to do it the same as everything else?
>>
>> Or make it the other way around and set IOREMAP_START/IOREMAP_END
>> to VMALLOC_START/VMALLOC_END by default?
> 
> Sure, if there is a reason for actually making them different.
>  From the git history, it appears that before commit 3d5134ee8341
> ("[POWERPC] Rewrite IO allocation & mapping on powerpc64"), the
> ioremap() and vmalloc() handling was largely duplicated. Ben
> cleaned it up by making most of the implementation shared but left
> the separate address spaces.
> 
> My guess is that there was no technical reason for this, other
> than having no reason to change the behavior at the time.
> 
>         Arnd

Re: [RFC PATCH 0/8] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way (Alternative)

[Baoquan He]

Hi Christophe,

On 10/12/22 at 12:09pm, Christophe Leroy wrote:
> From: 
> 
> As proposed in the discussion related to your series, here comes an
> exemple of how it could be.
> 
> I have taken it into ARC and IA64 architectures as an exemple. This is
> untested, even not compiled, it is just to illustrated my meaning in the
> discussion.
> 
> I also added a patch for powerpc architecture, that one in tested with
> both pmac32_defconfig and ppc64_le_defconfig.
> 
> From my point of view, this different approach provide less churn and
> less intellectual disturbance than the way you do it.

Yes, I agree, and admire your insistence on the thing you think right or
better. Learn from you.

When you suggested this in my v2 post, I made a draft patch at below link
according to your suggestion to request people to review. What worried
me is that I am not sure it's ignored or disliked after one week of
waiting.

https://lore.kernel.org/all/YwtND%2FL8xD+ViN3r@MiWiFi-R3L-srv/#related

Up to now, seems people don't oppose this generic_ioremap_prot() way, we
can take it. So what's your plan? You want me to continue with your
patches wrapped in, or I can leave it to you if you want to take over?

Thanks
Baoquan

> 
> Baoquan He (5):
>   hexagon: mm: Convert to GENERIC_IOREMAP
>   openrisc: mm: remove unneeded early ioremap code
>   mm: ioremap: allow ARCH to have its own ioremap definition
>   arc: mm: Convert to GENERIC_IOREMAP
>   ia64: mm: Convert to GENERIC_IOREMAP
> 
> Christophe Leroy (3):
>   mm/ioremap: Define generic_ioremap_prot() and generic_iounmap()
>   mm/ioremap: Consider IOREMAP space in generic ioremap
>   powerpc: mm: Convert to GENERIC_IOREMAP
> 
>  arch/arc/Kconfig              |  1 +
>  arch/arc/include/asm/io.h     |  7 +++---
>  arch/arc/mm/ioremap.c         | 46 +++--------------------------------
>  arch/hexagon/Kconfig          |  1 +
>  arch/hexagon/include/asm/io.h |  9 +++++--
>  arch/hexagon/mm/ioremap.c     | 44 ---------------------------------
>  arch/ia64/Kconfig             |  1 +
>  arch/ia64/include/asm/io.h    | 11 ++++++---
>  arch/ia64/mm/ioremap.c        | 45 ++++++----------------------------
>  arch/openrisc/mm/ioremap.c    | 22 ++++-------------
>  arch/powerpc/Kconfig          |  1 +
>  arch/powerpc/include/asm/io.h | 11 ++++++---
>  arch/powerpc/mm/ioremap.c     | 26 +-------------------
>  arch/powerpc/mm/ioremap_32.c  | 25 ++++++++-----------
>  arch/powerpc/mm/ioremap_64.c  | 22 +++++++----------
>  include/asm-generic/io.h      |  7 ++++++
>  mm/ioremap.c                  | 33 +++++++++++++++++++------
>  17 files changed, 98 insertions(+), 214 deletions(-)
>  delete mode 100644 arch/hexagon/mm/ioremap.c
> 
> -- 
> 2.37.1
> 

Re: [RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Michael Ellerman]

"Arnd Bergmann" <arnd@arndb.de> writes:
> On Sun, Oct 16, 2022, at 9:54 AM, Alexander Gordeev wrote:
>> On Wed, Oct 12, 2022 at 12:39:11PM +0200, Arnd Bergmann wrote:
>>> "Some" means exactly powerpc64, right? It looks like microblaze
>>> and powerpc32 still share some of this code, but effectively
>>> just use the vmalloc area once the slab allocator is up.
>>> 
>>> Is the special case still useful for powerpc64 or could this be
>>> changed to do it the same as everything else?
>>
>> Or make it the other way around and set IOREMAP_START/IOREMAP_END
>> to VMALLOC_START/VMALLOC_END by default?
>
> Sure, if there is a reason for actually making them different.
> From the git history, it appears that before commit 3d5134ee8341
> ("[POWERPC] Rewrite IO allocation & mapping on powerpc64"), the
> ioremap() and vmalloc() handling was largely duplicated. Ben
> cleaned it up by making most of the implementation shared but left
> the separate address spaces.
>
> My guess is that there was no technical reason for this, other
> than having no reason to change the behavior at the time.

I think the immediate reason for it is that on some CPUs we have to use
4K pages in the HPT for IO mappings, but PAGE_SIZE == 64K, and we can
only have a single page size per segment (256M or 1T).

cheers

Re: [RFC PATCH 0/8] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way (Alternative)

[Christophe Leroy]

Hi Baoquan,

Le 17/10/2022 à 02:37, Baoquan He a écrit :
> Hi Christophe,
> 
> On 10/12/22 at 12:09pm, Christophe Leroy wrote:
>> From:
>>
>> As proposed in the discussion related to your series, here comes an
>> exemple of how it could be.
>>
>> I have taken it into ARC and IA64 architectures as an exemple. This is
>> untested, even not compiled, it is just to illustrated my meaning in the
>> discussion.
>>
>> I also added a patch for powerpc architecture, that one in tested with
>> both pmac32_defconfig and ppc64_le_defconfig.
>>
>>  From my point of view, this different approach provide less churn and
>> less intellectual disturbance than the way you do it.
> 
> Yes, I agree, and admire your insistence on the thing you think right or
> better. Learn from you.
> 
> When you suggested this in my v2 post, I made a draft patch at below link
> according to your suggestion to request people to review. What worried
> me is that I am not sure it's ignored or disliked after one week of
> waiting.
> 
> https://lore.kernel.org/all/YwtND%2FL8xD+ViN3r@MiWiFi-R3L-srv/#related
> 
> Up to now, seems people don't oppose this generic_ioremap_prot() way, we
> can take it. So what's your plan? You want me to continue with your
> patches wrapped in, or I can leave it to you if you want to take over?

I don't plan to steal your work. If you feel confortable with my 
proposal, feel free to continue with it and amplify it. You have done 
most of the job, you have a clear view of all subtilities in the 
different architectures, so please continue, I don't plan to take over 
the good work you've done until now.

The only purpose of my series was to illustrate my comments and convince 
myself it was a possible way, nothing more.

Thanks
Christophe

> 
> Thanks
> Baoquan
> 
>>
>> Baoquan He (5):
>>    hexagon: mm: Convert to GENERIC_IOREMAP
>>    openrisc: mm: remove unneeded early ioremap code
>>    mm: ioremap: allow ARCH to have its own ioremap definition
>>    arc: mm: Convert to GENERIC_IOREMAP
>>    ia64: mm: Convert to GENERIC_IOREMAP
>>
>> Christophe Leroy (3):
>>    mm/ioremap: Define generic_ioremap_prot() and generic_iounmap()
>>    mm/ioremap: Consider IOREMAP space in generic ioremap
>>    powerpc: mm: Convert to GENERIC_IOREMAP
>>
>>   arch/arc/Kconfig              |  1 +
>>   arch/arc/include/asm/io.h     |  7 +++---
>>   arch/arc/mm/ioremap.c         | 46 +++--------------------------------
>>   arch/hexagon/Kconfig          |  1 +
>>   arch/hexagon/include/asm/io.h |  9 +++++--
>>   arch/hexagon/mm/ioremap.c     | 44 ---------------------------------
>>   arch/ia64/Kconfig             |  1 +
>>   arch/ia64/include/asm/io.h    | 11 ++++++---
>>   arch/ia64/mm/ioremap.c        | 45 ++++++----------------------------
>>   arch/openrisc/mm/ioremap.c    | 22 ++++-------------
>>   arch/powerpc/Kconfig          |  1 +
>>   arch/powerpc/include/asm/io.h | 11 ++++++---
>>   arch/powerpc/mm/ioremap.c     | 26 +-------------------
>>   arch/powerpc/mm/ioremap_32.c  | 25 ++++++++-----------
>>   arch/powerpc/mm/ioremap_64.c  | 22 +++++++----------
>>   include/asm-generic/io.h      |  7 ++++++
>>   mm/ioremap.c                  | 33 +++++++++++++++++++------
>>   17 files changed, 98 insertions(+), 214 deletions(-)
>>   delete mode 100644 arch/hexagon/mm/ioremap.c
>>
>> -- 
>> 2.37.1
>>
> 

Re: [RFC PATCH 7/8] mm/ioremap: Consider IOREMAP space in generic ioremap

[Arnd Bergmann]

On Mon, Oct 17, 2022, at 2:50 PM, Michael Ellerman wrote:
> "Arnd Bergmann" <arnd@arndb.de> writes:
>>
>> My guess is that there was no technical reason for this, other
>> than having no reason to change the behavior at the time.
>
> I think the immediate reason for it is that on some CPUs we have to use
> 4K pages in the HPT for IO mappings, but PAGE_SIZE == 64K, and we can
> only have a single page size per segment (256M or 1T).

Right, makes sense. Both the original patch, or the variant with
defining IOREMAP_START everywhere seem fine to me then.

     Arnd

Re: [RFC PATCH 0/8] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way (Alternative)

[Baoquan He]

On 10/17/22 at 05:06pm, Christophe Leroy wrote:
> Hi Baoquan,
> 
> Le 17/10/2022 à 02:37, Baoquan He a écrit :
> > Hi Christophe,
> > 
> > On 10/12/22 at 12:09pm, Christophe Leroy wrote:
> >> From:
> >>
> >> As proposed in the discussion related to your series, here comes an
> >> exemple of how it could be.
> >>
> >> I have taken it into ARC and IA64 architectures as an exemple. This is
> >> untested, even not compiled, it is just to illustrated my meaning in the
> >> discussion.
> >>
> >> I also added a patch for powerpc architecture, that one in tested with
> >> both pmac32_defconfig and ppc64_le_defconfig.
> >>
> >>  From my point of view, this different approach provide less churn and
> >> less intellectual disturbance than the way you do it.
> > 
> > Yes, I agree, and admire your insistence on the thing you think right or
> > better. Learn from you.
> > 
> > When you suggested this in my v2 post, I made a draft patch at below link
> > according to your suggestion to request people to review. What worried
> > me is that I am not sure it's ignored or disliked after one week of
> > waiting.
> > 
> > https://lore.kernel.org/all/YwtND%2FL8xD+ViN3r@MiWiFi-R3L-srv/#related
> > 
> > Up to now, seems people don't oppose this generic_ioremap_prot() way, we
> > can take it. So what's your plan? You want me to continue with your
> > patches wrapped in, or I can leave it to you if you want to take over?
> 
> I don't plan to steal your work. If you feel confortable with my 
> proposal, feel free to continue with it and amplify it. You have done 
> most of the job, you have a clear view of all subtilities in the 
> different architectures, so please continue, I don't plan to take over 
> the good work you've done until now.
> 
> The only purpose of my series was to illustrate my comments and convince 
> myself it was a possible way, nothing more.

Thanks a lot for all these you have done, I will post another version with
the introducing generic_ioremap_prot() way you suggesed.