[PATCH v2 00/11] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way

[PATCH v2 00/11] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way

[Baoquan He]

Currently, many architecutres have't taken the standard GENERIC_IOREMAP
way to implement ioremap_prot(), iounmap(), and ioremap_xx(), but make
these functions specifically under each arch's folder. Those cause many
duplicated codes of ioremap() and iounmap().

In this patchset, firstly adapt the hooks io[re|un]map_allowed, then
make use of them to convert those ARCH-es to take GENERIC_IOREMAP method.
With these change, duplicated ioremap/iounmap() code uder ARCH-es are
removed.

This is suggested by Christoph in below discussion:
https://lore.kernel.org/all/Yp7h0Jv6vpgt6xdZ@infradead.org/T/#u

And it's basically further action after arm64 has converted to
GENERIC_IOREMAP way in below patchset.
[PATCH v5 0/6] arm64: Cleanup ioremap() and support ioremap_prot()
https://lore.kernel.org/all/20220607125027.44946-1-wangkefeng.wang@huawei.com/T/#u

And some change of io[re|un]map_allowed() is borrowed from the v3 of
arm64 converting patch.
[PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u

v1->v2:
  Rename io[re|un]map_allowed() to arch_io[re|un]map() and made
  some minor changes in patch 1~2 as per Alexander and Kefeng's
  suggestions. Accordingly, adjust patches~4~11 because of the renaming
  arch_io[re|un]map().

Testing:
It passes the testing on arm64. For other ARCHes, I only tried to pass
build with existing RPMs of cross compiling tools. ARCHes like openrisc,
parisc, don't build because of lack of cross compiling RPMS.

Baoquan He (11):
  mm/ioremap: change the return value of io[re|un]map_allowed and rename
  mm: ioremap: fixup the physical address and page prot
  mm: ioremap: allow ARCH to have its own ioremap definition
  arc: mm: Convert to GENERIC_IOREMAP
  hexagon: mm: Convert to GENERIC_IOREMAP
  ia64: mm: Convert to GENERIC_IOREMAP
  openrisc: mm: Convert to GENERIC_IOREMAP
  parisc: mm: Convert to GENERIC_IOREMAP
  s390: mm: Convert to GENERIC_IOREMAP
  sh: mm: Convert to GENERIC_IOREMAP
  xtensa: mm: Convert to GENERIC_IOREMAP

 arch/arc/Kconfig               |  1 +
 arch/arc/include/asm/io.h      | 19 +++++++---
 arch/arc/mm/ioremap.c          | 60 +++++--------------------------
 arch/arm64/include/asm/io.h    |  5 +--
 arch/arm64/mm/ioremap.c        | 16 ++++++---
 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     | 26 ++++++++------
 arch/ia64/mm/ioremap.c         | 50 ++++++--------------------
 arch/openrisc/Kconfig          |  1 +
 arch/openrisc/include/asm/io.h | 16 ++++++---
 arch/openrisc/mm/ioremap.c     | 51 +++++++++-----------------
 arch/parisc/Kconfig            |  1 +
 arch/parisc/include/asm/io.h   | 16 ++++++---
 arch/parisc/mm/ioremap.c       | 65 ++++------------------------------
 arch/s390/Kconfig              |  1 +
 arch/s390/include/asm/io.h     | 26 +++++++++-----
 arch/s390/pci/pci.c            | 60 +++++--------------------------
 arch/sh/Kconfig                |  1 +
 arch/sh/include/asm/io.h       | 47 ++++++++----------------
 arch/sh/mm/ioremap.c           | 61 ++++++-------------------------
 arch/xtensa/Kconfig            |  1 +
 arch/xtensa/include/asm/io.h   | 39 +++++++++-----------
 arch/xtensa/mm/ioremap.c       | 56 +++++++----------------------
 include/asm-generic/io.h       | 34 ++++++++++--------
 mm/ioremap.c                   | 12 ++++---
 28 files changed, 230 insertions(+), 490 deletions(-)
 delete mode 100644 arch/hexagon/mm/ioremap.c

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Baoquan He]

In some architectures, there are ARCH specifici io address mapping
handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
openrisc, s390, sh.

In oder to convert them to take GENERIC_IOREMAP method, we need change
the return value of hook ioremap_allowed() and iounmap_allowed().
Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
their current behaviour.

===
 arch_ioremap() return a bool,
   - IS_ERR means return an error
   - NULL means continue to remap
   - a non-NULL, non-IS_ERR pointer is returned directly
 arch_iounmap() return a bool,
   - 0 means continue to vunmap
   - error code means skip vunmap and return directly

This is taken from Kefeng's below old patch. Christoph suggested the
return value because he foresaw the doablity of converting to take
GENERIC_IOREMAP on more architectures.
 - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
 - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u

While at it, the invocation of arch_ioremap() need be moved to the
beginning of ioremap_prot() because architectures like sh, openrisc,
ia64, need do the ARCH specific io address mapping on the original
physical address. And in the later patch, the address fix up code
in arch_ioremap() also need be done on the original addre on some
architectures.

This is preparation for later patch, no functionality change.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/arm64/include/asm/io.h |  4 ++--
 arch/arm64/mm/ioremap.c     | 15 ++++++++++-----
 include/asm-generic/io.h    | 29 +++++++++++++++--------------
 mm/ioremap.c                | 12 ++++++++----
 4 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 877495a0fd0c..dd7e1c2dc86c 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -139,8 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
  * I/O memory mapping functions.
  */
 
-bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot);
-#define ioremap_allowed ioremap_allowed
+void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
+#define arch_ioremap arch_ioremap
 
 #define _PAGE_IOREMAP PROT_DEVICE_nGnRE
 
diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
index c5af103d4ad4..b0f4cea86f0e 100644
--- a/arch/arm64/mm/ioremap.c
+++ b/arch/arm64/mm/ioremap.c
@@ -3,19 +3,24 @@
 #include <linux/mm.h>
 #include <linux/io.h>
 
-bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot)
+void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
 {
-	unsigned long last_addr = phys_addr + size - 1;
+	unsigned long last_addr, offset;
+
+	offset = phys_addr & (~PAGE_MASK);
+	phys_addr -= offset;
+	size = PAGE_ALIGN(size + offset);
+	last_addr = phys_addr + size - 1;
 
 	/* Don't allow outside PHYS_MASK */
 	if (last_addr & ~PHYS_MASK)
-		return false;
+		return IOMEM_ERR_PTR(-EINVAL);
 
 	/* Don't allow RAM to be mapped. */
 	if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr))))
-		return false;
+		return IOMEM_ERR_PTR(-EINVAL);
 
-	return true;
+	return NULL;
 }
 
 /*
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index a68f8fbf423b..7b6bfb62ef80 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1049,27 +1049,28 @@ static inline void iounmap(volatile void __iomem *addr)
 
 /*
  * Arch code can implement the following two hooks when using GENERIC_IOREMAP
- * ioremap_allowed() return a bool,
- *   - true means continue to remap
- *   - false means skip remap and return directly
- * iounmap_allowed() return a bool,
- *   - true means continue to vunmap
- *   - false means skip vunmap and return directly
+ * arch_ioremap() return a bool,
+ *   - IS_ERR means return an error
+ *   - NULL means continue to remap
+ *   - a non-NULL, non-IS_ERR pointer is returned directly
+ * arch_iounmap() return a bool,
+ *   - 0 means continue to vunmap
+ *   - error code means skip vunmap and return directly
  */
-#ifndef ioremap_allowed
-#define ioremap_allowed ioremap_allowed
-static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size,
+#ifndef arch_ioremap
+#define arch_ioremap arch_ioremap
+static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size,
 				   unsigned long prot)
 {
-	return true;
+	return NULL;
 }
 #endif
 
-#ifndef iounmap_allowed
-#define iounmap_allowed iounmap_allowed
-static inline bool iounmap_allowed(void *addr)
+#ifndef arch_iounmap
+#define arch_iounmap arch_iounmap
+static inline int arch_iounmap(void __iomem *addr)
 {
-	return true;
+	return 0;
 }
 #endif
 
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 8652426282cc..99fde69becc7 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 	unsigned long offset, vaddr;
 	phys_addr_t last_addr;
 	struct vm_struct *area;
+	void __iomem *ioaddr;
+
+	ioaddr = arch_ioremap(phys_addr, size, prot);
+	if (IS_ERR(ioaddr))
+		return NULL;
+	else if (ioaddr)
+		return ioaddr;
 
 	/* Disallow wrap-around or zero size */
 	last_addr = phys_addr + size - 1;
@@ -28,9 +35,6 @@ 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))
-		return NULL;
-
 	area = get_vm_area_caller(size, VM_IOREMAP,
 			__builtin_return_address(0));
 	if (!area)
@@ -52,7 +56,7 @@ void iounmap(volatile void __iomem *addr)
 {
 	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
 
-	if (!iounmap_allowed(vaddr))
+	if (arch_iounmap((void __iomem *)addr))
 		return;
 
 	if (is_vmalloc_addr(vaddr))
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On some architectures, the physical address need be fixed up before
doing mapping, e.g, parisc. And on architectures, e.g arc, the
parameter 'prot' passed into ioremap_prot() need be adjusted too.

In oder to convert them to take GENERIC_IOREMAP method, we need wrap
the address fixing up code and page prot adjusting code into arch_ioremap()
and pass the new address and 'prot' out for ioremap_prot() handling.

This is a preparation patch, no functionality change.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/arm64/include/asm/io.h | 3 ++-
 arch/arm64/mm/ioremap.c     | 5 +++--
 include/asm-generic/io.h    | 4 ++--
 mm/ioremap.c                | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index dd7e1c2dc86c..6a5578ddbbf6 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -139,7 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
  * I/O memory mapping functions.
  */
 
-void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
 #define arch_ioremap arch_ioremap
 
 #define _PAGE_IOREMAP PROT_DEVICE_nGnRE
diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
index b0f4cea86f0e..ef75ffef4dbc 100644
--- a/arch/arm64/mm/ioremap.c
+++ b/arch/arm64/mm/ioremap.c
@@ -3,9 +3,10 @@
 #include <linux/mm.h>
 #include <linux/io.h>
 
-void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	unsigned long last_addr, offset;
+	unsigned long last_addr, offset, phys_addr = *paddr;
 
 	offset = phys_addr & (~PAGE_MASK);
 	phys_addr -= offset;
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7b6bfb62ef80..fb9bda2be8ed 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1059,8 +1059,8 @@ static inline void iounmap(volatile void __iomem *addr)
  */
 #ifndef arch_ioremap
 #define arch_ioremap arch_ioremap
-static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size,
-				   unsigned long prot)
+static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size,
+				   unsigned long *prot_val)
 {
 	return NULL;
 }
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 99fde69becc7..7914b5cf5b78 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -19,7 +19,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 	struct vm_struct *area;
 	void __iomem *ioaddr;
 
-	ioaddr = arch_ioremap(phys_addr, size, prot);
+	ioaddr = arch_ioremap(&phys_addr, size, &prot);
 	if (IS_ERR(ioaddr))
 		return NULL;
 	else if (ioaddr)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 03/11] mm: ioremap: allow ARCH to have its own ioremap definition

[Baoquan He]

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>
---
 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 fb9bda2be8ed..68a8117b30fa 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1078,11 +1078,14 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 			   unsigned long prot);
 void 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.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 04/11] arc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

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
---
 arch/arc/Kconfig          |  1 +
 arch/arc/include/asm/io.h | 19 +++++++++----
 arch/arc/mm/ioremap.c     | 60 ++++++---------------------------------
 3 files changed, 23 insertions(+), 57 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..af88a2c5550e 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -20,9 +20,20 @@
 #define __iowmb()		do { } while (0)
 #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);
+/*
+ * I/O memory mapping functions.
+ */
+
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
+
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
+
+void __iomem *ioremap(phys_addr_t paddr, unsigned long size);
+#define ioremap ioremap
+
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
 	return (void __iomem *)port;
@@ -32,8 +43,6 @@ static inline void ioport_unmap(void __iomem *addr)
 {
 }
 
-extern void iounmap(const void __iomem *addr);
-
 /*
  * io{read,write}{16,32}be() macros
  */
diff --git a/arch/arc/mm/ioremap.c b/arch/arc/mm/ioremap.c
index 0ee75aca6e10..72b01da31ea1 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
@@ -44,62 +37,25 @@ void __iomem *ioremap(phys_addr_t paddr, unsigned long size)
 }
 EXPORT_SYMBOL(ioremap);
 
-/*
- * ioremap with access flags
- * Cache semantics wise it is same as ioremap - "forced" uncached.
- * However unlike vanilla ioremap which bypasses ARC MMU for addresses in
- * ARC hardware uncached region, this one still goes thru the MMU as caller
- * might need finer access control (R/W/X)
- */
-void __iomem *ioremap_prot(phys_addr_t paddr, unsigned long size,
-			   unsigned long flags)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	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;
+		return IOMEM_ERR_PTR(-EINVAL);
 
 	/* force uncached */
-	prot = pgprot_noncached(prot);
+	*prot_val = pgprot_val(pgprot_noncached(__pgprot(*prot_val)));
 
-	/* Mappings have to be page-aligned */
-	off = paddr & ~PAGE_MASK;
-	paddr &= PAGE_MASK_PHYS;
-	size = PAGE_ALIGN(end + 1) - paddr;
+	return NULL;
 
-	/*
-	 * 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);
 }
-EXPORT_SYMBOL(ioremap_prot);
-
 
-void iounmap(const void __iomem *addr)
+int arch_iounmap(void __iomem *addr)
 {
 	/* weird double cast to handle phys_addr_t > 32 bits */
 	if (arc_uncached_addr_space((phys_addr_t)(u32)addr))
-		return;
+		return -EINVAL;
 
-	vfree((void *)(PAGE_MASK & (unsigned long __force)addr));
+	return 0;
 }
-EXPORT_SYMBOL(iounmap);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 05/11] hexagon: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

With it, the old ioremap() and iounmap() can be perfectly removed
since they are duplicated with the standard functions.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Brian Cain <bcain@quicinc.com>
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 c33241425a5c..e2d3091ec9d6 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.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 06/11] ia64: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

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
---
 arch/ia64/Kconfig          |  1 +
 arch/ia64/include/asm/io.h | 26 ++++++++++++--------
 arch/ia64/mm/ioremap.c     | 50 +++++++++-----------------------------
 3 files changed, 28 insertions(+), 49 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..d90a4c551de2 100644
--- a/arch/ia64/include/asm/io.h
+++ b/arch/ia64/include/asm/io.h
@@ -247,17 +247,23 @@ static inline void outsl(unsigned long port, const void *src,
 
 # ifdef __KERNEL__
 
-extern void __iomem * ioremap(unsigned long offset, unsigned long size);
-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_cache ioremap_cache
+/*
+ * I/O memory mapping functions.
+ */
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
+
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
+
+#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
+
+#define ioremap_cache(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
+
+void __iomem *ioremap_uc(unsigned long offset, unsigned long size);
 #define ioremap_uc ioremap_uc
-#define iounmap iounmap
 
 /*
  * String version of IO memory access ops:
diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
index 55fd3eb753ff..8a085fc660e3 100644
--- a/arch/ia64/mm/ioremap.c
+++ b/arch/ia64/mm/ioremap.c
@@ -30,15 +30,12 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
 }
 
 void __iomem *
-ioremap (unsigned long phys_addr, unsigned long size)
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	void __iomem *addr;
-	struct vm_struct *area;
-	unsigned long offset;
-	pgprot_t prot;
-	u64 attr;
+	phys_addr_t phys_addr = *paddr;
 	unsigned long gran_base, gran_size;
 	unsigned long page_base;
+	u64 attr;
 
 	/*
 	 * For things in kern_memmap, we must use the same attribute
@@ -69,35 +66,18 @@ 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);
+		return NULL;
 	}
 
 	return __ioremap_uc(phys_addr);
 }
-EXPORT_SYMBOL(ioremap);
+
+int arch_iounmap(void __iomem *addr)
+{
+	if (REGION_NUMBER(addr) != RGN_GATE)
+		return -EINVAL;
+	return 0;
+}
 
 void __iomem *
 ioremap_uc(unsigned long phys_addr, unsigned long size)
@@ -113,11 +93,3 @@ void
 early_iounmap (volatile void __iomem *addr, unsigned long size)
 {
 }
-
-void
-iounmap (volatile void __iomem *addr)
-{
-	if (REGION_NUMBER(addr) == RGN_GATE)
-		vunmap((void *) ((unsigned long) addr & PAGE_MASK));
-}
-EXPORT_SYMBOL(iounmap);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Add hooks arch_ioremap() and arch_iounmap() for operisc's special
operation when ioremap() and iounmap.

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/Kconfig          |  1 +
 arch/openrisc/include/asm/io.h | 16 ++++++++---
 arch/openrisc/mm/ioremap.c     | 51 +++++++++++-----------------------
 3 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index c7f282f60f64..fd9bb76a610b 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -21,6 +21,7 @@ config OPENRISC
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
 	select GENERIC_PCI_IOMAP
+	select GENERIC_IOREMAP
 	select GENERIC_CPU_DEVICES
 	select HAVE_PCI
 	select HAVE_UID16
diff --git a/arch/openrisc/include/asm/io.h b/arch/openrisc/include/asm/io.h
index ee6043a03173..9db67938bfc4 100644
--- a/arch/openrisc/include/asm/io.h
+++ b/arch/openrisc/include/asm/io.h
@@ -15,6 +15,8 @@
 #define __ASM_OPENRISC_IO_H
 
 #include <linux/types.h>
+#include <asm/pgtable.h>
+#include <asm/pgalloc.h>
 
 /*
  * PCI: We do not use IO ports in OpenRISC
@@ -27,11 +29,17 @@
 #define PIO_OFFSET		0
 #define PIO_MASK		0
 
-#define ioremap ioremap
-void __iomem *ioremap(phys_addr_t offset, unsigned long size);
+/*
+ * I/O memory mapping functions.
+ */
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
+
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
 
-#define iounmap iounmap
-extern void iounmap(volatile void __iomem *addr);
+#define _PAGE_IOREMAP (pgprot_val(PAGE_KERNEL) | _PAGE_CI)
 
 #include <asm-generic/io.h>
 
diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
index 8ec0dafecf25..bc41660e1fb0 100644
--- a/arch/openrisc/mm/ioremap.c
+++ b/arch/openrisc/mm/ioremap.c
@@ -24,26 +24,18 @@ 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
- * directly.
- *
- * NOTE! We need to allow non-page-aligned mappings too: we will obviously
- * have to convert them into an offset in a page-aligned mapping, but the
- * caller shouldn't need to know that small detail.
- */
-void __iomem *__ref ioremap(phys_addr_t addr, unsigned long size)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
 	phys_addr_t p;
 	unsigned long v;
-	unsigned long offset, last_addr;
-	struct vm_struct *area = NULL;
+	unsigned long offset, last_addr, addr = *paddr;
+	int ret = -EINVAL;
 
 	/* Don't allow wraparound or zero size */
 	last_addr = addr + size - 1;
 	if (!size || last_addr < addr)
-		return NULL;
+		return IOMEM_ERR_PTR(ret);
 
 	/*
 	 * Mappings have to be page-aligned
@@ -52,32 +44,24 @@ 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 (unlikely(!mem_init_done)) {
 		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
-			return NULL;
+			return IOMEM_ERR_PTR(ret);
 		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
 		fixmaps_used += (size >> PAGE_SHIFT);
-	}
 
-	if (ioremap_page_range(v, v + size, p,
-			__pgprot(pgprot_val(PAGE_KERNEL) | _PAGE_CI))) {
-		if (likely(mem_init_done))
-			vfree(area->addr);
-		else
+		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
 			fixmaps_used -= (size >> PAGE_SHIFT);
-		return NULL;
+			return IOMEM_ERR_PTR(ret);
+		}
+
+		return (void __iomem *)(offset + (char *)v);
 	}
 
-	return (void __iomem *)(offset + (char *)v);
+	return NULL;
 }
-EXPORT_SYMBOL(ioremap);
 
-void iounmap(volatile void __iomem *addr)
+int arch_iounmap(void __iomem *addr)
 {
 	/* If the page is from the fixmap pool then we just clear out
 	 * the fixmap mapping.
@@ -97,13 +81,10 @@ void iounmap(volatile void __iomem *addr)
 		 *   ii) invalid accesses to the freed areas aren't made
 		 */
 		flush_tlb_all();
-		return;
+		return -EINVAL;
 	}
-
-	return vfree((void *)(PAGE_MASK & (unsigned long)addr));
+	return 0;
 }
-EXPORT_SYMBOL(iounmap);
-
 /**
  * OK, this one's a bit tricky... ioremap can get called before memory is
  * initialized (early serial console does this) and will want to alloc a page
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 08/11] parisc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Add hook arch_ioremap() for parisc's special operation when ioremap(),
then ioremap_[wc|uc]() are converted to use ioremap_prot() from
GENERIC_IOREMAP.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: linux-parisc@vger.kernel.org
---
 arch/parisc/Kconfig          |  1 +
 arch/parisc/include/asm/io.h | 16 ++++++---
 arch/parisc/mm/ioremap.c     | 65 ++++--------------------------------
 3 files changed, 18 insertions(+), 64 deletions(-)

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 7f059cd1196a..5fed465c9b83 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -36,6 +36,7 @@ config PARISC
 	select GENERIC_ATOMIC64 if !64BIT
 	select GENERIC_IRQ_PROBE
 	select GENERIC_PCI_IOMAP
+	select GENERIC_IOREMAP
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select GENERIC_SMP_IDLE_THREAD
 	select GENERIC_ARCH_TOPOLOGY if SMP
diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
index 42ffb60a6ea9..614e21d9749f 100644
--- a/arch/parisc/include/asm/io.h
+++ b/arch/parisc/include/asm/io.h
@@ -123,13 +123,19 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr)
 }
 
 /*
- * The standard PCI ioremap interfaces
+ * I/O memory mapping functions.
  */
-void __iomem *ioremap(unsigned long offset, unsigned long size);
-#define ioremap_wc			ioremap
-#define ioremap_uc			ioremap
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long prot);
+#define arch_ioremap arch_ioremap
 
-extern void iounmap(const volatile void __iomem *addr);
+#define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | \
+		       _PAGE_ACCESSED | _PAGE_NO_CACHE)
+
+#define ioremap_wc(addr, size)  \
+	ioremap_prot((addr), (size), _PAGE_IOREMAP)
+#define ioremap_uc(addr, size)  \
+	ioremap_prot((addr), (size), _PAGE_IOREMAP)
 
 static inline unsigned char __raw_readb(const volatile void __iomem *addr)
 {
diff --git a/arch/parisc/mm/ioremap.c b/arch/parisc/mm/ioremap.c
index 345ff0b66499..28884757fad0 100644
--- a/arch/parisc/mm/ioremap.c
+++ b/arch/parisc/mm/ioremap.c
@@ -13,38 +13,19 @@
 #include <linux/io.h>
 #include <linux/mm.h>
 
-/*
- * Generic mapping function (not visible outside):
- */
-
-/*
- * Remap an arbitrary physical address space into the kernel virtual
- * address space.
- *
- * NOTE! We need to allow non-page-aligned mappings too: we will obviously
- * have to convert them into an offset in a page-aligned mapping, but the
- * caller shouldn't need to know that small detail.
- */
-void __iomem *ioremap(unsigned long phys_addr, unsigned long size)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	void __iomem *addr;
-	struct vm_struct *area;
-	unsigned long offset, last_addr;
-	pgprot_t pgprot;
+	phys_addr_t phys_addr = *paddr;
 
 #ifdef CONFIG_EISA
 	unsigned long end = phys_addr + size - 1;
 	/* Support EISA addresses */
 	if ((phys_addr >= 0x00080000 && end < 0x000fffff) ||
 	    (phys_addr >= 0x00500000 && end < 0x03bfffff))
-		phys_addr |= F_EXTEND(0xfc000000);
+		*paddr = phys_addr |= F_EXTEND(0xfc000000);
 #endif
 
-	/* Don't allow wraparound or zero size */
-	last_addr = phys_addr + size - 1;
-	if (!size || last_addr < phys_addr)
-		return NULL;
-
 	/*
 	 * Don't allow anybody to remap normal RAM that we're using..
 	 */
@@ -58,43 +39,9 @@ void __iomem *ioremap(unsigned long phys_addr, unsigned long size)
 		for (page = virt_to_page(t_addr); 
 		     page <= virt_to_page(t_end); page++) {
 			if(!PageReserved(page))
-				return NULL;
+				return IOMEM_ERR_PTR(-EINVAL);
 		}
 	}
 
-	pgprot = __pgprot(_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY |
-			  _PAGE_ACCESSED | _PAGE_NO_CACHE);
-
-	/*
-	 * Mappings have to be page-aligned
-	 */
-	offset = phys_addr & ~PAGE_MASK;
-	phys_addr &= PAGE_MASK;
-	size = PAGE_ALIGN(last_addr + 1) - phys_addr;
-
-	/*
-	 * Ok, go for it..
-	 */
-	area = get_vm_area(size, VM_IOREMAP);
-	if (!area)
-		return NULL;
-
-	addr = (void __iomem *) area->addr;
-	if (ioremap_page_range((unsigned long)addr, (unsigned long)addr + size,
-			       phys_addr, pgprot)) {
-		vunmap(addr);
-		return NULL;
-	}
-
-	return (void __iomem *) (offset + (char __iomem *)addr);
-}
-EXPORT_SYMBOL(ioremap);
-
-void iounmap(const volatile void __iomem *io_addr)
-{
-	unsigned long addr = (unsigned long)io_addr & PAGE_MASK;
-
-	if (is_vmalloc_addr((void *)addr))
-		vunmap((void *)addr);
+	return NULL;
 }
-EXPORT_SYMBOL(iounmap);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Add hooks arch_ioremap() and arch_iounmap() for s390's special
operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
converted to use ioremap_prot() from GENERIC_IOREMAP.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: linux-s390@vger.kernel.org
---
 arch/s390/Kconfig          |  1 +
 arch/s390/include/asm/io.h | 26 +++++++++++------
 arch/s390/pci/pci.c        | 60 +++++---------------------------------
 3 files changed, 26 insertions(+), 61 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 318fce77601d..c59e1b25f59d 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -135,6 +135,7 @@ config S390
 	select GENERIC_SMP_IDLE_THREAD
 	select GENERIC_TIME_VSYSCALL
 	select GENERIC_VDSO_TIME_NS
+	select GENERIC_IOREMAP
 	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_JUMP_LABEL
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index e3882b012bfa..f837e20b7bbd 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -22,11 +22,23 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
 
 #define IO_SPACE_LIMIT 0
 
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
-void __iomem *ioremap(phys_addr_t addr, size_t size);
-void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
-void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
-void iounmap(volatile void __iomem *addr);
+
+/*
+ * I/O memory mapping functions.
+ */
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
+
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
+
+#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
+
+#define ioremap_wc(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
+#define ioremap_wt(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
 
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
@@ -51,10 +63,6 @@ static inline void ioport_unmap(void __iomem *p)
 #define pci_iomap_wc pci_iomap_wc
 #define pci_iomap_wc_range pci_iomap_wc_range
 
-#define ioremap ioremap
-#define ioremap_wt ioremap_wt
-#define ioremap_wc ioremap_wc
-
 #define memcpy_fromio(dst, src, count)	zpci_memcpy_fromio(dst, src, count)
 #define memcpy_toio(dst, src, count)	zpci_memcpy_toio(dst, src, count)
 #define memset_io(dst, val, count)	zpci_memset_io(dst, val, count)
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 73cdc5539384..984cad9cd5a1 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -244,64 +244,20 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
        zpci_memcpy_toio(to, from, count);
 }
 
-static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	unsigned long offset, vaddr;
-	struct vm_struct *area;
-	phys_addr_t last_addr;
-
-	last_addr = addr + size - 1;
-	if (!size || last_addr < addr)
-		return NULL;
-
 	if (!static_branch_unlikely(&have_mio))
-		return (void __iomem *) addr;
-
-	offset = addr & ~PAGE_MASK;
-	addr &= PAGE_MASK;
-	size = PAGE_ALIGN(size + offset);
-	area = get_vm_area(size, VM_IOREMAP);
-	if (!area)
-		return NULL;
-
-	vaddr = (unsigned long) area->addr;
-	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
-		free_vm_area(area);
-		return NULL;
-	}
-	return (void __iomem *) ((unsigned long) area->addr + offset);
-}
-
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
-{
-	return __ioremap(addr, size, __pgprot(prot));
+		return (void __iomem *) *paddr;
+	return NULL;
 }
-EXPORT_SYMBOL(ioremap_prot);
 
-void __iomem *ioremap(phys_addr_t addr, size_t size)
+int arch_iounmap(void __iomem *addr)
 {
-	return __ioremap(addr, size, PAGE_KERNEL);
-}
-EXPORT_SYMBOL(ioremap);
-
-void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(ioremap_wc);
-
-void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
-{
-	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(ioremap_wt);
-
-void iounmap(volatile void __iomem *addr)
-{
-	if (static_branch_likely(&have_mio))
-		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
+	if (!static_branch_likely(&have_mio))
+		return -EINVAL;
+	return 0;
 }
-EXPORT_SYMBOL(iounmap);
 
 /* Create a virtual mapping cookie for a PCI BAR */
 static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Add hook arch_ioremap() and arch_iounmap for sh's special operation when
ioremap() and iounmap(), then ioremap_cache() is converted to use
ioremap_prot() from GENERIC_IOREMAP.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Cc: linux-sh@vger.kernel.org
---
 arch/sh/Kconfig          |  1 +
 arch/sh/include/asm/io.h | 47 +++++++++----------------------
 arch/sh/mm/ioremap.c     | 61 ++++++++--------------------------------
 3 files changed, 26 insertions(+), 83 deletions(-)

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 5f220e903e5a..b63ad4698cf8 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -25,6 +25,7 @@ config SUPERH
 	select GENERIC_SCHED_CLOCK
 	select GENERIC_SMP_IDLE_THREAD
 	select GUP_GET_PTE_LOW_HIGH if X2TLB
+	select GENERIC_IOREMAP if MMU
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_SECCOMP_FILTER
diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
index fba90e670ed4..3c5ff82a511a 100644
--- a/arch/sh/include/asm/io.h
+++ b/arch/sh/include/asm/io.h
@@ -242,45 +242,26 @@ unsigned long long poke_real_address_q(unsigned long long addr,
 #define phys_to_virt(address)	(__va(address))
 #endif
 
-#ifdef CONFIG_MMU
-void iounmap(void __iomem *addr);
-void __iomem *__ioremap_caller(phys_addr_t offset, unsigned long size,
-			       pgprot_t prot, void *caller);
-
-static inline void __iomem *ioremap(phys_addr_t offset, unsigned long size)
-{
-	return __ioremap_caller(offset, size, PAGE_KERNEL_NOCACHE,
-			__builtin_return_address(0));
-}
-
-static inline void __iomem *
-ioremap_cache(phys_addr_t offset, unsigned long size)
-{
-	return __ioremap_caller(offset, size, PAGE_KERNEL,
-			__builtin_return_address(0));
-}
-#define ioremap_cache ioremap_cache
+/*
+ * I/O memory mapping functions.
+ */
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
 
-#ifdef CONFIG_HAVE_IOREMAP_PROT
-static inline void __iomem *ioremap_prot(phys_addr_t offset, unsigned long size,
-		unsigned long flags)
-{
-	return __ioremap_caller(offset, size, __pgprot(flags),
-			__builtin_return_address(0));
-}
-#endif /* CONFIG_HAVE_IOREMAP_PROT */
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
 
-#else /* CONFIG_MMU */
-static inline void __iomem *ioremap(phys_addr_t offset, size_t size)
-{
-	return (void __iomem *)(unsigned long)offset;
-}
+#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL_NOCACHE)
 
-static inline void iounmap(volatile void __iomem *addr) { }
-#endif /* CONFIG_MMU */
+#define ioremap_cache(addr, size)  \
+	ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
+#define ioremap_cache ioremap_cache
 
 #define ioremap_uc	ioremap
 
+#include <asm-generic/io.h>
+
 /*
  * Convert a physical pointer to a virtual kernel pointer for /dev/mem
  * access
diff --git a/arch/sh/mm/ioremap.c b/arch/sh/mm/ioremap.c
index 21342581144d..720a9186b06b 100644
--- a/arch/sh/mm/ioremap.c
+++ b/arch/sh/mm/ioremap.c
@@ -72,22 +72,12 @@ __ioremap_29bit(phys_addr_t offset, unsigned long size, pgprot_t prot)
 #define __ioremap_29bit(offset, size, prot)		NULL
 #endif /* CONFIG_29BIT */
 
-/*
- * Remap an arbitrary physical address space into the kernel virtual
- * address space. Needed when the kernel wants to access high addresses
- * directly.
- *
- * NOTE! We need to allow non-page-aligned mappings too: we will obviously
- * have to convert them into an offset in a page-aligned mapping, but the
- * caller shouldn't need to know that small detail.
- */
-void __iomem * __ref
-__ioremap_caller(phys_addr_t phys_addr, unsigned long size,
-		 pgprot_t pgprot, void *caller)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	struct vm_struct *area;
-	unsigned long offset, last_addr, addr, orig_addr;
+	unsigned long last_addr, phys_addr = *paddr;
 	void __iomem *mapped;
+	pgprot_t pgprot = __pgprot(*prot_val);
 
 	mapped = __ioremap_trapped(phys_addr, size);
 	if (mapped)
@@ -100,7 +90,7 @@ __ioremap_caller(phys_addr_t phys_addr, unsigned long size,
 	/* Don't allow wraparound or zero size */
 	last_addr = phys_addr + size - 1;
 	if (!size || last_addr < phys_addr)
-		return NULL;
+		return IOMEM_ERR_PTR(-EINVAL);
 
 	/*
 	 * If we can't yet use the regular approach, go the fixmap route.
@@ -116,30 +106,8 @@ __ioremap_caller(phys_addr_t phys_addr, unsigned long size,
 	if (mapped && !IS_ERR(mapped))
 		return mapped;
 
-	/*
-	 * Mappings have to be page-aligned
-	 */
-	offset = phys_addr & ~PAGE_MASK;
-	phys_addr &= PAGE_MASK;
-	size = PAGE_ALIGN(last_addr+1) - phys_addr;
-
-	/*
-	 * Ok, go for it..
-	 */
-	area = get_vm_area_caller(size, VM_IOREMAP, caller);
-	if (!area)
-		return NULL;
-	area->phys_addr = phys_addr;
-	orig_addr = addr = (unsigned long)area->addr;
-
-	if (ioremap_page_range(addr, addr + size, phys_addr, pgprot)) {
-		vunmap((void *)orig_addr);
-		return NULL;
-	}
-
-	return (void __iomem *)(offset + (char *)orig_addr);
+	return NULL;
 }
-EXPORT_SYMBOL(__ioremap_caller);
 
 /*
  * Simple checks for non-translatable mappings.
@@ -158,7 +126,7 @@ static inline int iomapping_nontranslatable(unsigned long offset)
 	return 0;
 }
 
-void iounmap(void __iomem *addr)
+int arch_iounmap(void __iomem *addr)
 {
 	unsigned long vaddr = (unsigned long __force)addr;
 	struct vm_struct *p;
@@ -167,26 +135,19 @@ void iounmap(void __iomem *addr)
 	 * Nothing to do if there is no translatable mapping.
 	 */
 	if (iomapping_nontranslatable(vaddr))
-		return;
+		return -EINVAL;
 
 	/*
 	 * There's no VMA if it's from an early fixed mapping.
 	 */
 	if (iounmap_fixed(addr) == 0)
-		return;
+		return -EINVAL;
 
 	/*
 	 * If the PMB handled it, there's nothing else to do.
 	 */
 	if (pmb_unmap(addr) == 0)
-		return;
+		return -EINVAL;
 
-	p = remove_vm_area((void *)(vaddr & PAGE_MASK));
-	if (!p) {
-		printk(KERN_ERR "%s: bad address %p\n", __func__, addr);
-		return;
-	}
-
-	kfree(p);
+	return 0;
 }
-EXPORT_SYMBOL(iounmap);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 11/11] xtensa: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Add hooks arch_ioremap() and arch_iounmap() for xtensa's special
operation when ioremap() and iounmap(). Then define and implement its
own ioremap() and ioremap_cache().

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: linux-xtensa@linux-xtensa.org
---
 arch/xtensa/Kconfig          |  1 +
 arch/xtensa/include/asm/io.h | 39 +++++++++++--------------
 arch/xtensa/mm/ioremap.c     | 56 +++++++++---------------------------
 3 files changed, 31 insertions(+), 65 deletions(-)

diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig
index 12ac277282ba..ee3a638f5458 100644
--- a/arch/xtensa/Kconfig
+++ b/arch/xtensa/Kconfig
@@ -29,6 +29,7 @@ config XTENSA
 	select GENERIC_LIB_UCMPDI2
 	select GENERIC_PCI_IOMAP
 	select GENERIC_SCHED_CLOCK
+	select GENERIC_IOREMAP if MMU
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL
 	select HAVE_ARCH_KASAN if MMU && !XIP_KERNEL
diff --git a/arch/xtensa/include/asm/io.h b/arch/xtensa/include/asm/io.h
index a5b707e1c0f4..04dfc0dc936b 100644
--- a/arch/xtensa/include/asm/io.h
+++ b/arch/xtensa/include/asm/io.h
@@ -16,6 +16,7 @@
 #include <asm/vectors.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
+#include <linux/pgtable.h>
 
 #include <linux/types.h>
 
@@ -23,23 +24,29 @@
 #define IO_SPACE_LIMIT ~0
 #define PCI_IOBASE		((void __iomem *)XCHAL_KIO_BYPASS_VADDR)
 
-#ifdef CONFIG_MMU
-
-void __iomem *xtensa_ioremap_nocache(unsigned long addr, unsigned long size);
-void __iomem *xtensa_ioremap_cache(unsigned long addr, unsigned long size);
-void xtensa_iounmap(volatile void __iomem *addr);
-
 /*
- * Return the virtual address for the specified bus memory.
+ * I/O memory mapping functions.
  */
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
+#define arch_ioremap arch_ioremap
+
+int arch_iounmap(void __iomem *addr);
+#define arch_iounmap arch_iounmap
+
+void __iomem *ioremap_prot(phys_addr_t paddr, size_t size,
+			   unsigned long prot);
+
 static inline void __iomem *ioremap(unsigned long offset, unsigned long size)
 {
 	if (offset >= XCHAL_KIO_PADDR
 	    && offset - XCHAL_KIO_PADDR < XCHAL_KIO_SIZE)
 		return (void*)(offset-XCHAL_KIO_PADDR+XCHAL_KIO_BYPASS_VADDR);
 	else
-		return xtensa_ioremap_nocache(offset, size);
+		return ioremap_prot(offset, size,
+			pgprot_val(pgprot_noncached(PAGE_KERNEL)));
 }
+#define ioremap ioremap
 
 static inline void __iomem *ioremap_cache(unsigned long offset,
 		unsigned long size)
@@ -48,22 +55,10 @@ static inline void __iomem *ioremap_cache(unsigned long offset,
 	    && offset - XCHAL_KIO_PADDR < XCHAL_KIO_SIZE)
 		return (void*)(offset-XCHAL_KIO_PADDR+XCHAL_KIO_CACHED_VADDR);
 	else
-		return xtensa_ioremap_cache(offset, size);
-}
-#define ioremap_cache ioremap_cache
-
-static inline void iounmap(volatile void __iomem *addr)
-{
-	unsigned long va = (unsigned long) addr;
+		return ioremap_prot(offset, size, pgprot_val(PAGE_KERNEL));
 
-	if (!(va >= XCHAL_KIO_CACHED_VADDR &&
-	      va - XCHAL_KIO_CACHED_VADDR < XCHAL_KIO_SIZE) &&
-	    !(va >= XCHAL_KIO_BYPASS_VADDR &&
-	      va - XCHAL_KIO_BYPASS_VADDR < XCHAL_KIO_SIZE))
-		xtensa_iounmap(addr);
 }
-
-#endif /* CONFIG_MMU */
+#define ioremap_cache ioremap_cache
 
 #include <asm-generic/io.h>
 
diff --git a/arch/xtensa/mm/ioremap.c b/arch/xtensa/mm/ioremap.c
index a400188c16b9..0508448d767a 100644
--- a/arch/xtensa/mm/ioremap.c
+++ b/arch/xtensa/mm/ioremap.c
@@ -6,60 +6,30 @@
  */
 
 #include <linux/io.h>
-#include <linux/vmalloc.h>
 #include <linux/pgtable.h>
 #include <asm/cacheflush.h>
 #include <asm/io.h>
 
-static void __iomem *xtensa_ioremap(unsigned long paddr, unsigned long size,
-				    pgprot_t prot)
+void __iomem *
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	unsigned long offset = paddr & ~PAGE_MASK;
-	unsigned long pfn = __phys_to_pfn(paddr);
-	struct vm_struct *area;
-	unsigned long vaddr;
-	int err;
-
-	paddr &= PAGE_MASK;
+	unsigned long phys_addr = *paddr;
+	unsigned long pfn = __phys_to_pfn(phys_addr);
 
 	WARN_ON(pfn_valid(pfn));
 
-	size = PAGE_ALIGN(offset + size);
-
-	area = get_vm_area(size, VM_IOREMAP);
-	if (!area)
-		return NULL;
-
-	vaddr = (unsigned long)area->addr;
-	area->phys_addr = paddr;
-
-	err = ioremap_page_range(vaddr, vaddr + size, paddr, prot);
-
-	if (err) {
-		vunmap((void *)vaddr);
-		return NULL;
-	}
-
-	flush_cache_vmap(vaddr, vaddr + size);
-	return (void __iomem *)(offset + vaddr);
+	return NULL;
 }
 
-void __iomem *xtensa_ioremap_nocache(unsigned long addr, unsigned long size)
+int arch_iounmap(void __iomem *addr)
 {
-	return xtensa_ioremap(addr, size, pgprot_noncached(PAGE_KERNEL));
-}
-EXPORT_SYMBOL(xtensa_ioremap_nocache);
+	unsigned long va = (unsigned long) addr;
 
-void __iomem *xtensa_ioremap_cache(unsigned long addr, unsigned long size)
-{
-	return xtensa_ioremap(addr, size, PAGE_KERNEL);
-}
-EXPORT_SYMBOL(xtensa_ioremap_cache);
-
-void xtensa_iounmap(volatile void __iomem *io_addr)
-{
-	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
+	if ((va >= XCHAL_KIO_CACHED_VADDR &&
+	      va - XCHAL_KIO_CACHED_VADDR < XCHAL_KIO_SIZE) ||
+	    (va >= XCHAL_KIO_BYPASS_VADDR &&
+	      va - XCHAL_KIO_BYPASS_VADDR < XCHAL_KIO_SIZE))
+		return -EINVAL;
 
-	vunmap(addr);
+	return 0;
 }
-EXPORT_SYMBOL(xtensa_iounmap);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 05/11] hexagon: mm: Convert to GENERIC_IOREMAP

[Brian Cain]

> -----Original Message-----
> From: Baoquan He <bhe@redhat.com>
...
> With it, the old ioremap() and iounmap() can be perfectly removed
> since they are duplicated with the standard functions.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Brian Cain <bcain@quicinc.com>
> Cc: linux-hexagon@vger.kernel.org
> ---

Acked-by: Brian Cain <bcain@quicinc.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 08/11] parisc: mm: Convert to GENERIC_IOREMAP

[kernel test robot]

Hi Baoquan,

I love your patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: parisc-randconfig-r005-20220820 (https://download.01.org/0day-ci/archive/20220820/202208201135.YyN9CXsu-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/570f2a3347cc83c9ea71d3dbbebfad8ea085ecc6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
        git checkout 570f2a3347cc83c9ea71d3dbbebfad8ea085ecc6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc prepare

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from arch/parisc/include/asm/io.h:315,
                    from include/linux/io.h:13,
                    from include/linux/irq.h:20,
                    from arch/parisc/include/asm/hardirq.h:13,
                    from include/linux/hardirq.h:11,
                    from arch/parisc/kernel/asm-offsets.c:21:
>> include/asm-generic/iomap.h:97: warning: "ioremap_wc" redefined
      97 | #define ioremap_wc ioremap
         | 
   arch/parisc/include/asm/io.h:135: note: this is the location of the previous definition
     135 | #define ioremap_wc(addr, size)  \
         | 
   include/linux/io.h: In function 'pci_remap_cfgspace':
>> include/linux/io.h:89:44: error: implicit declaration of function 'ioremap'; did you mean 'ioremap_np'? [-Werror=implicit-function-declaration]
      89 |         return ioremap_np(offset, size) ?: ioremap(offset, size);
         |                                            ^~~~~~~
         |                                            ioremap_np
>> include/linux/io.h:89:42: warning: pointer/integer type mismatch in conditional expression
      89 |         return ioremap_np(offset, size) ?: ioremap(offset, size);
         |                                          ^
   cc1: some warnings being treated as errors
   make[2]: *** [scripts/Makefile.build:117: arch/parisc/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1207: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:222: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +89 include/linux/io.h

7d3dcf26a6559f Christoph Hellwig 2015-08-10  72  
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  73  #ifdef CONFIG_PCI
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  74  /*
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  75   * The PCI specifications (Rev 3.0, 3.2.5 "Transaction Ordering and
b10eb2d50911f9 Hector Martin     2021-03-25  76   * Posting") mandate non-posted configuration transactions. This default
b10eb2d50911f9 Hector Martin     2021-03-25  77   * implementation attempts to use the ioremap_np() API to provide this
b10eb2d50911f9 Hector Martin     2021-03-25  78   * on arches that support it, and falls back to ioremap() on those that
b10eb2d50911f9 Hector Martin     2021-03-25  79   * don't. Overriding this function is deprecated; arches that properly
b10eb2d50911f9 Hector Martin     2021-03-25  80   * support non-posted accesses should implement ioremap_np() instead, which
b10eb2d50911f9 Hector Martin     2021-03-25  81   * this default implementation can then use to return mappings compliant with
b10eb2d50911f9 Hector Martin     2021-03-25  82   * the PCI specification.
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  83   */
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  84  #ifndef pci_remap_cfgspace
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  85  #define pci_remap_cfgspace pci_remap_cfgspace
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  86  static inline void __iomem *pci_remap_cfgspace(phys_addr_t offset,
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  87  					       size_t size)
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  88  {
b10eb2d50911f9 Hector Martin     2021-03-25 @89  	return ioremap_np(offset, size) ?: ioremap(offset, size);
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  90  }
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  91  #endif
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  92  #endif
cf9ea8ca4a0bea Lorenzo Pieralisi 2017-04-19  93  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Christoph Hellwig]

On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote:
> +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);

Please avoid the overly long lines.

I also wonder if we just want a common definition with a __weak default
instead of duplicating it in many arch headers.

> +	ioaddr = arch_ioremap(phys_addr, size, prot);
> +	if (IS_ERR(ioaddr))
> +		return NULL;
> +	else if (ioaddr)
> +		return ioaddr;

No need for the else here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christoph Hellwig]

> -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);

It seems a bit odd to do this in two steps vs just doing the entire
change in the first patch.  Any good reason for that?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 03/11] mm: ioremap: allow ARCH to have its own ioremap definition

[Christoph Hellwig]

On Sat, Aug 20, 2022 at 08:31:17AM +0800, Baoquan He wrote:
> 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.

Do they?

For arc, the arc_uncached_addr_space case can be easily handled by
arch_ioremap, and the xtensa case looks very similar to that.

I'd really like to kill off arch definitions of ioremap going
forward, as they should just be a special case of ioremap_prot
by definition.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/11] hexagon: mm: Convert to GENERIC_IOREMAP

[Christoph Hellwig]

On Sat, Aug 20, 2022 at 08:31:19AM +0800, Baoquan He wrote:
> With it, the old ioremap() and iounmap() can be perfectly removed
> since they are duplicated with the standard functions.

Can you write a somewhat better commit message explaining that it
switches to the generic code that has equivalent functionality?

> +#define ioremap_uc(addr, size) ioremap((addr), (size))

This is wrong and hexagon should use the asm-generic version of
ioremap_uc that returns NULL.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/11] ia64: mm: Convert to GENERIC_IOREMAP

[Christoph Hellwig]

On Sat, Aug 20, 2022 at 08:31:20AM +0800, Baoquan He wrote:
> 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.

Same comment about the commit log (I won't repeat it as it applies
to all conversions).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Christoph Hellwig]

> +	if (unlikely(!mem_init_done)) {
>  		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> +			return IOMEM_ERR_PTR(ret);
>  		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
>  		fixmaps_used += (size >> PAGE_SHIFT);
>  
> +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
>  			fixmaps_used -= (size >> PAGE_SHIFT);
> +			return IOMEM_ERR_PTR(ret);
> +		}
> +
> +		return (void __iomem *)(offset + (char *)v);
>  	}

This code needs to go away, and all very early boot uses of ioremap
need to switch to use early_ioremap insted.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Christoph Hellwig]

> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>  {
>  	if (!static_branch_unlikely(&have_mio))
> +		return (void __iomem *) *paddr;
> +	return NULL;

This logic isn't new in the patch, but it could really use a comment
as it is rather non-obvious.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Christoph Hellwig]

> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> +#define arch_ioremap arch_ioremap

Shouldn't this still be under CONFIG_MMU?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Christophe Leroy]

Le 20/08/2022 à 02:31, Baoquan He a écrit :
> In some architectures, there are ARCH specifici io address mapping
> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
> openrisc, s390, sh.
> 
> In oder to convert them to take GENERIC_IOREMAP method, we need change
> the return value of hook ioremap_allowed() and iounmap_allowed().
> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
> their current behaviour.

Please don't just say you need to change the return value. Explain why.

And why does it need a name change ? The new name suggests that what was 
simply a check function becomes now a function doing the job. Is that 
the intention ?


> 
> ===
>   arch_ioremap() return a bool,

It is not a bool. A bool is either true or false.

>     - IS_ERR means return an error
>     - NULL means continue to remap
>     - a non-NULL, non-IS_ERR pointer is returned directly
>   arch_iounmap() return a bool,

Same here, not a bool either.

>     - 0 means continue to vunmap
>     - error code means skip vunmap and return directly
> 
> This is taken from Kefeng's below old patch. Christoph suggested the
> return value because he foresaw the doablity of converting to take
> GENERIC_IOREMAP on more architectures.
>   - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
>   - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u
> 
> While at it, the invocation of arch_ioremap() need be moved to the
> beginning of ioremap_prot() because architectures like sh, openrisc,
> ia64, need do the ARCH specific io address mapping on the original
> physical address. And in the later patch, the address fix up code
> in arch_ioremap() also need be done on the original addre on some
> architectures.
> 
> This is preparation for later patch, no functionality change.

No functionnal change, really ?

> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>   arch/arm64/include/asm/io.h |  4 ++--
>   arch/arm64/mm/ioremap.c     | 15 ++++++++++-----
>   include/asm-generic/io.h    | 29 +++++++++++++++--------------
>   mm/ioremap.c                | 12 ++++++++----
>   4 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 877495a0fd0c..dd7e1c2dc86c 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -139,8 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
>    * I/O memory mapping functions.
>    */
>   
> -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot);
> -#define ioremap_allowed ioremap_allowed
> +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
> +#define arch_ioremap arch_ioremap
>   
>   #define _PAGE_IOREMAP PROT_DEVICE_nGnRE
>   
> diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
> index c5af103d4ad4..b0f4cea86f0e 100644
> --- a/arch/arm64/mm/ioremap.c
> +++ b/arch/arm64/mm/ioremap.c
> @@ -3,19 +3,24 @@
>   #include <linux/mm.h>
>   #include <linux/io.h>
>   
> -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot)
> +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
>   {
> -	unsigned long last_addr = phys_addr + size - 1;
> +	unsigned long last_addr, offset;
> +
> +	offset = phys_addr & (~PAGE_MASK);
> +	phys_addr -= offset;
> +	size = PAGE_ALIGN(size + offset);
> +	last_addr = phys_addr + size - 1;
>   
>   	/* Don't allow outside PHYS_MASK */
>   	if (last_addr & ~PHYS_MASK)
> -		return false;
> +		return IOMEM_ERR_PTR(-EINVAL);
>   
>   	/* Don't allow RAM to be mapped. */
>   	if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr))))
> -		return false;
> +		return IOMEM_ERR_PTR(-EINVAL);
>   
> -	return true;
> +	return NULL;
>   }
>   
>   /*
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index a68f8fbf423b..7b6bfb62ef80 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1049,27 +1049,28 @@ static inline void iounmap(volatile void __iomem *addr)
>   
>   /*
>    * Arch code can implement the following two hooks when using GENERIC_IOREMAP
> - * ioremap_allowed() return a bool,
> - *   - true means continue to remap
> - *   - false means skip remap and return directly
> - * iounmap_allowed() return a bool,
> - *   - true means continue to vunmap
> - *   - false means skip vunmap and return directly
> + * arch_ioremap() return a bool,
> + *   - IS_ERR means return an error
> + *   - NULL means continue to remap
> + *   - a non-NULL, non-IS_ERR pointer is returned directly
> + * arch_iounmap() return a bool,
> + *   - 0 means continue to vunmap
> + *   - error code means skip vunmap and return directly
>    */
> -#ifndef ioremap_allowed
> -#define ioremap_allowed ioremap_allowed
> -static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size,
> +#ifndef arch_ioremap
> +#define arch_ioremap arch_ioremap
> +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size,
>   				   unsigned long prot)
>   {
> -	return true;
> +	return NULL;
>   }
>   #endif
>   
> -#ifndef iounmap_allowed
> -#define iounmap_allowed iounmap_allowed
> -static inline bool iounmap_allowed(void *addr)
> +#ifndef arch_iounmap
> +#define arch_iounmap arch_iounmap
> +static inline int arch_iounmap(void __iomem *addr)
>   {
> -	return true;
> +	return 0;
>   }
>   #endif
>   
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 8652426282cc..99fde69becc7 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>   	unsigned long offset, vaddr;
>   	phys_addr_t last_addr;
>   	struct vm_struct *area;
> +	void __iomem *ioaddr;
> +
> +	ioaddr = arch_ioremap(phys_addr, size, prot);
> +	if (IS_ERR(ioaddr))
> +		return NULL;
> +	else if (ioaddr)
> +		return ioaddr;
>   
>   	/* Disallow wrap-around or zero size */
>   	last_addr = phys_addr + size - 1;
> @@ -28,9 +35,6 @@ 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))
> -		return NULL;
> -
>   	area = get_vm_area_caller(size, VM_IOREMAP,
>   			__builtin_return_address(0));
>   	if (!area)
> @@ -52,7 +56,7 @@ void iounmap(volatile void __iomem *addr)
>   {
>   	void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
>   
> -	if (!iounmap_allowed(vaddr))
> +	if (arch_iounmap((void __iomem *)addr))
>   		return;
>   
>   	if (is_vmalloc_addr(vaddr))
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christophe Leroy]

Le 20/08/2022 à 02:31, Baoquan He a écrit :
> On some architectures, the physical address need be fixed up before
> doing mapping, e.g, parisc. And on architectures, e.g arc, the
> parameter 'prot' passed into ioremap_prot() need be adjusted too.
> 
> In oder to convert them to take GENERIC_IOREMAP method, we need wrap
> the address fixing up code and page prot adjusting code into arch_ioremap()
> and pass the new address and 'prot' out for ioremap_prot() handling.

Is it really the best approach ? Wouldn't it be better to have helpers 
to do that, those helpers being called by the ioremap_prot(), instead of 
doing it inside the arch_ioremap() function ?

> 
> This is a preparation patch, no functionality change.

Could this be squashed into previous patch ?

> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>   arch/arm64/include/asm/io.h | 3 ++-
>   arch/arm64/mm/ioremap.c     | 5 +++--
>   include/asm-generic/io.h    | 4 ++--
>   mm/ioremap.c                | 2 +-
>   4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index dd7e1c2dc86c..6a5578ddbbf6 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -139,7 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
>    * I/O memory mapping functions.
>    */
>   
> -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
>   #define arch_ioremap arch_ioremap
>   
>   #define _PAGE_IOREMAP PROT_DEVICE_nGnRE
> diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
> index b0f4cea86f0e..ef75ffef4dbc 100644
> --- a/arch/arm64/mm/ioremap.c
> +++ b/arch/arm64/mm/ioremap.c
> @@ -3,9 +3,10 @@
>   #include <linux/mm.h>
>   #include <linux/io.h>
>   
> -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>   {
> -	unsigned long last_addr, offset;
> +	unsigned long last_addr, offset, phys_addr = *paddr;
>   
>   	offset = phys_addr & (~PAGE_MASK);
>   	phys_addr -= offset;
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 7b6bfb62ef80..fb9bda2be8ed 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1059,8 +1059,8 @@ static inline void iounmap(volatile void __iomem *addr)
>    */
>   #ifndef arch_ioremap
>   #define arch_ioremap arch_ioremap
> -static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size,
> -				   unsigned long prot)
> +static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size,
> +				   unsigned long *prot_val)
>   {
>   	return NULL;
>   }
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 99fde69becc7..7914b5cf5b78 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -19,7 +19,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>   	struct vm_struct *area;
>   	void __iomem *ioaddr;
>   
> -	ioaddr = arch_ioremap(phys_addr, size, prot);
> +	ioaddr = arch_ioremap(&phys_addr, size, &prot);
>   	if (IS_ERR(ioaddr))
>   		return NULL;
>   	else if (ioaddr)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/11] hexagon: mm: Convert to GENERIC_IOREMAP

[Christophe Leroy]

Le 20/08/2022 à 02:31, Baoquan He a écrit :
> With it, the old ioremap() and iounmap() can be perfectly removed
> since they are duplicated with the standard functions.

I see nothing related to the preparation patches. Could this have been 
converted without all previous patches ? In that case I think this one 
should go in the begining of the series, it would help see which 
architectecture really needs the changes to the generic parts.

> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Brian Cain <bcain@quicinc.com>
> 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 c33241425a5c..e2d3091ec9d6 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));
> -}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Sun, 2022-08-21 at 00:05 -0700, Christoph Hellwig wrote:
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> >  {
> >  	if (!static_branch_unlikely(&have_mio))
> > +		return (void __iomem *) *paddr;
> > +	return NULL;
> 
> This logic isn't new in the patch, but it could really use a comment
> as it is rather non-obvious.

Yes, makes sense. Basically we fake MMIO addresses because the s390
architecture doesn't have MMIO as a concept. That is until the PCI MIO
instructions introduced pseudo-MMIO though only for specific PCI
load/store instructions. Without those PCI BAR spaces as well as config
space is accessed with so called function handles. As these are a bad
fit for Linux' MMIO based APIs we create fake MMIO addresses (called
address cookies) that encode an index into the zpci_iomap_start[] which
can be decoded by our implementation of ioread*/iowrite*().

I don't think this is the right place to describe this overall scheme
in detail but maybe we can leave a a good bread crumb. Maybe something
like below?

/* 
 * When PCI MIO instructions are unavailable the "physical" address encodes
 * a hint for accessing the PCI memory space it represents. Just pass it 
 * unchanged such that ioread/iowrite can decode it.
 */


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> Add hooks arch_ioremap() and arch_iounmap() for s390's special
> operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> converted to use ioremap_prot() from GENERIC_IOREMAP.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> ---
>  arch/s390/Kconfig          |  1 +
>  arch/s390/include/asm/io.h | 26 +++++++++++------
>  arch/s390/pci/pci.c        | 60 +++++---------------------------------
>  3 files changed, 26 insertions(+), 61 deletions(-)

Sorry I missed this mail until now and will still need a bit of time to
review and test the code as this is indeed pretty special on s390. From
a first glance this does look like a nice simplification.

Just out of curiosity, I wonder why get_maintainers.pl didn't add me
nor Gerald for direct CC despite the bulk of the changes affecting
arch/s390/pci/*.

> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
---8<---


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Baoquan He]

On 08/20/22 at 11:53pm, Christoph Hellwig wrote:
> On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote:
> > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
> 
> Please avoid the overly long lines.

Thanks for reviewing. Will break the line.

> 
> I also wonder if we just want a common definition with a __weak default
> instead of duplicating it in many arch headers.

Seems __weak symbol is not suggested any more in kernel. Please see
below thread.

[PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add]
https://lore.kernel.org/all/20220518181828.645877-1-naveen.n.rao@linux.vnet.ibm.com/T/#u

> 
> > +	ioaddr = arch_ioremap(phys_addr, size, prot);
> > +	if (IS_ERR(ioaddr))
> > +		return NULL;
> > +	else if (ioaddr)
> > +		return ioaddr;
> 
> No need for the else here.

Do you mean changing it like this? It's fine to me if I get it
correctly.

	ioaddr = arch_ioremap(phys_addr, size, prot);
	if (IS_ERR(ioaddr))
		return NULL;
	if (ioaddr)
		return ioaddr;

Thanks
Baoquan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Baoquan He]

On 08/22/22 at 06:25am, Christophe Leroy wrote:
> 
> 
> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> > In some architectures, there are ARCH specifici io address mapping
> > handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
> > openrisc, s390, sh.
> > 
> > In oder to convert them to take GENERIC_IOREMAP method, we need change
> > the return value of hook ioremap_allowed() and iounmap_allowed().
> > Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
> > their current behaviour.

Thanks for reviewing.

> 
> Please don't just say you need to change the return value. Explain why.

The 1st paragraph and the sentence 'In oder to convert them to take
GENERIC_IOREMAP method' tell the reason, no?


> 
> And why does it need a name change ? The new name suggests that what was 
> simply a check function becomes now a function doing the job. Is that 
> the intention ?

Yes, it's not a simple checking any more. It could do io address mapping
inside arch_ioremap(), and could modify the passed in 'phys_addr' and
'prot' in patch 2. The ioremap_allowed() isn't appropriate to reflect
those.

> 
> 
> > 
> > ===
> >   arch_ioremap() return a bool,
> 
> It is not a bool. A bool is either true or false.

Thanks, I forgot to update this accordingly.

> 
> >     - IS_ERR means return an error
> >     - NULL means continue to remap
> >     - a non-NULL, non-IS_ERR pointer is returned directly
> >   arch_iounmap() return a bool,
> 
> Same here, not a bool either.

And this place.
> 
> >     - 0 means continue to vunmap
> >     - error code means skip vunmap and return directly
> > 
> > This is taken from Kefeng's below old patch. Christoph suggested the
> > return value because he foresaw the doablity of converting to take
> > GENERIC_IOREMAP on more architectures.
> >   - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
> >   - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u
> > 
> > While at it, the invocation of arch_ioremap() need be moved to the
> > beginning of ioremap_prot() because architectures like sh, openrisc,
> > ia64, need do the ARCH specific io address mapping on the original
> > physical address. And in the later patch, the address fix up code
> > in arch_ioremap() also need be done on the original addre on some
> > architectures.
> > 
> > This is preparation for later patch, no functionality change.
> 
> No functionnal change, really ?

You mean the new arch_ioremap() owning different definition or the
invocation of arch_ioremap() moved up is functional change? Now I am
not sure about the latter one, may need update my knowledge base.

Thanks
Baoquan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On 08/20/22 at 11:54pm, Christoph Hellwig wrote:
> > -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> 
> It seems a bit odd to do this in two steps vs just doing the entire
> change in the first patch.  Any good reason for that?

I will merge patch 1~2 or 1~3 into patch 1. Earlier, I wrote the reason
as below paragraph in cover letter of v1 post. Now it seems not so hard
to tell and understand.

====
For patch 1~3, I don't merge them because I made them in different
rounds of changing. And splitting them makes me easily describe the
intention and make review easier. I can merge them after v1 reviewing
if anyone thinks they should be merged.
====


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On 08/22/22 at 06:30am, Christophe Leroy wrote:
> 
> 
> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> > On some architectures, the physical address need be fixed up before
> > doing mapping, e.g, parisc. And on architectures, e.g arc, the
> > parameter 'prot' passed into ioremap_prot() need be adjusted too.
> > 
> > In oder to convert them to take GENERIC_IOREMAP method, we need wrap
> > the address fixing up code and page prot adjusting code into arch_ioremap()
> > and pass the new address and 'prot' out for ioremap_prot() handling.
> 
> Is it really the best approach ? Wouldn't it be better to have helpers 
> to do that, those helpers being called by the ioremap_prot(), instead of 
> doing it inside the arch_ioremap() function ?

This is suggested too by Alexander during his v1 reviewing. I tried, but
feel the current way taken in this patchset is better. Because not all
architecutres need the address fix up, only parisc, and only few need
adjust the 'prot'. Introducing other helpers seems too much, that only
increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
method for people to understand and take.

> 
> > 
> > This is a preparation patch, no functionality change.
> 
> Could this be squashed into previous patch ?

Yep, will do. Thanks.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 03/11] mm: ioremap: allow ARCH to have its own ioremap definition

[Baoquan He]

On 08/20/22 at 11:57pm, Christoph Hellwig wrote:
> On Sat, Aug 20, 2022 at 08:31:17AM +0800, Baoquan He wrote:
> > 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.
> 
> Do they?
> 
> For arc, the arc_uncached_addr_space case can be easily handled by
> arch_ioremap, and the xtensa case looks very similar to that.

I am worried it will impact ioremap_prot(). Arc has selected
HAVE_IOREMAP_PROT in Kconfig. Putting arc_uncached_addr_space() calling
into arch_ioremap() will change ioremap_prot(), right?

And I have the same about xtensa. You can see ioremap() and
ioremap_cache() will return different value since they take
XCHAL_KIO_BYPASS_VADDR and XCHAL_KIO_CACHED_VADDR differently. I haven't
figured out a way to handle them in arch_ioremap() differently.

static inline void __iomem *ioremap(unsigned long offset, unsigned long size)
{
        if (offset >= XCHAL_KIO_PADDR
            && offset - XCHAL_KIO_PADDR < XCHAL_KIO_SIZE)
                return (void*)(offset-XCHAL_KIO_PADDR+XCHAL_KIO_BYPASS_VADDR);
        else
                return xtensa_ioremap_nocache(offset, size);
}

static inline void __iomem *ioremap_cache(unsigned long offset,
                unsigned long size)
{
        if (offset >= XCHAL_KIO_PADDR
            && offset - XCHAL_KIO_PADDR < XCHAL_KIO_SIZE)
                return (void*)(offset-XCHAL_KIO_PADDR+XCHAL_KIO_CACHED_VADDR);
        else
                return xtensa_ioremap_cache(offset, size);
}

> 
> I'd really like to kill off arch definitions of ioremap going
> forward, as they should just be a special case of ioremap_prot
> by definition.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Christophe Leroy]

Le 23/08/2022 à 02:20, Baoquan He a écrit :
> On 08/22/22 at 06:25am, Christophe Leroy wrote:
>>
>>
>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
>>> In some architectures, there are ARCH specifici io address mapping
>>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
>>> openrisc, s390, sh.
>>>
>>> In oder to convert them to take GENERIC_IOREMAP method, we need change
>>> the return value of hook ioremap_allowed() and iounmap_allowed().
>>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
>>> their current behaviour.
> 
> Thanks for reviewing.
> 
>>
>> Please don't just say you need to change the return value. Explain why.
> 
> The 1st paragraph and the sentence 'In oder to convert them to take
> GENERIC_IOREMAP method' tell the reason, no?

What I would like to read is _why_ you need to change the return value 
in order to convert to GENERIC_IOREMAP

> 
> 
>>
>> And why does it need a name change ? The new name suggests that what was
>> simply a check function becomes now a function doing the job. Is that
>> the intention ?
> 
> Yes, it's not a simple checking any more. It could do io address mapping
> inside arch_ioremap(), and could modify the passed in 'phys_addr' and
> 'prot' in patch 2. The ioremap_allowed() isn't appropriate to reflect
> those.

Fair enough, then all this needs to be explained in the commit message.

> 
>>
>>
>>>
>>> ===
>>>    arch_ioremap() return a bool,
>>
>> It is not a bool. A bool is either true or false.
> 
> Thanks, I forgot to update this accordingly.
> 
>>
>>>      - IS_ERR means return an error
>>>      - NULL means continue to remap
>>>      - a non-NULL, non-IS_ERR pointer is returned directly
>>>    arch_iounmap() return a bool,
>>
>> Same here, not a bool either.
> 
> And this place.
>>
>>>      - 0 means continue to vunmap
>>>      - error code means skip vunmap and return directly
>>>
>>> This is taken from Kefeng's below old patch. Christoph suggested the
>>> return value because he foresaw the doablity of converting to take
>>> GENERIC_IOREMAP on more architectures.
>>>    - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
>>>    - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u
>>>
>>> While at it, the invocation of arch_ioremap() need be moved to the
>>> beginning of ioremap_prot() because architectures like sh, openrisc,
>>> ia64, need do the ARCH specific io address mapping on the original
>>> physical address. And in the later patch, the address fix up code
>>> in arch_ioremap() also need be done on the original addre on some
>>> architectures.
>>>
>>> This is preparation for later patch, no functionality change.
>>
>> No functionnal change, really ?
> 
> You mean the new arch_ioremap() owning different definition or the
> invocation of arch_ioremap() moved up is functional change? Now I am
> not sure about the latter one, may need update my knowledge base.

Both indeed. I understand that this first step is not changing much to 
the logic, but I think the simple fact to change the arguments and name 
are some how a functionnal change.

> 
> Thanks
> Baoquan
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christophe Leroy]

Le 23/08/2022 à 03:19, Baoquan He a écrit :
> On 08/22/22 at 06:30am, Christophe Leroy wrote:
>>
>>
>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
>>> On some architectures, the physical address need be fixed up before
>>> doing mapping, e.g, parisc. And on architectures, e.g arc, the
>>> parameter 'prot' passed into ioremap_prot() need be adjusted too.
>>>
>>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap
>>> the address fixing up code and page prot adjusting code into arch_ioremap()
>>> and pass the new address and 'prot' out for ioremap_prot() handling.
>>
>> Is it really the best approach ? Wouldn't it be better to have helpers
>> to do that, those helpers being called by the ioremap_prot(), instead of
>> doing it inside the arch_ioremap() function ?
> 
> This is suggested too by Alexander during his v1 reviewing. I tried, but
> feel the current way taken in this patchset is better. Because not all
> architecutres need the address fix up, only parisc, and only few need
> adjust the 'prot'. Introducing other helpers seems too much, that only
> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
> method for people to understand and take.

I can't understand. Why is it difficult to do something like:

#ifndef ioremap_adjust_prot
static inline unsigned long ioremap_adjust_prot(unsigned long flags)
{
	return flags;
}
#endif

Then for arc you do

static inline unsigned long ioremap_adjust_prot(unsigned long flags)
{
	return pgprot_val(pgprot_noncached(__pgprot(flags)));
}
#define ioremap_adjust_prot ioremap_adjust_prot


By the way, could be a good opportunity to change ioremap_prot() flags 
type from unsigned long to pgprot_t

> 
>>
>>>
>>> This is a preparation patch, no functionality change.
>>
>> Could this be squashed into previous patch ?
> 
> Yep, will do. Thanks.
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Niklas Schnelle]

On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> Add hooks arch_ioremap() and arch_iounmap() for s390's special
> operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> converted to use ioremap_prot() from GENERIC_IOREMAP.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: linux-s390@vger.kernel.org
> ---
>  arch/s390/Kconfig          |  1 +
>  arch/s390/include/asm/io.h | 26 +++++++++++------
>  arch/s390/pci/pci.c        | 60 +++++---------------------------------
>  3 files changed, 26 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 318fce77601d..c59e1b25f59d 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -135,6 +135,7 @@ config S390
>  	select GENERIC_SMP_IDLE_THREAD
>  	select GENERIC_TIME_VSYSCALL
>  	select GENERIC_VDSO_TIME_NS
> +	select GENERIC_IOREMAP
>  	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
>  	select HAVE_ARCH_AUDITSYSCALL
>  	select HAVE_ARCH_JUMP_LABEL
> diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
> index e3882b012bfa..f837e20b7bbd 100644
> --- a/arch/s390/include/asm/io.h
> +++ b/arch/s390/include/asm/io.h
> @@ -22,11 +22,23 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
>  
>  #define IO_SPACE_LIMIT 0
>  
> -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
> -void __iomem *ioremap(phys_addr_t addr, size_t size);
> -void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
> -void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
> -void iounmap(volatile void __iomem *addr);
> +

Checkpatch nitpick, remove the empty line addition above so as not to
create two consecutive empty lines.

> +/*
> + * I/O memory mapping functions.
> + */
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> +#define arch_ioremap arch_ioremap
> +
> +int arch_iounmap(void __iomem *addr);
> +#define arch_iounmap arch_iounmap
> +
> +#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
> +
> +#define ioremap_wc(addr, size)  \
> +	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
> +#define ioremap_wt(addr, size)  \
> +	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
>  
>  static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
>  {
> @@ -51,10 +63,6 @@ static inline void ioport_unmap(void __iomem *p)
>  #define pci_iomap_wc pci_iomap_wc
>  #define pci_iomap_wc_range pci_iomap_wc_range
>  
> -#define ioremap ioremap
> -#define ioremap_wt ioremap_wt
> -#define ioremap_wc ioremap_wc
> -
>  #define memcpy_fromio(dst, src, count)	zpci_memcpy_fromio(dst, src, count)
>  #define memcpy_toio(dst, src, count)	zpci_memcpy_toio(dst, src, count)
>  #define memset_io(dst, val, count)	zpci_memset_io(dst, val, count)
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 73cdc5539384..984cad9cd5a1 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -244,64 +244,20 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
>         zpci_memcpy_toio(to, from, count);
>  }
>  
> -static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>  {
> -	unsigned long offset, vaddr;
> -	struct vm_struct *area;
> -	phys_addr_t last_addr;
> -
> -	last_addr = addr + size - 1;
> -	if (!size || last_addr < addr)
> -		return NULL;
> -
>  	if (!static_branch_unlikely(&have_mio))
> -		return (void __iomem *) addr;
> -
> -	offset = addr & ~PAGE_MASK;
> -	addr &= PAGE_MASK;
> -	size = PAGE_ALIGN(size + offset);
> -	area = get_vm_area(size, VM_IOREMAP);
> -	if (!area)
> -		return NULL;
> -
> -	vaddr = (unsigned long) area->addr;
> -	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
> -		free_vm_area(area);
> -		return NULL;
> -	}
> -	return (void __iomem *) ((unsigned long) area->addr + offset);
> -}
> -
> -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
> -{
> -	return __ioremap(addr, size, __pgprot(prot));
> +		return (void __iomem *) *paddr;

Another checkpatch nitpick no space after the cast.

> +	return NULL;
>  }
> -EXPORT_SYMBOL(ioremap_prot);
>  
> -void __iomem *ioremap(phys_addr_t addr, size_t size)
> +int arch_iounmap(void __iomem *addr)
>  {
> -	return __ioremap(addr, size, PAGE_KERNEL);
> -}
> -EXPORT_SYMBOL(ioremap);
> -
> -void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
> -{
> -	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
> -}
> -EXPORT_SYMBOL(ioremap_wc);
> -
> -void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
> -{
> -	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
> -}
> -EXPORT_SYMBOL(ioremap_wt);
> -
> -void iounmap(volatile void __iomem *addr)
> -{
> -	if (static_branch_likely(&have_mio))
> -		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
> +	if (!static_branch_likely(&have_mio))
> +		return -EINVAL;

As Christoph suggested this might be a good opportunity to add a
comment for this branch.

One other nitpick. The return value doesn't really matter here since
anything != NULL turns iounmap() into a no-op so this looks correct but
semantically I think returning -EINVAL wrongly suggests that addr was
invalid. Maybe -ENXIO would be better at conveying that there is
nothing to unmap.

Looking at your patch 1 another idea would be to have 3 kinds of return
values for arch_iounmap() too e.g.:

 arch_iounmap() return an __iomem pointer
   - IS_ERR means skip vunmap and return directly
   - NULL means continue to vunmap
   - a non-NULL, non-IS_ERR pointer has been unmapped successfully

Then we would simply return addr in case of
!static_branch_likely(&have_mio) and NULL otherwise.

What do you think? Either way no strong opinion on my side,
functionally it makes no difference.

> +	return 0;
>  }
> -EXPORT_SYMBOL(iounmap);
>  
>  /* Create a virtual mapping cookie for a PCI BAR */
>  static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,

Apart from the above nitpicks and suggestion this looks good to me.
I did also test this with and without PCI MIO support including use of
PCI MIO instructions in user-space (added in rdma-core v40).

So feel free to add:
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>

Since it looks like there will be a v3 due to other comments anyway
please Cc me on that directly and I'm sure I can upgrade the Acked-by
to Reviewed-by when we're closer to the final code.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On 08/23/22 at 05:33am, Christophe Leroy wrote:
> 
> 
> Le 23/08/2022 à 03:19, Baoquan He a écrit :
> > On 08/22/22 at 06:30am, Christophe Leroy wrote:
> >>
> >>
> >> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> >>> On some architectures, the physical address need be fixed up before
> >>> doing mapping, e.g, parisc. And on architectures, e.g arc, the
> >>> parameter 'prot' passed into ioremap_prot() need be adjusted too.
> >>>
> >>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap
> >>> the address fixing up code and page prot adjusting code into arch_ioremap()
> >>> and pass the new address and 'prot' out for ioremap_prot() handling.
> >>
> >> Is it really the best approach ? Wouldn't it be better to have helpers
> >> to do that, those helpers being called by the ioremap_prot(), instead of
> >> doing it inside the arch_ioremap() function ?
> > 
> > This is suggested too by Alexander during his v1 reviewing. I tried, but
> > feel the current way taken in this patchset is better. Because not all
> > architecutres need the address fix up, only parisc, and only few need
> > adjust the 'prot'. Introducing other helpers seems too much, that only
> > increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
> > method for people to understand and take.
> 
> I can't understand. Why is it difficult to do something like:
> 
> #ifndef ioremap_adjust_prot
> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> {
> 	return flags;
> }
> #endif
> 
> Then for arc you do
> 
> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> {
> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
> }
> #define ioremap_adjust_prot ioremap_adjust_prot

My thinking is we have four things to do in the added hookers.
1) check if we should do ioremap on ARCHes. If not, return NULL from
ioremap_prot();
2) handling the mapping io address specifically on ARCHes, e.g arc,
ia64, s390;
3) the original physical address passed into ioremap_prot() need be
fixed up, e.g arc;
4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
and xtensa.

With Kefeng's patches, the case 1) is handled with introduced
ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
case is not difficult from my side. I worry that as time goes by, those
several hooks my cause issue, e.g if a new adjustment need be done,
should we introduce a new helper or make do with the existed hook; how 

When I investigated this, one arch_ioremap() looks not complicated 
since not all ARCHes need cover all above 4 cases. That's why I finally
choose one hook. I am open to new idea, please let me know if we should
change it to introduce several different helpers.

> 
> 
> By the way, could be a good opportunity to change ioremap_prot() flags 
> type from unsigned long to pgprot_t

Tend to agree, I will give it a shot.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Baoquan He]

On 08/23/22 at 05:24am, Christophe Leroy wrote:
> 
> 
> Le 23/08/2022 à 02:20, Baoquan He a écrit :
> > On 08/22/22 at 06:25am, Christophe Leroy wrote:
> >>
> >>
> >> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> >>> In some architectures, there are ARCH specifici io address mapping
> >>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
> >>> openrisc, s390, sh.
> >>>
> >>> In oder to convert them to take GENERIC_IOREMAP method, we need change
> >>> the return value of hook ioremap_allowed() and iounmap_allowed().
> >>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
> >>> their current behaviour.
> > 
> > Thanks for reviewing.
> > 
> >>
> >> Please don't just say you need to change the return value. Explain why.
> > 
> > The 1st paragraph and the sentence 'In oder to convert them to take
> > GENERIC_IOREMAP method' tell the reason, no?
> 
> What I would like to read is _why_ you need to change the return value 
> in order to convert to GENERIC_IOREMAP

I rephrase the log as below, it's OK to you? Or please help check and
tell what I need to improve to better explain the reason.

====
The current io[re|un]map_allowed() hooks are used to check if the
io[re|un]map() actions are qualified to proceed when taking
GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map()
will return NULL.

On some architectures like arc, ia64, openris, s390, sh, there are
ARCH specific io address mapping to translate the passed in physical
address to io address when calling ioremap(). In order to convert
these architectures to take GENERIC_IOREMAP way to ioremap(), we need
change the return value of hook ioremap_allowed() and iounmap_allowed().
With the change, we can move the architecture specific io address
mapping into ioremap_allowed() hook, and give the mapped io address
out to let ioremap_prot() return it. While at it, rename the hooks to
arch_ioremap() and arch_iounmap() to reflect their new behaviour.
====


> 
> > 
> > 
> >>
> >> And why does it need a name change ? The new name suggests that what was
> >> simply a check function becomes now a function doing the job. Is that
> >> the intention ?
> > 
> > Yes, it's not a simple checking any more. It could do io address mapping
> > inside arch_ioremap(), and could modify the passed in 'phys_addr' and
> > 'prot' in patch 2. The ioremap_allowed() isn't appropriate to reflect
> > those.
> 
> Fair enough, then all this needs to be explained in the commit message.

Sure. After we decide the hooks, I will update the log accordingly.

> 
> > 
> >>
> >>
> >>>
> >>> ===
> >>>    arch_ioremap() return a bool,
> >>
> >> It is not a bool. A bool is either true or false.
> > 
> > Thanks, I forgot to update this accordingly.
> > 
> >>
> >>>      - IS_ERR means return an error
> >>>      - NULL means continue to remap
> >>>      - a non-NULL, non-IS_ERR pointer is returned directly
> >>>    arch_iounmap() return a bool,
> >>
> >> Same here, not a bool either.
> > 
> > And this place.
> >>
> >>>      - 0 means continue to vunmap
> >>>      - error code means skip vunmap and return directly
> >>>
> >>> This is taken from Kefeng's below old patch. Christoph suggested the
> >>> return value because he foresaw the doablity of converting to take
> >>> GENERIC_IOREMAP on more architectures.
> >>>    - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
> >>>    - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u
> >>>
> >>> While at it, the invocation of arch_ioremap() need be moved to the
> >>> beginning of ioremap_prot() because architectures like sh, openrisc,
> >>> ia64, need do the ARCH specific io address mapping on the original
> >>> physical address. And in the later patch, the address fix up code
> >>> in arch_ioremap() also need be done on the original addre on some
> >>> architectures.
> >>>
> >>> This is preparation for later patch, no functionality change.
> >>
> >> No functionnal change, really ?
> > 
> > You mean the new arch_ioremap() owning different definition or the
> > invocation of arch_ioremap() moved up is functional change? Now I am
> > not sure about the latter one, may need update my knowledge base.
> 
> Both indeed. I understand that this first step is not changing much to 
> the logic, but I think the simple fact to change the arguments and name 
> are some how a functionnal change.

OK, I thought the function interface change is not related to functional
change. I will remove the 'no functionality change' sentence to avoid
misleading. Thanks.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Christophe Leroy]

Le 23/08/2022 à 17:14, Baoquan He a écrit :
> On 08/23/22 at 05:24am, Christophe Leroy wrote:
>>
>>
>> Le 23/08/2022 à 02:20, Baoquan He a écrit :
>>> On 08/22/22 at 06:25am, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
>>>>> In some architectures, there are ARCH specifici io address mapping
>>>>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
>>>>> openrisc, s390, sh.
>>>>>
>>>>> In oder to convert them to take GENERIC_IOREMAP method, we need change
>>>>> the return value of hook ioremap_allowed() and iounmap_allowed().
>>>>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
>>>>> their current behaviour.
>>>
>>> Thanks for reviewing.
>>>
>>>>
>>>> Please don't just say you need to change the return value. Explain why.
>>>
>>> The 1st paragraph and the sentence 'In oder to convert them to take
>>> GENERIC_IOREMAP method' tell the reason, no?
>>
>> What I would like to read is _why_ you need to change the return value
>> in order to convert to GENERIC_IOREMAP
> 
> I rephrase the log as below, it's OK to you? Or please help check and
> tell what I need to improve to better explain the reason.
> 
> ====
> The current io[re|un]map_allowed() hooks are used to check if the
> io[re|un]map() actions are qualified to proceed when taking
> GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map()
> will return NULL.
> 
> On some architectures like arc, ia64, openris, s390, sh, there are
> ARCH specific io address mapping to translate the passed in physical
> address to io address when calling ioremap(). In order to convert
> these architectures to take GENERIC_IOREMAP way to ioremap(), we need
> change the return value of hook ioremap_allowed() and iounmap_allowed().
> With the change, we can move the architecture specific io address
> mapping into ioremap_allowed() hook, and give the mapped io address
> out to let ioremap_prot() return it. While at it, rename the hooks to
> arch_ioremap() and arch_iounmap() to reflect their new behaviour.
> ====
> 

That looks more in line with the type of explanation I foresee in the 
commit message, thanks.


Christophe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christophe Leroy]

Le 23/08/2022 à 14:32, Baoquan He a écrit :
> On 08/23/22 at 05:33am, Christophe Leroy wrote:
>>
>>
>> Le 23/08/2022 à 03:19, Baoquan He a écrit :
>>> On 08/22/22 at 06:30am, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
>>>>> On some architectures, the physical address need be fixed up before
>>>>> doing mapping, e.g, parisc. And on architectures, e.g arc, the
>>>>> parameter 'prot' passed into ioremap_prot() need be adjusted too.
>>>>>
>>>>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap
>>>>> the address fixing up code and page prot adjusting code into arch_ioremap()
>>>>> and pass the new address and 'prot' out for ioremap_prot() handling.
>>>>
>>>> Is it really the best approach ? Wouldn't it be better to have helpers
>>>> to do that, those helpers being called by the ioremap_prot(), instead of
>>>> doing it inside the arch_ioremap() function ?
>>>
>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
>>> feel the current way taken in this patchset is better. Because not all
>>> architecutres need the address fix up, only parisc, and only few need
>>> adjust the 'prot'. Introducing other helpers seems too much, that only
>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
>>> method for people to understand and take.
>>
>> I can't understand. Why is it difficult to do something like:
>>
>> #ifndef ioremap_adjust_prot
>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>> {
>> 	return flags;
>> }
>> #endif
>>
>> Then for arc you do
>>
>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>> {
>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
>> }
>> #define ioremap_adjust_prot ioremap_adjust_prot
> 
> My thinking is we have four things to do in the added hookers.
> 1) check if we should do ioremap on ARCHes. If not, return NULL from
> ioremap_prot();
> 2) handling the mapping io address specifically on ARCHes, e.g arc,
> ia64, s390;
> 3) the original physical address passed into ioremap_prot() need be
> fixed up, e.g arc;
> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
> and xtensa.
> 
> With Kefeng's patches, the case 1) is handled with introduced
> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
> case is not difficult from my side. I worry that as time goes by, those
> several hooks my cause issue, e.g if a new adjustment need be done,
> should we introduce a new helper or make do with the existed hook; how
> 
> When I investigated this, one arch_ioremap() looks not complicated
> since not all ARCHes need cover all above 4 cases. That's why I finally
> choose one hook. I am open to new idea, please let me know if we should
> change it to introduce several different helpers.
> 

A new idea that would have my preference would be to do just like we did 
with arch_get_unmapped_area(). Look at 
https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 
and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131

Instead of having the generic that calls the arch specific, make it the 
other way round, have the arch specific call the generic after doing its 
specialties.

>>
>>
>> By the way, could be a good opportunity to change ioremap_prot() flags
>> type from unsigned long to pgprot_t
> 
> Tend to agree, I will give it a shot.
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[David Laight]

From: Christophe Leroy
> Sent: 23 August 2022 16:26
> 
> Le 23/08/2022 à 17:14, Baoquan He a écrit :
> > On 08/23/22 at 05:24am, Christophe Leroy wrote:
> >>
> >>
> >> Le 23/08/2022 à 02:20, Baoquan He a écrit :
> >>> On 08/22/22 at 06:25am, Christophe Leroy wrote:
> >>>>
> >>>>
> >>>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> >>>>> In some architectures, there are ARCH specifici io address mapping
> >>>>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
> >>>>> openrisc, s390, sh.
> >>>>>
> >>>>> In oder to convert them to take GENERIC_IOREMAP method, we need change
> >>>>> the return value of hook ioremap_allowed() and iounmap_allowed().
> >>>>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
> >>>>> their current behaviour.
> >>>
> >>> Thanks for reviewing.
> >>>
> >>>>
> >>>> Please don't just say you need to change the return value. Explain why.
> >>>
> >>> The 1st paragraph and the sentence 'In oder to convert them to take
> >>> GENERIC_IOREMAP method' tell the reason, no?
> >>
> >> What I would like to read is _why_ you need to change the return value
> >> in order to convert to GENERIC_IOREMAP
> >
> > I rephrase the log as below, it's OK to you? Or please help check and
> > tell what I need to improve to better explain the reason.
> >
> > ====
> > The current io[re|un]map_allowed() hooks are used to check if the
> > io[re|un]map() actions are qualified to proceed when taking
> > GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map()
> > will return NULL.
> >
> > On some architectures like arc, ia64, openris, s390, sh, there are
> > ARCH specific io address mapping to translate the passed in physical
> > address to io address when calling ioremap(). In order to convert
> > these architectures to take GENERIC_IOREMAP way to ioremap(), we need
> > change the return value of hook ioremap_allowed() and iounmap_allowed().
> > With the change, we can move the architecture specific io address
> > mapping into ioremap_allowed() hook, and give the mapped io address
> > out to let ioremap_prot() return it. While at it, rename the hooks to
> > arch_ioremap() and arch_iounmap() to reflect their new behaviour.
> > ====
> >
> 
> That looks more in line with the type of explanation I foresee in the
> commit message, thanks.

I think you also need to summarise the change itself.
If the success/fail return actually changes then you really
need to change something so the compiler errors unchanged code.
Otherwise it is a complete recipe for disaster.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Alexander Gordeev]

On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote:

Hi Baoquan,

>  arch_ioremap() return a bool,
>    - IS_ERR means return an error
>    - NULL means continue to remap
>    - a non-NULL, non-IS_ERR pointer is returned directly
>  arch_iounmap() return a bool,
>    - 0 means continue to vunmap
>    - error code means skip vunmap and return directly

It would make more sense if the return values were described
from the prospective of an architecture, not the caller.
I.e true - unmapped, false - not supported, etc.

> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 8652426282cc..99fde69becc7 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
>  	unsigned long offset, vaddr;
>  	phys_addr_t last_addr;
>  	struct vm_struct *area;
> +	void __iomem *ioaddr;
> +
> +	ioaddr = arch_ioremap(phys_addr, size, prot);
> +	if (IS_ERR(ioaddr))
> +		return NULL;
> +	else if (ioaddr)
> +		return ioaddr;

It seems to me arch_ioremap() could simply return an address
or an error. Then IOMEM_ERR_PTR(-ENOSYS) if the architecture
does not support it reads much better than the cryptic NULL.

Probably arch_iounmap() returning error would look better too,
though not sure about that.

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Baoquan He]

On 08/28/22 at 10:36am, Alexander Gordeev wrote:
> On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote:
> 
> Hi Baoquan,
> 
> >  arch_ioremap() return a bool,
> >    - IS_ERR means return an error
> >    - NULL means continue to remap
> >    - a non-NULL, non-IS_ERR pointer is returned directly
> >  arch_iounmap() return a bool,
> >    - 0 means continue to vunmap
> >    - error code means skip vunmap and return directly
> 
> It would make more sense if the return values were described
> from the prospective of an architecture, not the caller.
> I.e true - unmapped, false - not supported, etc.

Yes, sounds reasonable to me, thanks.

While ChristopheL suggested to take another way. Please see below link.
I will reply to Christophe to discuss that.

https://lore.kernel.org/all/8df89136-a7f2-9b66-d522-a4fb9860bf22@csgroup.eu/T/#u

If the current arch_ioremap() way is taken, I will change the
description as you said.

> 
> > diff --git a/mm/ioremap.c b/mm/ioremap.c
> > index 8652426282cc..99fde69becc7 100644
> > --- a/mm/ioremap.c
> > +++ b/mm/ioremap.c
> > @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
> >  	unsigned long offset, vaddr;
> >  	phys_addr_t last_addr;
> >  	struct vm_struct *area;
> > +	void __iomem *ioaddr;
> > +
> > +	ioaddr = arch_ioremap(phys_addr, size, prot);
> > +	if (IS_ERR(ioaddr))
> > +		return NULL;
> > +	else if (ioaddr)
> > +		return ioaddr;
> 
> It seems to me arch_ioremap() could simply return an address
> or an error. Then IOMEM_ERR_PTR(-ENOSYS) if the architecture
> does not support it reads much better than the cryptic NULL.

I may not follow. Returning NULL means arch_ioremap() doesn't give out a
mapped address and doesn't encounter wrong thing. NULL is a little
twisting, maybe '0' is better?

> 
> Probably arch_iounmap() returning error would look better too,
> though not sure about that.

Don't follow either. arch_iounmap() is returning error now. 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On 08/23/22 at 07:03pm, Christophe Leroy wrote:
> 
> 
> Le 23/08/2022 à 14:32, Baoquan He a écrit :
> > On 08/23/22 at 05:33am, Christophe Leroy wrote:
> >>
> >>
> >> Le 23/08/2022 à 03:19, Baoquan He a écrit :
> >>> On 08/22/22 at 06:30am, Christophe Leroy wrote:
> >>>>
> >>>>
> >>>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> >>>>> On some architectures, the physical address need be fixed up before
> >>>>> doing mapping, e.g, parisc. And on architectures, e.g arc, the
> >>>>> parameter 'prot' passed into ioremap_prot() need be adjusted too.
> >>>>>
> >>>>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap
> >>>>> the address fixing up code and page prot adjusting code into arch_ioremap()
> >>>>> and pass the new address and 'prot' out for ioremap_prot() handling.
> >>>>
> >>>> Is it really the best approach ? Wouldn't it be better to have helpers
> >>>> to do that, those helpers being called by the ioremap_prot(), instead of
> >>>> doing it inside the arch_ioremap() function ?
> >>>
> >>> This is suggested too by Alexander during his v1 reviewing. I tried, but
> >>> feel the current way taken in this patchset is better. Because not all
> >>> architecutres need the address fix up, only parisc, and only few need
> >>> adjust the 'prot'. Introducing other helpers seems too much, that only
> >>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
> >>> method for people to understand and take.
> >>
> >> I can't understand. Why is it difficult to do something like:
> >>
> >> #ifndef ioremap_adjust_prot
> >> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> >> {
> >> 	return flags;
> >> }
> >> #endif
> >>
> >> Then for arc you do
> >>
> >> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> >> {
> >> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
> >> }
> >> #define ioremap_adjust_prot ioremap_adjust_prot
> > 
> > My thinking is we have four things to do in the added hookers.
> > 1) check if we should do ioremap on ARCHes. If not, return NULL from
> > ioremap_prot();
> > 2) handling the mapping io address specifically on ARCHes, e.g arc,
> > ia64, s390;
> > 3) the original physical address passed into ioremap_prot() need be
> > fixed up, e.g arc;
> > 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
> > and xtensa.
> > 
> > With Kefeng's patches, the case 1) is handled with introduced
> > ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
> > rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
> > 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
> > case is not difficult from my side. I worry that as time goes by, those
> > several hooks my cause issue, e.g if a new adjustment need be done,
> > should we introduce a new helper or make do with the existed hook; how
> > 
> > When I investigated this, one arch_ioremap() looks not complicated
> > since not all ARCHes need cover all above 4 cases. That's why I finally
> > choose one hook. I am open to new idea, please let me know if we should
> > change it to introduce several different helpers.
> > 
> 
> A new idea that would have my preference would be to do just like we did 
> with arch_get_unmapped_area(). Look at 
> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 
> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
> 
> Instead of having the generic that calls the arch specific, make it the 
> other way round, have the arch specific call the generic after doing its 
> specialties.

This sounds good. I made a draft patch to change code in generic code
part, just showing what it looks like.

Both arch_ioremap() way and the arch sepcific call the generic way look
good to me. Just it will take less effort for me to continue the
arch_ioremap() way. I would like to hear Christoph's opinion since he
introduced the GENERIC_IOREMAP method and suggested the earlier
arch_ioremap() way and change in this patchset. 

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 68a8117b30fa..4bc3e18c475f 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1047,35 +1047,18 @@ static inline void iounmap(volatile void __iomem *addr)
 #elif defined(CONFIG_GENERIC_IOREMAP)
 #include <linux/pgtable.h>
 
-/*
- * Arch code can implement the following two hooks when using GENERIC_IOREMAP
- * arch_ioremap() return a bool,
- *   - IS_ERR means return an error
- *   - NULL means continue to remap
- *   - a non-NULL, non-IS_ERR pointer is returned directly
- * arch_iounmap() return a bool,
- *   - 0 means continue to vunmap
- *   - error code means skip vunmap and return directly
- */
-#ifndef arch_ioremap
-#define arch_ioremap arch_ioremap
-static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size,
-				   unsigned long *prot_val)
-{
-	return NULL;
-}
-#endif
+void __iomem *
+generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot);
 
-#ifndef arch_iounmap
-#define arch_iounmap arch_iounmap
-static inline int arch_iounmap(void __iomem *addr)
+#ifndef ioremap_prot
+#define ioremap_prot ioremap_prot
+void __iomem *
+ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot);
 {
-	return 0;
+	return generic_ioremap_prot(phys_addr, size, prot);
 }
 #endif
 
-void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
-			   unsigned long prot);
 void iounmap(volatile void __iomem *addr);
 
 #ifndef ioremap
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 7914b5cf5b78..87d51003dee6 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, unsigned long prot)
 {
 	unsigned long offset, vaddr;
 	phys_addr_t last_addr;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Baoquan He]

Hi David,

On 08/24/22 at 08:16am, David Laight wrote:
......
> > >>>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> > >>>>> In some architectures, there are ARCH specifici io address mapping
> > >>>>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
> > >>>>> openrisc, s390, sh.
> > >>>>>
> > >>>>> In oder to convert them to take GENERIC_IOREMAP method, we need change
> > >>>>> the return value of hook ioremap_allowed() and iounmap_allowed().
> > >>>>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
> > >>>>> their current behaviour.
> > >>>
> > >>> Thanks for reviewing.
> > >>>
> > >>>>
> > >>>> Please don't just say you need to change the return value. Explain why.
> > >>>
> > >>> The 1st paragraph and the sentence 'In oder to convert them to take
> > >>> GENERIC_IOREMAP method' tell the reason, no?
> > >>
> > >> What I would like to read is _why_ you need to change the return value
> > >> in order to convert to GENERIC_IOREMAP
> > >
> > > I rephrase the log as below, it's OK to you? Or please help check and
> > > tell what I need to improve to better explain the reason.
> > >
> > > ====
> > > The current io[re|un]map_allowed() hooks are used to check if the
> > > io[re|un]map() actions are qualified to proceed when taking
> > > GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map()
> > > will return NULL.
> > >
> > > On some architectures like arc, ia64, openris, s390, sh, there are
> > > ARCH specific io address mapping to translate the passed in physical
> > > address to io address when calling ioremap(). In order to convert
> > > these architectures to take GENERIC_IOREMAP way to ioremap(), we need
> > > change the return value of hook ioremap_allowed() and iounmap_allowed().
> > > With the change, we can move the architecture specific io address
> > > mapping into ioremap_allowed() hook, and give the mapped io address
> > > out to let ioremap_prot() return it. While at it, rename the hooks to
> > > arch_ioremap() and arch_iounmap() to reflect their new behaviour.
> > > ====
> > >
> > 
> > That looks more in line with the type of explanation I foresee in the
> > commit message, thanks.
> 
> I think you also need to summarise the change itself.
> If the success/fail return actually changes then you really
> need to change something so the compiler errors unchanged code.
> Otherwise it is a complete recipe for disaster.

Thanks for looking into this and sorry for late response.

I am not sure if I follow you. In this patch, I just rename the old
ioremap_allowed() to arch_ioremap(), and change its return value. Except
of arm64 which has taken GENERIC_IOREMAP way to provide
ioremap_allowed(), no other ARCHes are affected yet. This is what have
been changed. Could you be more specific what I should add?

Or are you suggesting words like below sentences need be added to patch
log?

If the success/fail return actually changes then you really
need to change something so the compiler errors unchanged code.
Otherwise it is a complete recipe for disaster.

Thanks
Baoquan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/11] hexagon: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/21/22 at 12:00am, Christoph Hellwig wrote:
> On Sat, Aug 20, 2022 at 08:31:19AM +0800, Baoquan He wrote:
> > With it, the old ioremap() and iounmap() can be perfectly removed
> > since they are duplicated with the standard functions.
> 
> Can you write a somewhat better commit message explaining that it
> switches to the generic code that has equivalent functionality?

OK, I will rephrase like below. Please check if it's OK.
==
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 iounma(), and has the equivalent functioality
as before.

> 
> > +#define ioremap_uc(addr, size) ioremap((addr), (size))
> 
> This is wrong and hexagon should use the asm-generic version of
> ioremap_uc that returns NULL.

I don't follow. Do you mean the function verion, but not a macro? Or
define it like below?

#define ioremap_uc(addr, size)  \
        ioremap_prot((addr), (size), _PAGE_IOREMAP)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/11] hexagon: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/22/22 at 06:38am, Christophe Leroy wrote:
> 
> 
> Le 20/08/2022 à 02:31, Baoquan He a écrit :
> > With it, the old ioremap() and iounmap() can be perfectly removed
> > since they are duplicated with the standard functions.
> 
> I see nothing related to the preparation patches. Could this have been 
> converted without all previous patches ? In that case I think this one 
> should go in the begining of the series, it would help see which 
> architectecture really needs the changes to the generic parts.

You are right. Hexagon basically has the same implementation as standard
ioremap and iounmap code. I will move this at the beginning of patch
series in next round of post. Thanks for point this out.

> 
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: Brian Cain <bcain@quicinc.com>
> > 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 c33241425a5c..e2d3091ec9d6 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));
> > -}


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/11] ia64: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/21/22 at 12:02am, Christoph Hellwig wrote:
> On Sat, Aug 20, 2022 at 08:31:20AM +0800, Baoquan He wrote:
> > 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.
> 
> Same comment about the commit log (I won't repeat it as it applies
> to all conversions).

Will do.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/21/22 at 12:03am, Christoph Hellwig wrote:
> > +	if (unlikely(!mem_init_done)) {
> >  		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> > +			return IOMEM_ERR_PTR(ret);
> >  		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
> >  		fixmaps_used += (size >> PAGE_SHIFT);
> >  
> > +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
> >  			fixmaps_used -= (size >> PAGE_SHIFT);
> > +			return IOMEM_ERR_PTR(ret);
> > +		}
> > +
> > +		return (void __iomem *)(offset + (char *)v);
> >  	}
> 
> This code needs to go away, and all very early boot uses of ioremap
> need to switch to use early_ioremap insted.

Makes sense. On openrisc, the thing is I didn't find one place where
ioremap() is called in arch code. I can cut the early ioremap out and
wrap into a separate early_ioremap() function, however I don't know
where to put it. Not sure if I miss anything or openrisc doesn't really
need early ioremap.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Stafford Horne]

On Sat, Aug 20, 2022 at 08:31:21AM +0800, Baoquan He wrote:
> Add hooks arch_ioremap() and arch_iounmap() for operisc's special
> operation when ioremap() and iounmap.
> 
> 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/Kconfig          |  1 +
>  arch/openrisc/include/asm/io.h | 16 ++++++++---
>  arch/openrisc/mm/ioremap.c     | 51 +++++++++++-----------------------
>  3 files changed, 29 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> index c7f282f60f64..fd9bb76a610b 100644
> --- a/arch/openrisc/Kconfig
> +++ b/arch/openrisc/Kconfig
> @@ -21,6 +21,7 @@ config OPENRISC
>  	select GENERIC_IRQ_PROBE
>  	select GENERIC_IRQ_SHOW
>  	select GENERIC_PCI_IOMAP
> +	select GENERIC_IOREMAP
>  	select GENERIC_CPU_DEVICES
>  	select HAVE_PCI
>  	select HAVE_UID16
> diff --git a/arch/openrisc/include/asm/io.h b/arch/openrisc/include/asm/io.h
> index ee6043a03173..9db67938bfc4 100644
> --- a/arch/openrisc/include/asm/io.h
> +++ b/arch/openrisc/include/asm/io.h
> @@ -15,6 +15,8 @@
>  #define __ASM_OPENRISC_IO_H
>  
>  #include <linux/types.h>
> +#include <asm/pgtable.h>
> +#include <asm/pgalloc.h>

This seems to cause a compilation issue when building virt_defconig:

      CC      kernel/irq/generic-chip.o
    In file included from ./include/asm-generic/pgtable-nopud.h:7,
		     from ./include/asm-generic/pgtable-nopmd.h:7,
		     from ./arch/openrisc/include/asm/pgtable.h:24,
		     from ./arch/openrisc/include/asm/io.h:18,
		     from ./include/linux/io.h:13,
		     from kernel/irq/generic-chip.c:7:
    ./include/asm-generic/pgtable-nop4d.h:9:18: error: unknown type name 'pgd_t'
	9 | typedef struct { pgd_t pgd; } p4d_t;
	  |                  ^~~~~

It works if we swap the order arround:

    +#include <asm/pgalloc.h>
    +#include <asm/pgtable.h>

Otherwise we need to add an asm/page.h include to asm/pgtable.h (which is more
correct) but means you would touch more files.

>  /*
>   * PCI: We do not use IO ports in OpenRISC
> @@ -27,11 +29,17 @@
>  #define PIO_OFFSET		0
>  #define PIO_MASK		0
>  
> -#define ioremap ioremap
> -void __iomem *ioremap(phys_addr_t offset, unsigned long size);
> +/*
> + * I/O memory mapping functions.
> + */
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> +#define arch_ioremap arch_ioremap
> +
> +int arch_iounmap(void __iomem *addr);
> +#define arch_iounmap arch_iounmap
>  
> -#define iounmap iounmap
> -extern void iounmap(volatile void __iomem *addr);
> +#define _PAGE_IOREMAP (pgprot_val(PAGE_KERNEL) | _PAGE_CI)
>  
>  #include <asm-generic/io.h>
>  
> diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
> index 8ec0dafecf25..bc41660e1fb0 100644
> --- a/arch/openrisc/mm/ioremap.c
> +++ b/arch/openrisc/mm/ioremap.c
> @@ -24,26 +24,18 @@ 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
> - * directly.
> - *
> - * NOTE! We need to allow non-page-aligned mappings too: we will obviously
> - * have to convert them into an offset in a page-aligned mapping, but the
> - * caller shouldn't need to know that small detail.
> - */
> -void __iomem *__ref ioremap(phys_addr_t addr, unsigned long size)
> +void __iomem *
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>  {
>  	phys_addr_t p;
>  	unsigned long v;
> -	unsigned long offset, last_addr;
> -	struct vm_struct *area = NULL;
> +	unsigned long offset, last_addr, addr = *paddr;
> +	int ret = -EINVAL;
>  
>  	/* Don't allow wraparound or zero size */
>  	last_addr = addr + size - 1;
>  	if (!size || last_addr < addr)
> -		return NULL;
> +		return IOMEM_ERR_PTR(ret);
>  
>  	/*
>  	 * Mappings have to be page-aligned
> @@ -52,32 +44,24 @@ 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 (unlikely(!mem_init_done)) {
>  		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> -			return NULL;
> +			return IOMEM_ERR_PTR(ret);
>  		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
>  		fixmaps_used += (size >> PAGE_SHIFT);
> -	}
>  
> -	if (ioremap_page_range(v, v + size, p,
> -			__pgprot(pgprot_val(PAGE_KERNEL) | _PAGE_CI))) {
> -		if (likely(mem_init_done))
> -			vfree(area->addr);
> -		else
> +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
>  			fixmaps_used -= (size >> PAGE_SHIFT);
> -		return NULL;
> +			return IOMEM_ERR_PTR(ret);
> +		}
> +
> +		return (void __iomem *)(offset + (char *)v);
>  	}
>  
> -	return (void __iomem *)(offset + (char *)v);
> +	return NULL;
>  }
> -EXPORT_SYMBOL(ioremap);
>  
> -void iounmap(volatile void __iomem *addr)
> +int arch_iounmap(void __iomem *addr)
>  {
>  	/* If the page is from the fixmap pool then we just clear out
>  	 * the fixmap mapping.
> @@ -97,13 +81,10 @@ void iounmap(volatile void __iomem *addr)
>  		 *   ii) invalid accesses to the freed areas aren't made
>  		 */
>  		flush_tlb_all();
> -		return;
> +		return -EINVAL;
>  	}
> -
> -	return vfree((void *)(PAGE_MASK & (unsigned long)addr));
> +	return 0;
>  }
> -EXPORT_SYMBOL(iounmap);
> -
>  /**
>   * OK, this one's a bit tricky... ioremap can get called before memory is
>   * initialized (early serial console does this) and will want to alloc a page
> -- 
> 2.34.1

Other than that compiler issue, I fixed it and test booted this and it works
well.

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Stafford Horne]

On Mon, Aug 29, 2022 at 09:40:24AM +0800, Baoquan He wrote:
> On 08/21/22 at 12:03am, Christoph Hellwig wrote:
> > > +	if (unlikely(!mem_init_done)) {
> > >  		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> > > +			return IOMEM_ERR_PTR(ret);
> > >  		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
> > >  		fixmaps_used += (size >> PAGE_SHIFT);
> > >  
> > > +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
> > >  			fixmaps_used -= (size >> PAGE_SHIFT);
> > > +			return IOMEM_ERR_PTR(ret);
> > > +		}
> > > +
> > > +		return (void __iomem *)(offset + (char *)v);
> > >  	}
> > 
> > This code needs to go away, and all very early boot uses of ioremap
> > need to switch to use early_ioremap insted.
> 
> Makes sense. On openrisc, the thing is I didn't find one place where
> ioremap() is called in arch code. I can cut the early ioremap out and
> wrap into a separate early_ioremap() function, however I don't know
> where to put it. Not sure if I miss anything or openrisc doesn't really
> need early ioremap.

Hi,

I don't know of any early_ioremap usage either in openrisc, maybe some drivers
use it?  However, we do not initialize any early_ioremap infrastructure in
openrisc so that may cause issues if it is used.

We can try to remove it all.  I tested these below additional changes and they
work, if you want to add them we can see if kbuild robots pick anything up.

-Stafford

diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
index bc41660e1fb0..ffe6d5e2b5fe 100644
--- a/arch/openrisc/mm/ioremap.c
+++ b/arch/openrisc/mm/ioremap.c
@@ -22,14 +22,10 @@
 
 extern int mem_init_done;
 
-static unsigned int fixmaps_used __initdata;
-
 void __iomem *
 arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-       phys_addr_t p;
-       unsigned long v;
-       unsigned long offset, last_addr, addr = *paddr;
+       unsigned long last_addr, addr = *paddr;
        int ret = -EINVAL;
 
        /* Don't allow wraparound or zero size */
@@ -37,27 +33,6 @@ arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
        if (!size || last_addr < addr)
                return IOMEM_ERR_PTR(ret);
 
-       /*
-        * Mappings have to be page-aligned
-        */
-       offset = addr & ~PAGE_MASK;
-       p = addr & PAGE_MASK;
-       size = PAGE_ALIGN(last_addr + 1) - p;
-
-       if (unlikely(!mem_init_done)) {
-               if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
-                       return IOMEM_ERR_PTR(ret);
-               v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
-               fixmaps_used += (size >> PAGE_SHIFT);
-
-               if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
-                       fixmaps_used -= (size >> PAGE_SHIFT);
-                       return IOMEM_ERR_PTR(ret);
-               }
-
-               return (void __iomem *)(offset + (char *)v);
-       }
-
        return NULL;
 }
 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/29/22 at 06:42am, Stafford Horne wrote:
> On Mon, Aug 29, 2022 at 09:40:24AM +0800, Baoquan He wrote:
> > On 08/21/22 at 12:03am, Christoph Hellwig wrote:
> > > > +	if (unlikely(!mem_init_done)) {
> > > >  		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> > > > +			return IOMEM_ERR_PTR(ret);
> > > >  		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
> > > >  		fixmaps_used += (size >> PAGE_SHIFT);
> > > >  
> > > > +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
> > > >  			fixmaps_used -= (size >> PAGE_SHIFT);
> > > > +			return IOMEM_ERR_PTR(ret);
> > > > +		}
> > > > +
> > > > +		return (void __iomem *)(offset + (char *)v);
> > > >  	}
> > > 
> > > This code needs to go away, and all very early boot uses of ioremap
> > > need to switch to use early_ioremap insted.
> > 
> > Makes sense. On openrisc, the thing is I didn't find one place where
> > ioremap() is called in arch code. I can cut the early ioremap out and
> > wrap into a separate early_ioremap() function, however I don't know
> > where to put it. Not sure if I miss anything or openrisc doesn't really
> > need early ioremap.
> 
> Hi,
> 
> I don't know of any early_ioremap usage either in openrisc, maybe some drivers
> use it?  However, we do not initialize any early_ioremap infrastructure in
> openrisc so that may cause issues if it is used.

The variable mem_init_done indicates if mem_init() is called or not.
Driver should only initialize after mem_init(). With my understanding,
the early ioremap is only needed by arch code.
> 
> We can try to remove it all.  I tested these below additional changes and they
> work, if you want to add them we can see if kbuild robots pick anything up.

Very helpful information. Then I will remove the !mem_init_done part
code as you listed below, let's see how test robots react. Thanks a lot.

> 
> -Stafford
> 
> diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
> index bc41660e1fb0..ffe6d5e2b5fe 100644
> --- a/arch/openrisc/mm/ioremap.c
> +++ b/arch/openrisc/mm/ioremap.c
> @@ -22,14 +22,10 @@
>  
>  extern int mem_init_done;
>  
> -static unsigned int fixmaps_used __initdata;
> -
>  void __iomem *
>  arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>  {
> -       phys_addr_t p;
> -       unsigned long v;
> -       unsigned long offset, last_addr, addr = *paddr;
> +       unsigned long last_addr, addr = *paddr;
>         int ret = -EINVAL;
>  
>         /* Don't allow wraparound or zero size */
> @@ -37,27 +33,6 @@ arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>         if (!size || last_addr < addr)
>                 return IOMEM_ERR_PTR(ret);
>  
> -       /*
> -        * Mappings have to be page-aligned
> -        */
> -       offset = addr & ~PAGE_MASK;
> -       p = addr & PAGE_MASK;
> -       size = PAGE_ALIGN(last_addr + 1) - p;
> -
> -       if (unlikely(!mem_init_done)) {
> -               if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> -                       return IOMEM_ERR_PTR(ret);
> -               v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
> -               fixmaps_used += (size >> PAGE_SHIFT);
> -
> -               if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
> -                       fixmaps_used -= (size >> PAGE_SHIFT);
> -                       return IOMEM_ERR_PTR(ret);
> -               }
> -
> -               return (void __iomem *)(offset + (char *)v);
> -       }
> -
>         return NULL;
>  }
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/29/22 at 06:32am, Stafford Horne wrote:
> On Sat, Aug 20, 2022 at 08:31:21AM +0800, Baoquan He wrote:
> > Add hooks arch_ioremap() and arch_iounmap() for operisc's special
> > operation when ioremap() and iounmap.
> > 
> > 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/Kconfig          |  1 +
> >  arch/openrisc/include/asm/io.h | 16 ++++++++---
> >  arch/openrisc/mm/ioremap.c     | 51 +++++++++++-----------------------
> >  3 files changed, 29 insertions(+), 39 deletions(-)
> > 
> > diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
> > index c7f282f60f64..fd9bb76a610b 100644
> > --- a/arch/openrisc/Kconfig
> > +++ b/arch/openrisc/Kconfig
> > @@ -21,6 +21,7 @@ config OPENRISC
> >  	select GENERIC_IRQ_PROBE
> >  	select GENERIC_IRQ_SHOW
> >  	select GENERIC_PCI_IOMAP
> > +	select GENERIC_IOREMAP
> >  	select GENERIC_CPU_DEVICES
> >  	select HAVE_PCI
> >  	select HAVE_UID16
> > diff --git a/arch/openrisc/include/asm/io.h b/arch/openrisc/include/asm/io.h
> > index ee6043a03173..9db67938bfc4 100644
> > --- a/arch/openrisc/include/asm/io.h
> > +++ b/arch/openrisc/include/asm/io.h
> > @@ -15,6 +15,8 @@
> >  #define __ASM_OPENRISC_IO_H
> >  
> >  #include <linux/types.h>
> > +#include <asm/pgtable.h>
> > +#include <asm/pgalloc.h>
> 
> This seems to cause a compilation issue when building virt_defconig:
> 
>       CC      kernel/irq/generic-chip.o
>     In file included from ./include/asm-generic/pgtable-nopud.h:7,
> 		     from ./include/asm-generic/pgtable-nopmd.h:7,
> 		     from ./arch/openrisc/include/asm/pgtable.h:24,
> 		     from ./arch/openrisc/include/asm/io.h:18,
> 		     from ./include/linux/io.h:13,
> 		     from kernel/irq/generic-chip.c:7:
>     ./include/asm-generic/pgtable-nop4d.h:9:18: error: unknown type name 'pgd_t'
> 	9 | typedef struct { pgd_t pgd; } p4d_t;
> 	  |                  ^~~~~
> 
> It works if we swap the order arround:
> 
>     +#include <asm/pgalloc.h>
>     +#include <asm/pgtable.h>

Thanks a lot, add this change to patch.

> 
> Otherwise we need to add an asm/page.h include to asm/pgtable.h (which is more
> correct) but means you would touch more files.
> 
> >  /*
> >   * PCI: We do not use IO ports in OpenRISC
> > @@ -27,11 +29,17 @@
> >  #define PIO_OFFSET		0
> >  #define PIO_MASK		0
> >  
> > -#define ioremap ioremap
> > -void __iomem *ioremap(phys_addr_t offset, unsigned long size);
> > +/*
> > + * I/O memory mapping functions.
> > + */
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> > +#define arch_ioremap arch_ioremap
> > +
> > +int arch_iounmap(void __iomem *addr);
> > +#define arch_iounmap arch_iounmap
> >  
> > -#define iounmap iounmap
> > -extern void iounmap(volatile void __iomem *addr);
> > +#define _PAGE_IOREMAP (pgprot_val(PAGE_KERNEL) | _PAGE_CI)
> >  
> >  #include <asm-generic/io.h>
> >  
> > diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
> > index 8ec0dafecf25..bc41660e1fb0 100644
> > --- a/arch/openrisc/mm/ioremap.c
> > +++ b/arch/openrisc/mm/ioremap.c
> > @@ -24,26 +24,18 @@ 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
> > - * directly.
> > - *
> > - * NOTE! We need to allow non-page-aligned mappings too: we will obviously
> > - * have to convert them into an offset in a page-aligned mapping, but the
> > - * caller shouldn't need to know that small detail.
> > - */
> > -void __iomem *__ref ioremap(phys_addr_t addr, unsigned long size)
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> >  {
> >  	phys_addr_t p;
> >  	unsigned long v;
> > -	unsigned long offset, last_addr;
> > -	struct vm_struct *area = NULL;
> > +	unsigned long offset, last_addr, addr = *paddr;
> > +	int ret = -EINVAL;
> >  
> >  	/* Don't allow wraparound or zero size */
> >  	last_addr = addr + size - 1;
> >  	if (!size || last_addr < addr)
> > -		return NULL;
> > +		return IOMEM_ERR_PTR(ret);
> >  
> >  	/*
> >  	 * Mappings have to be page-aligned
> > @@ -52,32 +44,24 @@ 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 (unlikely(!mem_init_done)) {
> >  		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
> > -			return NULL;
> > +			return IOMEM_ERR_PTR(ret);
> >  		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
> >  		fixmaps_used += (size >> PAGE_SHIFT);
> > -	}
> >  
> > -	if (ioremap_page_range(v, v + size, p,
> > -			__pgprot(pgprot_val(PAGE_KERNEL) | _PAGE_CI))) {
> > -		if (likely(mem_init_done))
> > -			vfree(area->addr);
> > -		else
> > +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
> >  			fixmaps_used -= (size >> PAGE_SHIFT);
> > -		return NULL;
> > +			return IOMEM_ERR_PTR(ret);
> > +		}
> > +
> > +		return (void __iomem *)(offset + (char *)v);
> >  	}
> >  
> > -	return (void __iomem *)(offset + (char *)v);
> > +	return NULL;
> >  }
> > -EXPORT_SYMBOL(ioremap);
> >  
> > -void iounmap(volatile void __iomem *addr)
> > +int arch_iounmap(void __iomem *addr)
> >  {
> >  	/* If the page is from the fixmap pool then we just clear out
> >  	 * the fixmap mapping.
> > @@ -97,13 +81,10 @@ void iounmap(volatile void __iomem *addr)
> >  		 *   ii) invalid accesses to the freed areas aren't made
> >  		 */
> >  		flush_tlb_all();
> > -		return;
> > +		return -EINVAL;
> >  	}
> > -
> > -	return vfree((void *)(PAGE_MASK & (unsigned long)addr));
> > +	return 0;
> >  }
> > -EXPORT_SYMBOL(iounmap);
> > -
> >  /**
> >   * OK, this one's a bit tricky... ioremap can get called before memory is
> >   * initialized (early serial console does this) and will want to alloc a page
> > -- 
> > 2.34.1
> 
> Other than that compiler issue, I fixed it and test booted this and it works
> well.
> 
> Acked-by: Stafford Horne <shorne@gmail.com>

Thanks, will add this tag when repost.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/11] openrisc: mm: Convert to GENERIC_IOREMAP

[Christophe Leroy]

Hi Christoph,

Le 21/08/2022 à 09:03, Christoph Hellwig a écrit :
>> +	if (unlikely(!mem_init_done)) {
>>   		if ((fixmaps_used + (size >> PAGE_SHIFT)) > FIX_N_IOREMAPS)
>> +			return IOMEM_ERR_PTR(ret);
>>   		v = fix_to_virt(FIX_IOREMAP_BEGIN + fixmaps_used);
>>   		fixmaps_used += (size >> PAGE_SHIFT);
>>   
>> +		if (ioremap_page_range(v, v + size, p, __pgprot(*prot_val))) {
>>   			fixmaps_used -= (size >> PAGE_SHIFT);
>> +			return IOMEM_ERR_PTR(ret);
>> +		}
>> +
>> +		return (void __iomem *)(offset + (char *)v);
>>   	}
> 
> This code needs to go away, and all very early boot uses of ioremap
> need to switch to use early_ioremap insted.
> 

early_ioremap() is not that easy to use. I don't know in this particular 
openrisc context, but in powerpc we have many places that use ioremap() 
very early.

Three years ago we implemented EARLY_IOREMAP, commit d538aadc2718 
("powerpc/ioremap: warn on early use of ioremap()") added a warning to 
identify all places using ioremap() early with the target being to get 
rid of early usage of ioremap. But it turned that using early_ioremap() 
is not that easy.

The problem with early_ioremap() is that it installs ephemeral mappings 
that must be freed before the end of init. Most early uses of ioremap() 
in powerpc are to install mappings that will last for life.

If we could have something that works like memblock_alloc() and allows 
to keep the allocation forever, then it would be a lot easier to use 
early_ioremap() and we could get rid of the early handling of ioremap() 
in powerpc.

Christophe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 08/11] parisc: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Hi,

On 08/20/22 at 12:03pm, kernel test robot wrote:
> Hi Baoquan,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on akpm-mm/mm-everything]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> config: parisc-randconfig-r005-20220820 (https://download.01.org/0day-ci/archive/20220820/202208201135.YyN9CXsu-lkp@intel.com/config)
> compiler: hppa-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/570f2a3347cc83c9ea71d3dbbebfad8ea085ecc6
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
>         git checkout 570f2a3347cc83c9ea71d3dbbebfad8ea085ecc6
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc prepare
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>

I finally installed below RPM packages and run command to reproduce
the build errors. Thanks for your help, Helge.

binutils-hppa64-linux-gnu
gcc-hppa64-linux-gnu
binutils-hppa-linux-gnu
gcc-hppa-linux-gnu
make ARCH=parisc defconfig
make ARCH=parisc -j320 CROSS_COMPILE=/usr/bin/hppa-linux-gnu-
make ARCH=parisc64 defconfig
make ARCH=parisc64 -j320 CROSS_COMPILE=/usr/bin/hppa64-linux-gnu-

The below draft patch can fix all build errors. There are some symbol
declaration or defination conflict among asm-generic/io.h,
asm-generic/iomap.h, asm-generic/pci-iomap.h and asm/io.h. Some of them
may need be taken out to be an independent patch since it could be a
generic issue if enable GENERIC_IOREMAP. I saw the build error on superH
too.

diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
index 614e21d9749f..879578edf342 100644
--- a/arch/parisc/include/asm/io.h
+++ b/arch/parisc/include/asm/io.h
@@ -126,7 +126,7 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr)
  * I/O memory mapping functions.
  */
 void __iomem *
-arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long prot);
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
 #define arch_ioremap arch_ioremap
 
 #define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | \
@@ -137,6 +137,8 @@ arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long prot);
 #define ioremap_uc(addr, size)  \
 	ioremap_prot((addr), (size), _PAGE_IOREMAP)
 
+#define ARCH_HAS_IOREMAP_WC
+
 static inline unsigned char __raw_readb(const volatile void __iomem *addr)
 {
 	return (*(volatile unsigned char __force *) (addr));
@@ -205,6 +207,15 @@ static inline void writeq(unsigned long long q, volatile void __iomem *addr)
 	__raw_writeq((__u64 __force) cpu_to_le64(q), addr);
 }
 
+#define __raw_readb __raw_readb
+#define __raw_readw __raw_readw
+#define __raw_readl __raw_readl
+#define __raw_readq __raw_readq
+#define __raw_writeb __raw_writeb
+#define __raw_writew __raw_writew
+#define __raw_writel __raw_writel
+#define __raw_writeq __raw_writeq
+
 #define	readb	readb
 #define	readw	readw
 #define	readl	readl
@@ -223,6 +234,9 @@ static inline void writeq(unsigned long long q, volatile void __iomem *addr)
 #define writel_relaxed(l, addr)	writel(l, addr)
 #define writeq_relaxed(q, addr)	writeq(q, addr)
 
+#define memset_io memset_io
+#define memcpy_fromio memcpy_fromio
+#define memcpy_toio memcpy_toio
 void memset_io(volatile void __iomem *addr, unsigned char val, int count);
 void memcpy_fromio(void *dst, const volatile void __iomem *src, int count);
 void memcpy_toio(volatile void __iomem *dst, const void *src, int count);
@@ -244,10 +258,17 @@ extern void eisa_out16(unsigned short data, unsigned short port);
 extern void eisa_out32(unsigned int data, unsigned short port);
 
 #if defined(CONFIG_PCI)
+#define inb inb
+#define inw inw
+#define inl inl
 extern unsigned char inb(int addr);
 extern unsigned short inw(int addr);
 extern unsigned int inl(int addr);
 
+#define outb outb
+#define outw outw
+#define outl outl
+#define inl inl
 extern void outb(unsigned char b, int addr);
 extern void outw(unsigned short b, int addr);
 extern void outl(unsigned int b, int addr);
@@ -292,6 +313,13 @@ extern void outsb (unsigned long port, const void *src, unsigned long count);
 extern void outsw (unsigned long port, const void *src, unsigned long count);
 extern void outsl (unsigned long port, const void *src, unsigned long count);
 
+#define insb insb
+#define insw insw
+#define insl insl
+#define outsb outsb
+#define outsw outsw
+#define outsl outsl
+
 
 /* IO Port space is :      BBiiii   where BB is HBA number. */
 #define IO_SPACE_LIMIT 0x00ffffff
@@ -320,6 +348,7 @@ extern void iowrite64be(u64 val, void __iomem *addr);
  */
 #define xlate_dev_mem_ptr(p)	__va(p)
 
+#include <asm-generic/io.h>
 extern int devmem_is_allowed(unsigned long pfn);
 
 #endif
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 08237ae8b840..7bf3581ecb0e 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -26,12 +26,19 @@
  * in the low address range. Architectures for which this is not
  * true can't use this generic implementation.
  */
+#define ioread8 ioread8
+#define ioread16 ioread16
+#define ioread16be ioread16be
+#define ioread32 ioread32
+#define ioread32be ioread32be
 extern unsigned int ioread8(const void __iomem *);
 extern unsigned int ioread16(const void __iomem *);
 extern unsigned int ioread16be(const void __iomem *);
 extern unsigned int ioread32(const void __iomem *);
 extern unsigned int ioread32be(const void __iomem *);
 #ifdef CONFIG_64BIT
+#define ioread64 ioread64
+#define ioread64be ioread64be
 extern u64 ioread64(const void __iomem *);
 extern u64 ioread64be(const void __iomem *);
 #endif
@@ -47,12 +54,19 @@ extern u64 ioread64be_lo_hi(const void __iomem *addr);
 extern u64 ioread64be_hi_lo(const void __iomem *addr);
 #endif
 
+#define iowrite8 iowrite8
+#define iowrite16 iowrite16
+#define iowrite16be iowrite16be
+#define iowrite32 iowrite32
+#define iowrite32be iowrite32be
 extern void iowrite8(u8, void __iomem *);
 extern void iowrite16(u16, void __iomem *);
 extern void iowrite16be(u16, void __iomem *);
 extern void iowrite32(u32, void __iomem *);
 extern void iowrite32be(u32, void __iomem *);
 #ifdef CONFIG_64BIT
+#define iowrite64 iowrite64
+#define iowrite64be iowrite64be
 extern void iowrite64(u64, void __iomem *);
 extern void iowrite64be(u64, void __iomem *);
 #endif
@@ -79,16 +93,24 @@ extern void iowrite64be_hi_lo(u64 val, void __iomem *addr);
  * memory across multiple ports, use "memcpy_toio()"
  * and friends.
  */
+#define ioread8_rep ioread8_rep
+#define ioread16_rep ioread16_rep
+#define ioread32_rep ioread32_rep
 extern void ioread8_rep(const void __iomem *port, void *buf, unsigned long count);
 extern void ioread16_rep(const void __iomem *port, void *buf, unsigned long count);
 extern void ioread32_rep(const void __iomem *port, void *buf, unsigned long count);
 
+#define iowrite8_rep iowrite8_rep
+#define iowrite16_rep iowrite16_rep
+#define iowrite32_rep iowrite32_rep
 extern void iowrite8_rep(void __iomem *port, const void *buf, unsigned long count);
 extern void iowrite16_rep(void __iomem *port, const void *buf, unsigned long count);
 extern void iowrite32_rep(void __iomem *port, const void *buf, unsigned long count);
 
 #ifdef CONFIG_HAS_IOPORT_MAP
 /* Create a virtual mapping cookie for an IO port range */
+#define ioport_map ioport_map
+#define ioport_unmap ioport_unmap
 extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
 extern void ioport_unmap(void __iomem *);
 #endif
diff --git a/include/asm-generic/pci_iomap.h b/include/asm-generic/pci_iomap.h
index 8fbb0a55545d..f6889ea449fa 100644
--- a/include/asm-generic/pci_iomap.h
+++ b/include/asm-generic/pci_iomap.h
@@ -19,6 +19,7 @@ extern void __iomem *pci_iomap_wc_range(struct pci_dev *dev, int bar,
 					unsigned long offset,
 					unsigned long maxlen);
 extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
+#define pci_iounmap pci_iounmap
 /* Create a virtual mapping cookie for a port on a given PCI device.
  * Do not call this directly, it exists to make it easier for architectures
  * to override */


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Hi Niklas,

On 08/23/22 at 02:30pm, Niklas Schnelle wrote:
> On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> > Add hooks arch_ioremap() and arch_iounmap() for s390's special
> > operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> > converted to use ioremap_prot() from GENERIC_IOREMAP.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> > ---
> >  arch/s390/Kconfig          |  1 +
> >  arch/s390/include/asm/io.h | 26 +++++++++++------
> >  arch/s390/pci/pci.c        | 60 +++++---------------------------------
> >  3 files changed, 26 insertions(+), 61 deletions(-)
> > 
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 318fce77601d..c59e1b25f59d 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -135,6 +135,7 @@ config S390
> >  	select GENERIC_SMP_IDLE_THREAD
> >  	select GENERIC_TIME_VSYSCALL
> >  	select GENERIC_VDSO_TIME_NS
> > +	select GENERIC_IOREMAP
> >  	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
> >  	select HAVE_ARCH_AUDITSYSCALL
> >  	select HAVE_ARCH_JUMP_LABEL
> > diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
> > index e3882b012bfa..f837e20b7bbd 100644
> > --- a/arch/s390/include/asm/io.h
> > +++ b/arch/s390/include/asm/io.h
> > @@ -22,11 +22,23 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
> >  
> >  #define IO_SPACE_LIMIT 0
> >  
> > -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
> > -void __iomem *ioremap(phys_addr_t addr, size_t size);
> > -void __iomem *ioremap_wc(phys_addr_t addr, size_t size);
> > -void __iomem *ioremap_wt(phys_addr_t addr, size_t size);
> > -void iounmap(volatile void __iomem *addr);
> > +
> 

Thanks a lot for reviewing, and sorry for late response.

> Checkpatch nitpick, remove the empty line addition above so as not to
> create two consecutive empty lines.

Will do, thanks.

> 
> > +/*
> > + * I/O memory mapping functions.
> > + */
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> > +#define arch_ioremap arch_ioremap
> > +
> > +int arch_iounmap(void __iomem *addr);
> > +#define arch_iounmap arch_iounmap
> > +
> > +#define _PAGE_IOREMAP pgprot_val(PAGE_KERNEL)
> > +
> > +#define ioremap_wc(addr, size)  \
> > +	ioremap_prot((addr), (size), pgprot_val(pgprot_writecombine(PAGE_KERNEL)))
> > +#define ioremap_wt(addr, size)  \
> > +	ioremap_prot((addr), (size), pgprot_val(pgprot_writethrough(PAGE_KERNEL)))
> >  
> >  static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
> >  {
> > @@ -51,10 +63,6 @@ static inline void ioport_unmap(void __iomem *p)
> >  #define pci_iomap_wc pci_iomap_wc
> >  #define pci_iomap_wc_range pci_iomap_wc_range
> >  
> > -#define ioremap ioremap
> > -#define ioremap_wt ioremap_wt
> > -#define ioremap_wc ioremap_wc
> > -
> >  #define memcpy_fromio(dst, src, count)	zpci_memcpy_fromio(dst, src, count)
> >  #define memcpy_toio(dst, src, count)	zpci_memcpy_toio(dst, src, count)
> >  #define memset_io(dst, val, count)	zpci_memset_io(dst, val, count)
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index 73cdc5539384..984cad9cd5a1 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -244,64 +244,20 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
> >         zpci_memcpy_toio(to, from, count);
> >  }
> >  
> > -static void __iomem *__ioremap(phys_addr_t addr, size_t size, pgprot_t prot)
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> >  {
> > -	unsigned long offset, vaddr;
> > -	struct vm_struct *area;
> > -	phys_addr_t last_addr;
> > -
> > -	last_addr = addr + size - 1;
> > -	if (!size || last_addr < addr)
> > -		return NULL;
> > -
> >  	if (!static_branch_unlikely(&have_mio))
> > -		return (void __iomem *) addr;
> > -
> > -	offset = addr & ~PAGE_MASK;
> > -	addr &= PAGE_MASK;
> > -	size = PAGE_ALIGN(size + offset);
> > -	area = get_vm_area(size, VM_IOREMAP);
> > -	if (!area)
> > -		return NULL;
> > -
> > -	vaddr = (unsigned long) area->addr;
> > -	if (ioremap_page_range(vaddr, vaddr + size, addr, prot)) {
> > -		free_vm_area(area);
> > -		return NULL;
> > -	}
> > -	return (void __iomem *) ((unsigned long) area->addr + offset);
> > -}
> > -
> > -void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
> > -{
> > -	return __ioremap(addr, size, __pgprot(prot));
> > +		return (void __iomem *) *paddr;
> 
> Another checkpatch nitpick no space after the cast.

Will fix.

> 
> > +	return NULL;
> >  }
> > -EXPORT_SYMBOL(ioremap_prot);
> >  
> > -void __iomem *ioremap(phys_addr_t addr, size_t size)
> > +int arch_iounmap(void __iomem *addr)
> >  {
> > -	return __ioremap(addr, size, PAGE_KERNEL);
> > -}
> > -EXPORT_SYMBOL(ioremap);
> > -
> > -void __iomem *ioremap_wc(phys_addr_t addr, size_t size)
> > -{
> > -	return __ioremap(addr, size, pgprot_writecombine(PAGE_KERNEL));
> > -}
> > -EXPORT_SYMBOL(ioremap_wc);
> > -
> > -void __iomem *ioremap_wt(phys_addr_t addr, size_t size)
> > -{
> > -	return __ioremap(addr, size, pgprot_writethrough(PAGE_KERNEL));
> > -}
> > -EXPORT_SYMBOL(ioremap_wt);
> > -
> > -void iounmap(volatile void __iomem *addr)
> > -{
> > -	if (static_branch_likely(&have_mio))
> > -		vunmap((__force void *) ((unsigned long) addr & PAGE_MASK));
> > +	if (!static_branch_likely(&have_mio))
> > +		return -EINVAL;
> 
> As Christoph suggested this might be a good opportunity to add a
> comment for this branch.
> 
> One other nitpick. The return value doesn't really matter here since
> anything != NULL turns iounmap() into a no-op so this looks correct but
> semantically I think returning -EINVAL wrongly suggests that addr was
> invalid. Maybe -ENXIO would be better at conveying that there is
> nothing to unmap.
> 
> Looking at your patch 1 another idea would be to have 3 kinds of return
> values for arch_iounmap() too e.g.:
> 
>  arch_iounmap() return an __iomem pointer
>    - IS_ERR means skip vunmap and return directly
>    - NULL means continue to vunmap
>    - a non-NULL, non-IS_ERR pointer has been unmapped successfully

I agree with you, The returned -EINVAL does cause confusion.

I checked all ARCHes related in this patchset, we only do the arch
specific address filtering (or address checking), then call vunmap().
Seems no error case or error handling in iounmap(). I'm thinking if
we should make the return value as boolean value. Ture indicates
address checking passed, false indicates not passed.


> 
> Then we would simply return addr in case of
> !static_branch_likely(&have_mio) and NULL otherwise.
> 
> What do you think? Either way no strong opinion on my side,
> functionally it makes no difference.
> 
> > +	return 0;
> >  }
> > -EXPORT_SYMBOL(iounmap);
> >  
> >  /* Create a virtual mapping cookie for a PCI BAR */
> >  static void __iomem *pci_iomap_range_fh(struct pci_dev *pdev, int bar,
> 
> Apart from the above nitpicks and suggestion this looks good to me.
> I did also test this with and without PCI MIO support including use of
> PCI MIO instructions in user-space (added in rdma-core v40).
> 
> So feel free to add:
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> Since it looks like there will be a v3 due to other comments anyway
> please Cc me on that directly and I'm sure I can upgrade the Acked-by
> to Reviewed-by when we're closer to the final code.

Sure, I will add you in Cc when repost. Thank again for these details.


> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/22/22 at 05:19pm, Niklas Schnelle wrote:
> On Sat, 2022-08-20 at 08:31 +0800, Baoquan He wrote:
> > Add hooks arch_ioremap() and arch_iounmap() for s390's special
> > operation when ioremap() and iounmap(), then ioremap_[wc|wt]() are
> > converted to use ioremap_prot() from GENERIC_IOREMAP.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Cc: Heiko Carstens <hca@linux.ibm.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Alexander Gordeev <agordeev@linux.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: linux-s390@vger.kernel.org
> > ---
> >  arch/s390/Kconfig          |  1 +
> >  arch/s390/include/asm/io.h | 26 +++++++++++------
> >  arch/s390/pci/pci.c        | 60 +++++---------------------------------
> >  3 files changed, 26 insertions(+), 61 deletions(-)
> 
> Sorry I missed this mail until now and will still need a bit of time to
> review and test the code as this is indeed pretty special on s390. From
> a first glance this does look like a nice simplification.
> 
> Just out of curiosity, I wonder why get_maintainers.pl didn't add me
> nor Gerald for direct CC despite the bulk of the changes affecting
> arch/s390/pci/*.

I run below script again, it does print you and Gerald's name in the
list. It must be me who made mistake when copying maintainers' name into
Cc. Sorry about that.
./scripts/get_maintainer.pl 0009-s390-mm-Convert-to-GENERIC_IOREMAP.patch

> 
> > 
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> ---8<---
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/11] s390: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/22/22 at 05:08pm, Niklas Schnelle wrote:
> On Sun, 2022-08-21 at 00:05 -0700, Christoph Hellwig wrote:
> > > +void __iomem *
> > > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
> > >  {
> > >  	if (!static_branch_unlikely(&have_mio))
> > > +		return (void __iomem *) *paddr;
> > > +	return NULL;
> > 
> > This logic isn't new in the patch, but it could really use a comment
> > as it is rather non-obvious.
> 
> Yes, makes sense. Basically we fake MMIO addresses because the s390
> architecture doesn't have MMIO as a concept. That is until the PCI MIO
> instructions introduced pseudo-MMIO though only for specific PCI
> load/store instructions. Without those PCI BAR spaces as well as config
> space is accessed with so called function handles. As these are a bad
> fit for Linux' MMIO based APIs we create fake MMIO addresses (called
> address cookies) that encode an index into the zpci_iomap_start[] which
> can be decoded by our implementation of ioread*/iowrite*().
> 
> I don't think this is the right place to describe this overall scheme
> in detail but maybe we can leave a a good bread crumb. Maybe something
> like below?
> 
> /* 
>  * When PCI MIO instructions are unavailable the "physical" address encodes
>  * a hint for accessing the PCI memory space it represents. Just pass it 
>  * unchanged such that ioread/iowrite can decode it.
>  */

Thanks. Looks good to me, I will add these to above the code.

> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/21/22 at 12:06am, Christoph Hellwig wrote:
> > +void __iomem *
> > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val);
> > +#define arch_ioremap arch_ioremap
> 
> Shouldn't this still be under CONFIG_MMU?

Yeah, you are right, will put them under CONFIG_MMU.

I thought making GENERIC_IOREMAP depend MMU in Kconfig will contain
that. But people can manually set the Kconfig item freely. Thanks for
pointing it out.

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 5f220e903e5a..b63ad4698cf8 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -25,6 +25,7 @@ config SUPERH
        select GENERIC_SCHED_CLOCK
        select GENERIC_SMP_IDLE_THREAD
        select GUP_GET_PTE_LOW_HIGH if X2TLB
+       select GENERIC_IOREMAP if MMU

> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

Hi,

On 08/20/22 at 11:41am, kernel test robot wrote:
> Hi Baoquan,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on akpm-mm/mm-everything]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> config: sh-allmodconfig
> compiler: sh4-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/503a31451202f89e58bc5f0a49261398fafbd90e
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
>         git checkout 503a31451202f89e58bc5f0a49261398fafbd90e
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare

I can only find binutils-sh-linux-gnu on fedora, but no gcc-sh-linux-gnu
for cross compiling superh kernel. So I tried above steps, but it failed
as below, any suggestion I can fix it to proceed for reproducing the
reporting issues? Thanks in advance.

[root@ampere-mtsnow-altra-09 ~]# ls
0day  anaconda-ks.cfg  bin  config  EFI_BOOT_ENTRY.TXT  linux  NETBOOT_METHOD.TXT  original-ks.cfg  RECIPE.TXT
[root@ampere-mtsnow-altra-09 ~]# ls 0day/gcc-12.1.0-nolibc/sh4-linux/bin/
sh4-linux-addr2line  sh4-linux-elfedit     sh4-linux-gcc-ranlib  sh4-linux-ld        sh4-linux-objdump  sh4-linux-strip
sh4-linux-ar         sh4-linux-gcc         sh4-linux-gcov        sh4-linux-ld.bfd    sh4-linux-ranlib
sh4-linux-as         sh4-linux-gcc-12.1.0  sh4-linux-gcov-dump   sh4-linux-lto-dump  sh4-linux-readelf
sh4-linux-c++filt    sh4-linux-gcc-ar      sh4-linux-gcov-tool   sh4-linux-nm        sh4-linux-size
sh4-linux-cpp        sh4-linux-gcc-nm      sh4-linux-gprof       sh4-linux-objcopy   sh4-linux-strings
[root@ampere-mtsnow-altra-09 ~]# ls bin
make.cross
[root@ampere-mtsnow-altra-09 ~]# cd linux/
[root@ampere-mtsnow-altra-09 linux]# ls
arch       certs    CREDITS        drivers  init      Kbuild   lib          Makefile  README   security  usr
block      config   crypto         fs       io_uring  Kconfig  LICENSES     mm        samples  sound     virt
build_dir  COPYING  Documentation  include  ipc       kernel   MAINTAINERS  net       scripts  tools
[root@ampere-mtsnow-altra-09 linux]# ls build_dir/  -a
.  ..  arch  .config  .gitignore  include  Makefile  scripts  source  usr

[root@ampere-mtsnow-altra-09 linux]# COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare
Compiler will be installed in /root/0day
	not a dynamic executable
make --keep-going CONFIG_OF_ALL_DTBS=y CONFIG_DTC=y CROSS_COMPILE=/root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux- --jobs=160 W=1 O=build_dir ARCH=sh prepare
make[1]: Entering directory '/root/linux/build_dir'
/bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
/bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
/bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
  SYNC    include/config/auto.conf.cmd
/bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
/bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
/bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
  GEN     Makefile
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/kconfig/conf.o
  HOSTCC  scripts/kconfig/confdata.o
  HOSTCC  scripts/kconfig/expr.o
  LEX     scripts/kconfig/lexer.lex.c
  YACC    scripts/kconfig/parser.tab.[ch]
  HOSTCC  scripts/kconfig/menu.o
  HOSTCC  scripts/kconfig/preprocess.o
  HOSTCC  scripts/kconfig/symbol.o
  HOSTCC  scripts/kconfig/util.o
  HOSTCC  scripts/kconfig/lexer.lex.o
  HOSTCC  scripts/kconfig/parser.tab.o
  HOSTLD  scripts/kconfig/conf
/root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: unknown compiler
scripts/Kconfig.include:44: Sorry, this compiler is not supported.
make[3]: *** [../scripts/kconfig/Makefile:77: syncconfig] Error 1
make[2]: *** [../Makefile:632: syncconfig] Error 2
make[1]: *** [/root/linux/Makefile:734: include/config/auto.conf.cmd] Error 2
make[1]: Failed to remake makefile 'include/config/auto.conf.cmd'.
make[1]: Failed to remake makefile 'include/config/auto.conf'.
  GEN     Makefile
  SYSHDR  arch/sh/include/generated/uapi/asm/unistd_32.h
  SYSTBL  arch/sh/include/generated/asm/syscall_table.h
Error: kernelrelease not valid - run 'make prepare' to update it
  HOSTCC  scripts/dtc/dtc.o
  HOSTCC  scripts/dtc/flattree.o
  HOSTCC  scripts/dtc/fstree.o
  HOSTCC  scripts/dtc/data.o
  HOSTCC  scripts/dtc/livetree.o
  HOSTCC  scripts/dtc/treesource.o
  HOSTCC  scripts/dtc/srcpos.o
  HOSTCC  scripts/dtc/checks.o
  HOSTCC  scripts/dtc/util.o
  LEX     scripts/dtc/dtc-lexer.lex.c
  YACC    scripts/dtc/dtc-parser.tab.[ch]
  HOSTCC  scripts/dtc/libfdt/fdt_ro.o
  HOSTCC  scripts/dtc/libfdt/fdt.o
  HOSTCC  scripts/dtc/libfdt/fdt_wip.o
  HOSTCC  scripts/dtc/libfdt/fdt_sw.o
  HOSTCC  scripts/dtc/libfdt/fdt_rw.o
  HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
  HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
  HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
  HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
  HOSTCC  scripts/dtc/fdtoverlay.o
  HOSTCC  scripts/dtc/dtc-lexer.lex.o
  HOSTCC  scripts/dtc/dtc-parser.tab.o
  HOSTLD  scripts/dtc/fdtoverlay
  HOSTLD  scripts/dtc/dtc
make[1]: Target 'prepare' not remade because of errors.
make[1]: Leaving directory '/root/linux/build_dir'
make: *** [Makefile:222: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kbuild-all] Re: [PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Chen, Rong A]

On 9/1/2022 6:39 PM, Baoquan He wrote:
> Hi,
> 
> On 08/20/22 at 11:41am, kernel test robot wrote:
>> Hi Baoquan,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on akpm-mm/mm-everything]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
>> config: sh-allmodconfig
>> compiler: sh4-linux-gcc (GCC) 12.1.0
>> reproduce (this is a W=1 build):
>>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # https://github.com/intel-lab-lkp/linux/commit/503a31451202f89e58bc5f0a49261398fafbd90e
>>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>>          git fetch --no-tags linux-review Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
>>          git checkout 503a31451202f89e58bc5f0a49261398fafbd90e
>>          # save the config file
>>          mkdir build_dir && cp config build_dir/.config
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare
> 
> I can only find binutils-sh-linux-gnu on fedora, but no gcc-sh-linux-gnu
> for cross compiling superh kernel. So I tried above steps, but it failed
> as below, any suggestion I can fix it to proceed for reproducing the
> reporting issues? Thanks in advance.

Hi Baoquan,

Sorry for the inconvenience, /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin
/sh4-linux-gcc is for x86_64 machine, as the name "ampere-mtsnow-altra",
is it a arm server?

the tool for arm64 can be downloaded from 
https://cdn.kernel.org/pub/tools/crosstool/files/bin/arm64/12.1.0/

Best Regards,
Rong Chen

> 
> [root@ampere-mtsnow-altra-09 ~]# ls
> 0day  anaconda-ks.cfg  bin  config  EFI_BOOT_ENTRY.TXT  linux  NETBOOT_METHOD.TXT  original-ks.cfg  RECIPE.TXT
> [root@ampere-mtsnow-altra-09 ~]# ls 0day/gcc-12.1.0-nolibc/sh4-linux/bin/
> sh4-linux-addr2line  sh4-linux-elfedit     sh4-linux-gcc-ranlib  sh4-linux-ld        sh4-linux-objdump  sh4-linux-strip
> sh4-linux-ar         sh4-linux-gcc         sh4-linux-gcov        sh4-linux-ld.bfd    sh4-linux-ranlib
> sh4-linux-as         sh4-linux-gcc-12.1.0  sh4-linux-gcov-dump   sh4-linux-lto-dump  sh4-linux-readelf
> sh4-linux-c++filt    sh4-linux-gcc-ar      sh4-linux-gcov-tool   sh4-linux-nm        sh4-linux-size
> sh4-linux-cpp        sh4-linux-gcc-nm      sh4-linux-gprof       sh4-linux-objcopy   sh4-linux-strings
> [root@ampere-mtsnow-altra-09 ~]# ls bin
> make.cross
> [root@ampere-mtsnow-altra-09 ~]# cd linux/
> [root@ampere-mtsnow-altra-09 linux]# ls
> arch       certs    CREDITS        drivers  init      Kbuild   lib          Makefile  README   security  usr
> block      config   crypto         fs       io_uring  Kconfig  LICENSES     mm        samples  sound     virt
> build_dir  COPYING  Documentation  include  ipc       kernel   MAINTAINERS  net       scripts  tools
> [root@ampere-mtsnow-altra-09 linux]# ls build_dir/  -a
> .  ..  arch  .config  .gitignore  include  Makefile  scripts  source  usr
> 
> [root@ampere-mtsnow-altra-09 linux]# COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare
> Compiler will be installed in /root/0day
> 	not a dynamic executable
> make --keep-going CONFIG_OF_ALL_DTBS=y CONFIG_DTC=y CROSS_COMPILE=/root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux- --jobs=160 W=1 O=build_dir ARCH=sh prepare
> make[1]: Entering directory '/root/linux/build_dir'
> /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
>    SYNC    include/config/auto.conf.cmd
> /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
>    GEN     Makefile
>    HOSTCC  scripts/basic/fixdep
>    HOSTCC  scripts/kconfig/conf.o
>    HOSTCC  scripts/kconfig/confdata.o
>    HOSTCC  scripts/kconfig/expr.o
>    LEX     scripts/kconfig/lexer.lex.c
>    YACC    scripts/kconfig/parser.tab.[ch]
>    HOSTCC  scripts/kconfig/menu.o
>    HOSTCC  scripts/kconfig/preprocess.o
>    HOSTCC  scripts/kconfig/symbol.o
>    HOSTCC  scripts/kconfig/util.o
>    HOSTCC  scripts/kconfig/lexer.lex.o
>    HOSTCC  scripts/kconfig/parser.tab.o
>    HOSTLD  scripts/kconfig/conf
> /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: unknown compiler
> scripts/Kconfig.include:44: Sorry, this compiler is not supported.
> make[3]: *** [../scripts/kconfig/Makefile:77: syncconfig] Error 1
> make[2]: *** [../Makefile:632: syncconfig] Error 2
> make[1]: *** [/root/linux/Makefile:734: include/config/auto.conf.cmd] Error 2
> make[1]: Failed to remake makefile 'include/config/auto.conf.cmd'.
> make[1]: Failed to remake makefile 'include/config/auto.conf'.
>    GEN     Makefile
>    SYSHDR  arch/sh/include/generated/uapi/asm/unistd_32.h
>    SYSTBL  arch/sh/include/generated/asm/syscall_table.h
> Error: kernelrelease not valid - run 'make prepare' to update it
>    HOSTCC  scripts/dtc/dtc.o
>    HOSTCC  scripts/dtc/flattree.o
>    HOSTCC  scripts/dtc/fstree.o
>    HOSTCC  scripts/dtc/data.o
>    HOSTCC  scripts/dtc/livetree.o
>    HOSTCC  scripts/dtc/treesource.o
>    HOSTCC  scripts/dtc/srcpos.o
>    HOSTCC  scripts/dtc/checks.o
>    HOSTCC  scripts/dtc/util.o
>    LEX     scripts/dtc/dtc-lexer.lex.c
>    YACC    scripts/dtc/dtc-parser.tab.[ch]
>    HOSTCC  scripts/dtc/libfdt/fdt_ro.o
>    HOSTCC  scripts/dtc/libfdt/fdt.o
>    HOSTCC  scripts/dtc/libfdt/fdt_wip.o
>    HOSTCC  scripts/dtc/libfdt/fdt_sw.o
>    HOSTCC  scripts/dtc/libfdt/fdt_rw.o
>    HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
>    HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
>    HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
>    HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
>    HOSTCC  scripts/dtc/fdtoverlay.o
>    HOSTCC  scripts/dtc/dtc-lexer.lex.o
>    HOSTCC  scripts/dtc/dtc-parser.tab.o
>    HOSTLD  scripts/dtc/fdtoverlay
>    HOSTLD  scripts/dtc/dtc
> make[1]: Target 'prepare' not remade because of errors.
> make[1]: Leaving directory '/root/linux/build_dir'
> make: *** [Makefile:222: __sub-make] Error 2
> make: Target 'prepare' not remade because of errors.
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [kbuild-all] Re: [PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 09/01/22 at 08:11pm, Chen, Rong A wrote:
> 
> 
> On 9/1/2022 6:39 PM, Baoquan He wrote:
> > Hi,
> > 
> > On 08/20/22 at 11:41am, kernel test robot wrote:
> > > Hi Baoquan,
> > > 
> > > I love your patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on akpm-mm/mm-everything]
> > > 
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> > > config: sh-allmodconfig
> > > compiler: sh4-linux-gcc (GCC) 12.1.0
> > > reproduce (this is a W=1 build):
> > >          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >          chmod +x ~/bin/make.cross
> > >          # https://github.com/intel-lab-lkp/linux/commit/503a31451202f89e58bc5f0a49261398fafbd90e
> > >          git remote add linux-review https://github.com/intel-lab-lkp/linux
> > >          git fetch --no-tags linux-review Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
> > >          git checkout 503a31451202f89e58bc5f0a49261398fafbd90e
> > >          # save the config file
> > >          mkdir build_dir && cp config build_dir/.config
> > >          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare
> > 
> > I can only find binutils-sh-linux-gnu on fedora, but no gcc-sh-linux-gnu
> > for cross compiling superh kernel. So I tried above steps, but it failed
> > as below, any suggestion I can fix it to proceed for reproducing the
> > reporting issues? Thanks in advance.
> 
> Hi Baoquan,
> 
> Sorry for the inconvenience, /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin
> /sh4-linux-gcc is for x86_64 machine, as the name "ampere-mtsnow-altra",
> is it a arm server?

Oops, I thought the cross compilig can be done on any platform. Since
I need test arm64 patch in this patchset, so didn't get x86 system to
reproduce. Didn't realize lkp has an assumption. I will get a x86 system
to test this.

Thanks for your help!
> 
> the tool for arm64 can be downloaded from
> https://cdn.kernel.org/pub/tools/crosstool/files/bin/arm64/12.1.0/
> 
> Best Regards,
> Rong Chen
> 
> > 
> > [root@ampere-mtsnow-altra-09 ~]# ls
> > 0day  anaconda-ks.cfg  bin  config  EFI_BOOT_ENTRY.TXT  linux  NETBOOT_METHOD.TXT  original-ks.cfg  RECIPE.TXT
> > [root@ampere-mtsnow-altra-09 ~]# ls 0day/gcc-12.1.0-nolibc/sh4-linux/bin/
> > sh4-linux-addr2line  sh4-linux-elfedit     sh4-linux-gcc-ranlib  sh4-linux-ld        sh4-linux-objdump  sh4-linux-strip
> > sh4-linux-ar         sh4-linux-gcc         sh4-linux-gcov        sh4-linux-ld.bfd    sh4-linux-ranlib
> > sh4-linux-as         sh4-linux-gcc-12.1.0  sh4-linux-gcov-dump   sh4-linux-lto-dump  sh4-linux-readelf
> > sh4-linux-c++filt    sh4-linux-gcc-ar      sh4-linux-gcov-tool   sh4-linux-nm        sh4-linux-size
> > sh4-linux-cpp        sh4-linux-gcc-nm      sh4-linux-gprof       sh4-linux-objcopy   sh4-linux-strings
> > [root@ampere-mtsnow-altra-09 ~]# ls bin
> > make.cross
> > [root@ampere-mtsnow-altra-09 ~]# cd linux/
> > [root@ampere-mtsnow-altra-09 linux]# ls
> > arch       certs    CREDITS        drivers  init      Kbuild   lib          Makefile  README   security  usr
> > block      config   crypto         fs       io_uring  Kconfig  LICENSES     mm        samples  sound     virt
> > build_dir  COPYING  Documentation  include  ipc       kernel   MAINTAINERS  net       scripts  tools
> > [root@ampere-mtsnow-altra-09 linux]# ls build_dir/  -a
> > .  ..  arch  .config  .gitignore  include  Makefile  scripts  source  usr
> > 
> > [root@ampere-mtsnow-altra-09 linux]# COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare
> > Compiler will be installed in /root/0day
> > 	not a dynamic executable
> > make --keep-going CONFIG_OF_ALL_DTBS=y CONFIG_DTC=y CROSS_COMPILE=/root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux- --jobs=160 W=1 O=build_dir ARCH=sh prepare
> > make[1]: Entering directory '/root/linux/build_dir'
> > /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> > /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> > /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> >    SYNC    include/config/auto.conf.cmd
> > /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> > /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> > /bin/sh: line 1: /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: cannot execute binary file: Exec format error
> >    GEN     Makefile
> >    HOSTCC  scripts/basic/fixdep
> >    HOSTCC  scripts/kconfig/conf.o
> >    HOSTCC  scripts/kconfig/confdata.o
> >    HOSTCC  scripts/kconfig/expr.o
> >    LEX     scripts/kconfig/lexer.lex.c
> >    YACC    scripts/kconfig/parser.tab.[ch]
> >    HOSTCC  scripts/kconfig/menu.o
> >    HOSTCC  scripts/kconfig/preprocess.o
> >    HOSTCC  scripts/kconfig/symbol.o
> >    HOSTCC  scripts/kconfig/util.o
> >    HOSTCC  scripts/kconfig/lexer.lex.o
> >    HOSTCC  scripts/kconfig/parser.tab.o
> >    HOSTLD  scripts/kconfig/conf
> > /root/0day/gcc-12.1.0-nolibc/sh4-linux/bin/sh4-linux-gcc: unknown compiler
> > scripts/Kconfig.include:44: Sorry, this compiler is not supported.
> > make[3]: *** [../scripts/kconfig/Makefile:77: syncconfig] Error 1
> > make[2]: *** [../Makefile:632: syncconfig] Error 2
> > make[1]: *** [/root/linux/Makefile:734: include/config/auto.conf.cmd] Error 2
> > make[1]: Failed to remake makefile 'include/config/auto.conf.cmd'.
> > make[1]: Failed to remake makefile 'include/config/auto.conf'.
> >    GEN     Makefile
> >    SYSHDR  arch/sh/include/generated/uapi/asm/unistd_32.h
> >    SYSTBL  arch/sh/include/generated/asm/syscall_table.h
> > Error: kernelrelease not valid - run 'make prepare' to update it
> >    HOSTCC  scripts/dtc/dtc.o
> >    HOSTCC  scripts/dtc/flattree.o
> >    HOSTCC  scripts/dtc/fstree.o
> >    HOSTCC  scripts/dtc/data.o
> >    HOSTCC  scripts/dtc/livetree.o
> >    HOSTCC  scripts/dtc/treesource.o
> >    HOSTCC  scripts/dtc/srcpos.o
> >    HOSTCC  scripts/dtc/checks.o
> >    HOSTCC  scripts/dtc/util.o
> >    LEX     scripts/dtc/dtc-lexer.lex.c
> >    YACC    scripts/dtc/dtc-parser.tab.[ch]
> >    HOSTCC  scripts/dtc/libfdt/fdt_ro.o
> >    HOSTCC  scripts/dtc/libfdt/fdt.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_wip.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_sw.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_rw.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_strerror.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_empty_tree.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_addresses.o
> >    HOSTCC  scripts/dtc/libfdt/fdt_overlay.o
> >    HOSTCC  scripts/dtc/fdtoverlay.o
> >    HOSTCC  scripts/dtc/dtc-lexer.lex.o
> >    HOSTCC  scripts/dtc/dtc-parser.tab.o
> >    HOSTLD  scripts/dtc/fdtoverlay
> >    HOSTLD  scripts/dtc/dtc
> > make[1]: Target 'prepare' not remade because of errors.
> > make[1]: Leaving directory '/root/linux/build_dir'
> > make: *** [Makefile:222: __sub-make] Error 2
> > make: Target 'prepare' not remade because of errors.
> > _______________________________________________
> > kbuild-all mailing list -- kbuild-all@lists.01.org
> > To unsubscribe send an email to kbuild-all-leave@lists.01.org
> > 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 10/11] sh: mm: Convert to GENERIC_IOREMAP

[Baoquan He]

On 08/20/22 at 11:41am, kernel test robot wrote:
> Hi Baoquan,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on akpm-mm/mm-everything]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> config: sh-allmodconfig
> compiler: sh4-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/503a31451202f89e58bc5f0a49261398fafbd90e
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Baoquan-He/mm-ioremap-Convert-architectures-to-take-GENERIC_IOREMAP-way/20220820-083435
>         git checkout 503a31451202f89e58bc5f0a49261398fafbd90e
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh prepare

I finally find gcc-sh-linux-gnu and its dependency on rpmfind.net, and
succeeded to reproduce the building failure.
isl-0.16.1-13
cross-gcc-common
gcc-sh-linux-gnu

Based on previous fixing patch for parisc, below draft patch can fix all
reported building issues on sh.

diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
index 3c5ff82a511a..eb550c72922d 100644
--- a/arch/sh/include/asm/io.h
+++ b/arch/sh/include/asm/io.h
@@ -225,6 +225,9 @@ __BUILD_IOPORT_STRING(q, u64)
 #define IO_SPACE_LIMIT 0xffffffff
 
 /* We really want to try and get these to memcpy etc */
+#define memset_io memset_io
+#define memcpy_fromio memcpy_fromio
+#define memcpy_toio memcpy_toio
 void memcpy_fromio(void *, const volatile void __iomem *, unsigned long);
 void memcpy_toio(volatile void __iomem *, const void *, unsigned long);
 void memset_io(volatile void __iomem *, int, unsigned long);
@@ -256,18 +259,17 @@ int arch_iounmap(void __iomem *addr);
 
 #define ioremap_cache(addr, size)  \
 	ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
-#define ioremap_cache ioremap_cache
 
 #define ioremap_uc	ioremap
 
-#include <asm-generic/io.h>
-
 /*
  * Convert a physical pointer to a virtual kernel pointer for /dev/mem
  * access
  */
 #define xlate_dev_mem_ptr(p)	__va(p)
 
+#include <asm-generic/io.h>
+
 #define ARCH_HAS_VALID_PHYS_ADDR_RANGE
 int valid_phys_addr_range(phys_addr_t addr, size_t size);
 int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
diff --git a/arch/sh/include/asm/io_noioport.h b/arch/sh/include/asm/io_noioport.h
index f7938fe0f911..5ba4116b4265 100644
--- a/arch/sh/include/asm/io_noioport.h
+++ b/arch/sh/include/asm/io_noioport.h
@@ -53,6 +53,13 @@ static inline void ioport_unmap(void __iomem *addr)
 #define outw_p(x, addr)	outw((x), (addr))
 #define outl_p(x, addr)	outl((x), (addr))
 
+#define insb insb
+#define insw insw
+#define insl insl
+#define outsb outsb
+#define outsw outsw
+#define outsl outsl
+
 static inline void insb(unsigned long port, void *dst, unsigned long count)
 {
 	BUG();
diff --git a/arch/sh/mm/ioremap.c b/arch/sh/mm/ioremap.c
index 720a9186b06b..725a1623675a 100644
--- a/arch/sh/mm/ioremap.c
+++ b/arch/sh/mm/ioremap.c
@@ -72,7 +72,7 @@ __ioremap_29bit(phys_addr_t offset, unsigned long size, pgprot_t prot)
 #define __ioremap_29bit(offset, size, prot)		NULL
 #endif /* CONFIG_29BIT */
 
-void __iomem *
+void __iomem * __ref
 arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
 	unsigned long last_addr, phys_addr = *paddr;
@@ -102,7 +102,8 @@ arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 	 * First try to remap through the PMB.
 	 * PMB entries are all pre-faulted.
 	 */
-	mapped = pmb_remap_caller(phys_addr, size, pgprot, caller);
+	mapped = pmb_remap_caller(phys_addr, size, pgprot,
+			__builtin_return_address(0));
 	if (mapped && !IS_ERR(mapped))
 		return mapped;
 
@@ -129,7 +130,6 @@ static inline int iomapping_nontranslatable(unsigned long offset)
 int arch_iounmap(void __iomem *addr)
 {
 	unsigned long vaddr = (unsigned long __force)addr;
-	struct vm_struct *p;
 
 	/*
 	 * Nothing to do if there is no translatable mapping.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

Hi Christophe,

On 08/28/22 at 07:10pm, Baoquan He wrote:
> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
......
> > >>>> Is it really the best approach ? Wouldn't it be better to have helpers
> > >>>> to do that, those helpers being called by the ioremap_prot(), instead of
> > >>>> doing it inside the arch_ioremap() function ?
> > >>>
> > >>> This is suggested too by Alexander during his v1 reviewing. I tried, but
> > >>> feel the current way taken in this patchset is better. Because not all
> > >>> architecutres need the address fix up, only parisc, and only few need
> > >>> adjust the 'prot'. Introducing other helpers seems too much, that only
> > >>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
> > >>> method for people to understand and take.
> > >>
> > >> I can't understand. Why is it difficult to do something like:
> > >>
> > >> #ifndef ioremap_adjust_prot
> > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> > >> {
> > >> 	return flags;
> > >> }
> > >> #endif
> > >>
> > >> Then for arc you do
> > >>
> > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> > >> {
> > >> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
> > >> }
> > >> #define ioremap_adjust_prot ioremap_adjust_prot
> > > 
> > > My thinking is we have four things to do in the added hookers.
> > > 1) check if we should do ioremap on ARCHes. If not, return NULL from
> > > ioremap_prot();
> > > 2) handling the mapping io address specifically on ARCHes, e.g arc,
> > > ia64, s390;
> > > 3) the original physical address passed into ioremap_prot() need be
> > > fixed up, e.g arc;
> > > 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
> > > and xtensa.
> > > 
> > > With Kefeng's patches, the case 1) is handled with introduced
> > > ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
> > > rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
> > > 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
> > > case is not difficult from my side. I worry that as time goes by, those
> > > several hooks my cause issue, e.g if a new adjustment need be done,
> > > should we introduce a new helper or make do with the existed hook; how
> > > 
> > > When I investigated this, one arch_ioremap() looks not complicated
> > > since not all ARCHes need cover all above 4 cases. That's why I finally
> > > choose one hook. I am open to new idea, please let me know if we should
> > > change it to introduce several different helpers.
> > > 
> > 
> > A new idea that would have my preference would be to do just like we did 
> > with arch_get_unmapped_area(). Look at 
> > https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 
> > and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
> > 
> > Instead of having the generic that calls the arch specific, make it the 
> > other way round, have the arch specific call the generic after doing its 
> > specialties.
> 
> This sounds good. I made a draft patch to change code in generic code
> part, just showing what it looks like.
> 
> Both arch_ioremap() way and the arch sepcific call the generic way look
> good to me. Just it will take less effort for me to continue the
> arch_ioremap() way. I would like to hear Christoph's opinion since he
> introduced the GENERIC_IOREMAP method and suggested the earlier
> arch_ioremap() way and change in this patchset. 

I will make another round change and post. Since Christoph doesn't
reply, I would like to continue with the existing
arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
on the new idea to reintroduce ioremap().

> 
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 68a8117b30fa..4bc3e18c475f 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1047,35 +1047,18 @@ static inline void iounmap(volatile void __iomem *addr)
>  #elif defined(CONFIG_GENERIC_IOREMAP)
>  #include <linux/pgtable.h>
>  
> -/*
> - * Arch code can implement the following two hooks when using GENERIC_IOREMAP
> - * arch_ioremap() return a bool,
> - *   - IS_ERR means return an error
> - *   - NULL means continue to remap
> - *   - a non-NULL, non-IS_ERR pointer is returned directly
> - * arch_iounmap() return a bool,
> - *   - 0 means continue to vunmap
> - *   - error code means skip vunmap and return directly
> - */
> -#ifndef arch_ioremap
> -#define arch_ioremap arch_ioremap
> -static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size,
> -				   unsigned long *prot_val)
> -{
> -	return NULL;
> -}
> -#endif
> +void __iomem *
> +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot);
>  
> -#ifndef arch_iounmap
> -#define arch_iounmap arch_iounmap
> -static inline int arch_iounmap(void __iomem *addr)
> +#ifndef ioremap_prot
> +#define ioremap_prot ioremap_prot
> +void __iomem *
> +ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot);
>  {
> -	return 0;
> +	return generic_ioremap_prot(phys_addr, size, prot);
>  }
>  #endif
>  
> -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
> -			   unsigned long prot);
>  void iounmap(volatile void __iomem *addr);
>  
>  #ifndef ioremap
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 7914b5cf5b78..87d51003dee6 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, unsigned long prot)
>  {
>  	unsigned long offset, vaddr;
>  	phys_addr_t last_addr;
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christophe Leroy]

Hi Baoquan,

Le 12/09/2022 à 04:55, Baoquan He a écrit :
> Hi Christophe,
> 
> On 08/28/22 at 07:10pm, Baoquan He wrote:
>> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
> ......
>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers
>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of
>>>>>>> doing it inside the arch_ioremap() function ?
>>>>>>
>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
>>>>>> feel the current way taken in this patchset is better. Because not all
>>>>>> architecutres need the address fix up, only parisc, and only few need
>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only
>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
>>>>>> method for people to understand and take.
>>>>>
>>>>> I can't understand. Why is it difficult to do something like:
>>>>>
>>>>> #ifndef ioremap_adjust_prot
>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>> {
>>>>> 	return flags;
>>>>> }
>>>>> #endif
>>>>>
>>>>> Then for arc you do
>>>>>
>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>> {
>>>>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
>>>>> }
>>>>> #define ioremap_adjust_prot ioremap_adjust_prot
>>>>
>>>> My thinking is we have four things to do in the added hookers.
>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from
>>>> ioremap_prot();
>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc,
>>>> ia64, s390;
>>>> 3) the original physical address passed into ioremap_prot() need be
>>>> fixed up, e.g arc;
>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
>>>> and xtensa.
>>>>
>>>> With Kefeng's patches, the case 1) is handled with introduced
>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
>>>> case is not difficult from my side. I worry that as time goes by, those
>>>> several hooks my cause issue, e.g if a new adjustment need be done,
>>>> should we introduce a new helper or make do with the existed hook; how
>>>>
>>>> When I investigated this, one arch_ioremap() looks not complicated
>>>> since not all ARCHes need cover all above 4 cases. That's why I finally
>>>> choose one hook. I am open to new idea, please let me know if we should
>>>> change it to introduce several different helpers.
>>>>
>>>
>>> A new idea that would have my preference would be to do just like we did
>>> with arch_get_unmapped_area(). Look at
>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
>>>
>>> Instead of having the generic that calls the arch specific, make it the
>>> other way round, have the arch specific call the generic after doing its
>>> specialties.
>>
>> This sounds good. I made a draft patch to change code in generic code
>> part, just showing what it looks like.
>>
>> Both arch_ioremap() way and the arch sepcific call the generic way look
>> good to me. Just it will take less effort for me to continue the
>> arch_ioremap() way. I would like to hear Christoph's opinion since he
>> introduced the GENERIC_IOREMAP method and suggested the earlier
>> arch_ioremap() way and change in this patchset.
> 
> I will make another round change and post. Since Christoph doesn't
> reply, I would like to continue with the existing
> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
> on the new idea to reintroduce ioremap().
> 

I still dislike you approach with the architectures modifying local vars 
by reference, and as you said earlier I'm not the only one : "This is 
suggested too by Alexander during his v1 reviewing".

So I'd really prefer you to reconsider your approach and avoid passign 
pointers to local vars to architecture helpers.

Thanks
Christophe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On 09/12/22 at 07:10am, Christophe Leroy wrote:
> Hi Baoquan,
> 
> Le 12/09/2022 à 04:55, Baoquan He a écrit :
> > Hi Christophe,
> > 
> > On 08/28/22 at 07:10pm, Baoquan He wrote:
> >> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
> > ......
> >>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers
> >>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of
> >>>>>>> doing it inside the arch_ioremap() function ?
> >>>>>>
> >>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
> >>>>>> feel the current way taken in this patchset is better. Because not all
> >>>>>> architecutres need the address fix up, only parisc, and only few need
> >>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only
> >>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
> >>>>>> method for people to understand and take.
> >>>>>
> >>>>> I can't understand. Why is it difficult to do something like:
> >>>>>
> >>>>> #ifndef ioremap_adjust_prot
> >>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> >>>>> {
> >>>>> 	return flags;
> >>>>> }
> >>>>> #endif
> >>>>>
> >>>>> Then for arc you do
> >>>>>
> >>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> >>>>> {
> >>>>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
> >>>>> }
> >>>>> #define ioremap_adjust_prot ioremap_adjust_prot
> >>>>
> >>>> My thinking is we have four things to do in the added hookers.
> >>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from
> >>>> ioremap_prot();
> >>>> 2) handling the mapping io address specifically on ARCHes, e.g arc,
> >>>> ia64, s390;
> >>>> 3) the original physical address passed into ioremap_prot() need be
> >>>> fixed up, e.g arc;
> >>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
> >>>> and xtensa.
> >>>>
> >>>> With Kefeng's patches, the case 1) is handled with introduced
> >>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
> >>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
> >>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
> >>>> case is not difficult from my side. I worry that as time goes by, those
> >>>> several hooks my cause issue, e.g if a new adjustment need be done,
> >>>> should we introduce a new helper or make do with the existed hook; how
> >>>>
> >>>> When I investigated this, one arch_ioremap() looks not complicated
> >>>> since not all ARCHes need cover all above 4 cases. That's why I finally
> >>>> choose one hook. I am open to new idea, please let me know if we should
> >>>> change it to introduce several different helpers.
> >>>>
> >>>
> >>> A new idea that would have my preference would be to do just like we did
> >>> with arch_get_unmapped_area(). Look at
> >>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
> >>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
> >>>
> >>> Instead of having the generic that calls the arch specific, make it the
> >>> other way round, have the arch specific call the generic after doing its
> >>> specialties.
> >>
> >> This sounds good. I made a draft patch to change code in generic code
> >> part, just showing what it looks like.
> >>
> >> Both arch_ioremap() way and the arch sepcific call the generic way look
> >> good to me. Just it will take less effort for me to continue the
> >> arch_ioremap() way. I would like to hear Christoph's opinion since he
> >> introduced the GENERIC_IOREMAP method and suggested the earlier
> >> arch_ioremap() way and change in this patchset.
> > 
> > I will make another round change and post. Since Christoph doesn't
> > reply, I would like to continue with the existing
> > arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
> > on the new idea to reintroduce ioremap().
> > 
> 
> I still dislike you approach with the architectures modifying local vars 
> by reference, and as you said earlier I'm not the only one : "This is 
> suggested too by Alexander during his v1 reviewing".

Alexander suggested several helpers, as I have explained earlier, that
will cause at least four helpers currently. And could be more later if
new change is introduced. And the address fixup and prot modifcation
are related in few architecutures. Adding all of them is is not so
necessary.

> 
> So I'd really prefer you to reconsider your approach and avoid passign 
> pointers to local vars to architecture helpers.

If only passing pointers to local vars is disliked, I can explain why I
did so. Let me take arch_ioremap() of a64 as example. I can derefence
pointer in arch_ioremap() to avoid assigning pointers to local vars.
Please see below two version for comparing, and please tell which one is
better.

To me, assigning pointers to local vars make code simple and clean,
honestly. 

******
The version in which pointers assigned to local vars looks like below.
The old phys_addr is passed in, the assigning can decrease changed line.
diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
index 55fd3eb753ff..8a085fc660e3 100644
--- a/arch/ia64/mm/ioremap.c
+++ b/arch/ia64/mm/ioremap.c
@@ -30,15 +30,12 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
 }
 
 void __iomem *
-ioremap (unsigned long phys_addr, unsigned long size)
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	void __iomem *addr;
-	struct vm_struct *area;
-	unsigned long offset;
-	pgprot_t prot;
-	u64 attr;
+	phys_addr_t phys_addr = *paddr;
 	unsigned long gran_base, gran_size;
 	unsigned long page_base;
+	u64 attr;
 
 	/*
 	 * For things in kern_memmap, we must use the same attribute
@@ -69,35 +66,18 @@ 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);
+		return NULL;
 	}
 
 	return __ioremap_uc(phys_addr);
 }
-EXPORT_SYMBOL(ioremap);
+
+int arch_iounmap(void __iomem *addr)
+{
+	if (REGION_NUMBER(addr) != RGN_GATE)
+		return -EINVAL;
+	return 0;
+}
 
 void __iomem *
 ioremap_uc(unsigned long phys_addr, unsigned long size)


*********
The version in which pointer is dereferenced directly in place. Then
more lines of code are involved. And some pointer derefencing takes
place in macro, so bracket is needed.
diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
index 55fd3eb753ff..e1b991dc2347 100644
--- a/arch/ia64/mm/ioremap.c
+++ b/arch/ia64/mm/ioremap.c
@@ -30,35 +30,32 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
 }
 
 void __iomem *
-ioremap (unsigned long phys_addr, unsigned long size)
+arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
 {
-	void __iomem *addr;
-	struct vm_struct *area;
-	unsigned long offset;
-	pgprot_t prot;
-	u64 attr;
+	phys_addr_t phys_addr = *paddr;
 	unsigned long gran_base, gran_size;
 	unsigned long page_base;
+	u64 attr;
 
 	/*
 	 * For things in kern_memmap, we must use the same attribute
 	 * as the rest of the kernel.  For more details, see
 	 * Documentation/ia64/aliasing.rst.
 	 */
-	attr = kern_mem_attribute(phys_addr, size);
+	attr = kern_mem_attribute(*paddr, size);
 	if (attr & EFI_MEMORY_WB)
-		return (void __iomem *) phys_to_virt(phys_addr);
+		return (void __iomem *) phys_to_virt(*paddr);
 	else if (attr & EFI_MEMORY_UC)
-		return __ioremap_uc(phys_addr);
+		return __ioremap_uc(*paddr);
 
 	/*
 	 * Some chipsets don't support UC access to memory.  If
 	 * WB is supported for the whole granule, we prefer that.
 	 */
-	gran_base = GRANULEROUNDDOWN(phys_addr);
-	gran_size = GRANULEROUNDUP(phys_addr + size) - gran_base;
+	gran_base = GRANULEROUNDDOWN(*paddr);
+	gran_size = GRANULEROUNDUP(*paddr + size) - gran_base;
 	if (efi_mem_attribute(gran_base, gran_size) & EFI_MEMORY_WB)
-		return (void __iomem *) phys_to_virt(phys_addr);
+		return (void __iomem *) phys_to_virt(*paddr);
 
 	/*
 	 * WB is not supported for the whole granule, so we can't use
@@ -66,38 +63,21 @@ ioremap (unsigned long phys_addr, unsigned long size)
 	 * area with kernel page table mappings, we can use those
 	 * instead.
 	 */
-	page_base = phys_addr & PAGE_MASK;
-	size = PAGE_ALIGN(phys_addr + size) - page_base;
+	page_base = (*paddr) & PAGE_MASK;
+	size = PAGE_ALIGN(*paddr + 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);
+		return NULL;
 	}
 
-	return __ioremap_uc(phys_addr);
+	return __ioremap_uc(*paddr);
+}
+
+int arch_iounmap(void __iomem *addr)
+{
+	if (REGION_NUMBER(addr) != RGN_GATE)
+		return -EINVAL;
+	return 0;
 }
-EXPORT_SYMBOL(ioremap);
 
 void __iomem *
 ioremap_uc(unsigned long phys_addr, unsigned long size)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christophe Leroy]

Le 13/09/2022 à 17:11, Baoquan He a écrit :
> On 09/12/22 at 07:10am, Christophe Leroy wrote:
>> Hi Baoquan,
>>
>> Le 12/09/2022 à 04:55, Baoquan He a écrit :
>>> Hi Christophe,
>>>
>>> On 08/28/22 at 07:10pm, Baoquan He wrote:
>>>> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
>>> ......
>>>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers
>>>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of
>>>>>>>>> doing it inside the arch_ioremap() function ?
>>>>>>>>
>>>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
>>>>>>>> feel the current way taken in this patchset is better. Because not all
>>>>>>>> architecutres need the address fix up, only parisc, and only few need
>>>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only
>>>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
>>>>>>>> method for people to understand and take.
>>>>>>>
>>>>>>> I can't understand. Why is it difficult to do something like:
>>>>>>>
>>>>>>> #ifndef ioremap_adjust_prot
>>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>>>> {
>>>>>>> 	return flags;
>>>>>>> }
>>>>>>> #endif
>>>>>>>
>>>>>>> Then for arc you do
>>>>>>>
>>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>>>> {
>>>>>>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
>>>>>>> }
>>>>>>> #define ioremap_adjust_prot ioremap_adjust_prot
>>>>>>
>>>>>> My thinking is we have four things to do in the added hookers.
>>>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from
>>>>>> ioremap_prot();
>>>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc,
>>>>>> ia64, s390;
>>>>>> 3) the original physical address passed into ioremap_prot() need be
>>>>>> fixed up, e.g arc;
>>>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
>>>>>> and xtensa.
>>>>>>
>>>>>> With Kefeng's patches, the case 1) is handled with introduced
>>>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
>>>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
>>>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
>>>>>> case is not difficult from my side. I worry that as time goes by, those
>>>>>> several hooks my cause issue, e.g if a new adjustment need be done,
>>>>>> should we introduce a new helper or make do with the existed hook; how
>>>>>>
>>>>>> When I investigated this, one arch_ioremap() looks not complicated
>>>>>> since not all ARCHes need cover all above 4 cases. That's why I finally
>>>>>> choose one hook. I am open to new idea, please let me know if we should
>>>>>> change it to introduce several different helpers.
>>>>>>
>>>>>
>>>>> A new idea that would have my preference would be to do just like we did
>>>>> with arch_get_unmapped_area(). Look at
>>>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
>>>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
>>>>>
>>>>> Instead of having the generic that calls the arch specific, make it the
>>>>> other way round, have the arch specific call the generic after doing its
>>>>> specialties.
>>>>
>>>> This sounds good. I made a draft patch to change code in generic code
>>>> part, just showing what it looks like.
>>>>
>>>> Both arch_ioremap() way and the arch sepcific call the generic way look
>>>> good to me. Just it will take less effort for me to continue the
>>>> arch_ioremap() way. I would like to hear Christoph's opinion since he
>>>> introduced the GENERIC_IOREMAP method and suggested the earlier
>>>> arch_ioremap() way and change in this patchset.
>>>
>>> I will make another round change and post. Since Christoph doesn't
>>> reply, I would like to continue with the existing
>>> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
>>> on the new idea to reintroduce ioremap().
>>>
>>
>> I still dislike you approach with the architectures modifying local vars
>> by reference, and as you said earlier I'm not the only one : "This is
>> suggested too by Alexander during his v1 reviewing".
> 
> Alexander suggested several helpers, as I have explained earlier, that
> will cause at least four helpers currently. And could be more later if
> new change is introduced. And the address fixup and prot modifcation
> are related in few architecutures. Adding all of them is is not so
> necessary.
> 
>>
>> So I'd really prefer you to reconsider your approach and avoid passign
>> pointers to local vars to architecture helpers.
> 
> If only passing pointers to local vars is disliked, I can explain why I
> did so. Let me take arch_ioremap() of a64 as example. I can derefence
> pointer in arch_ioremap() to avoid assigning pointers to local vars.
> Please see below two version for comparing, and please tell which one is
> better.

Ok, yes I overlooked and didn't remember it right.

> 
> To me, assigning pointers to local vars make code simple and clean,
> honestly.

Well, for me it looks ood, not intellectually natural.

If I understand correctly, you do

ioremap()
   --> Call arch_ioremap()
     --> If the arch doesn't want to handle ioremap itself, it returns NULL
     --> Then you fallback on generic handling.

The arch may say "I don't want to handle it", but at the same time it 
blindly modifies the parameters so that the generic handling is not 
exacly the generic handling.
Not easy to follow for the reader. Do you have any exemple in the kernel 
that works more or less with the same approach ?


What I propose is

Arch specific ioremap()
   --> do proper preparation
   --> call generic_ioremap()

And the generic fallback implementation when the arch doesn't have a 
specific ioremap()

__weak ioremap()
   --> call generic_ioremap().

The above looks a lot more natural and easier to follow, it is clear for 
the reader which function does what.


Christophe



> 
> ******
> The version in which pointers assigned to local vars looks like below.
> The old phys_addr is passed in, the assigning can decrease changed line.
> diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
> index 55fd3eb753ff..8a085fc660e3 100644
> --- a/arch/ia64/mm/ioremap.c
> +++ b/arch/ia64/mm/ioremap.c
> @@ -30,15 +30,12 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
>   }
>   
>   void __iomem *
> -ioremap (unsigned long phys_addr, unsigned long size)
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>   {
> -	void __iomem *addr;
> -	struct vm_struct *area;
> -	unsigned long offset;
> -	pgprot_t prot;
> -	u64 attr;
> +	phys_addr_t phys_addr = *paddr;
>   	unsigned long gran_base, gran_size;
>   	unsigned long page_base;
> +	u64 attr;
>   
>   	/*
>   	 * For things in kern_memmap, we must use the same attribute
> @@ -69,35 +66,18 @@ 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);
> +		return NULL;
>   	}
>   
>   	return __ioremap_uc(phys_addr);
>   }
> -EXPORT_SYMBOL(ioremap);
> +
> +int arch_iounmap(void __iomem *addr)
> +{
> +	if (REGION_NUMBER(addr) != RGN_GATE)
> +		return -EINVAL;
> +	return 0;
> +}
>   
>   void __iomem *
>   ioremap_uc(unsigned long phys_addr, unsigned long size)
> 
> 
> *********
> The version in which pointer is dereferenced directly in place. Then
> more lines of code are involved. And some pointer derefencing takes
> place in macro, so bracket is needed.
> diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c
> index 55fd3eb753ff..e1b991dc2347 100644
> --- a/arch/ia64/mm/ioremap.c
> +++ b/arch/ia64/mm/ioremap.c
> @@ -30,35 +30,32 @@ early_ioremap (unsigned long phys_addr, unsigned long size)
>   }
>   
>   void __iomem *
> -ioremap (unsigned long phys_addr, unsigned long size)
> +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val)
>   {
> -	void __iomem *addr;
> -	struct vm_struct *area;
> -	unsigned long offset;
> -	pgprot_t prot;
> -	u64 attr;
> +	phys_addr_t phys_addr = *paddr;
>   	unsigned long gran_base, gran_size;
>   	unsigned long page_base;
> +	u64 attr;
>   
>   	/*
>   	 * For things in kern_memmap, we must use the same attribute
>   	 * as the rest of the kernel.  For more details, see
>   	 * Documentation/ia64/aliasing.rst.
>   	 */
> -	attr = kern_mem_attribute(phys_addr, size);
> +	attr = kern_mem_attribute(*paddr, size);
>   	if (attr & EFI_MEMORY_WB)
> -		return (void __iomem *) phys_to_virt(phys_addr);
> +		return (void __iomem *) phys_to_virt(*paddr);
>   	else if (attr & EFI_MEMORY_UC)
> -		return __ioremap_uc(phys_addr);
> +		return __ioremap_uc(*paddr);
>   
>   	/*
>   	 * Some chipsets don't support UC access to memory.  If
>   	 * WB is supported for the whole granule, we prefer that.
>   	 */
> -	gran_base = GRANULEROUNDDOWN(phys_addr);
> -	gran_size = GRANULEROUNDUP(phys_addr + size) - gran_base;
> +	gran_base = GRANULEROUNDDOWN(*paddr);
> +	gran_size = GRANULEROUNDUP(*paddr + size) - gran_base;
>   	if (efi_mem_attribute(gran_base, gran_size) & EFI_MEMORY_WB)
> -		return (void __iomem *) phys_to_virt(phys_addr);
> +		return (void __iomem *) phys_to_virt(*paddr);
>   
>   	/*
>   	 * WB is not supported for the whole granule, so we can't use
> @@ -66,38 +63,21 @@ ioremap (unsigned long phys_addr, unsigned long size)
>   	 * area with kernel page table mappings, we can use those
>   	 * instead.
>   	 */
> -	page_base = phys_addr & PAGE_MASK;
> -	size = PAGE_ALIGN(phys_addr + size) - page_base;
> +	page_base = (*paddr) & PAGE_MASK;
> +	size = PAGE_ALIGN(*paddr + 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);
> +		return NULL;
>   	}
>   
> -	return __ioremap_uc(phys_addr);
> +	return __ioremap_uc(*paddr);
> +}
> +
> +int arch_iounmap(void __iomem *addr)
> +{
> +	if (REGION_NUMBER(addr) != RGN_GATE)
> +		return -EINVAL;
> +	return 0;
>   }
> -EXPORT_SYMBOL(ioremap);
>   
>   void __iomem *
>   ioremap_uc(unsigned long phys_addr, unsigned long size)
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Baoquan He]

On 09/21/22 at 04:40pm, Christophe Leroy wrote:
> 
> 
> Le 13/09/2022 à 17:11, Baoquan He a écrit :
> > On 09/12/22 at 07:10am, Christophe Leroy wrote:
> >> Hi Baoquan,
> >>
> >> Le 12/09/2022 à 04:55, Baoquan He a écrit :
> >>> Hi Christophe,
> >>>
> >>> On 08/28/22 at 07:10pm, Baoquan He wrote:
> >>>> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
> >>> ......
> >>>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers
> >>>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of
> >>>>>>>>> doing it inside the arch_ioremap() function ?
> >>>>>>>>
> >>>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
> >>>>>>>> feel the current way taken in this patchset is better. Because not all
> >>>>>>>> architecutres need the address fix up, only parisc, and only few need
> >>>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only
> >>>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
> >>>>>>>> method for people to understand and take.
> >>>>>>>
> >>>>>>> I can't understand. Why is it difficult to do something like:
> >>>>>>>
> >>>>>>> #ifndef ioremap_adjust_prot
> >>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> >>>>>>> {
> >>>>>>> 	return flags;
> >>>>>>> }
> >>>>>>> #endif
> >>>>>>>
> >>>>>>> Then for arc you do
> >>>>>>>
> >>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
> >>>>>>> {
> >>>>>>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
> >>>>>>> }
> >>>>>>> #define ioremap_adjust_prot ioremap_adjust_prot
> >>>>>>
> >>>>>> My thinking is we have four things to do in the added hookers.
> >>>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from
> >>>>>> ioremap_prot();
> >>>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc,
> >>>>>> ia64, s390;
> >>>>>> 3) the original physical address passed into ioremap_prot() need be
> >>>>>> fixed up, e.g arc;
> >>>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
> >>>>>> and xtensa.
> >>>>>>
> >>>>>> With Kefeng's patches, the case 1) is handled with introduced
> >>>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
> >>>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
> >>>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
> >>>>>> case is not difficult from my side. I worry that as time goes by, those
> >>>>>> several hooks my cause issue, e.g if a new adjustment need be done,
> >>>>>> should we introduce a new helper or make do with the existed hook; how
> >>>>>>
> >>>>>> When I investigated this, one arch_ioremap() looks not complicated
> >>>>>> since not all ARCHes need cover all above 4 cases. That's why I finally
> >>>>>> choose one hook. I am open to new idea, please let me know if we should
> >>>>>> change it to introduce several different helpers.
> >>>>>>
> >>>>>
> >>>>> A new idea that would have my preference would be to do just like we did
> >>>>> with arch_get_unmapped_area(). Look at
> >>>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
> >>>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
> >>>>>
> >>>>> Instead of having the generic that calls the arch specific, make it the
> >>>>> other way round, have the arch specific call the generic after doing its
> >>>>> specialties.
> >>>>
> >>>> This sounds good. I made a draft patch to change code in generic code
> >>>> part, just showing what it looks like.
> >>>>
> >>>> Both arch_ioremap() way and the arch sepcific call the generic way look
> >>>> good to me. Just it will take less effort for me to continue the
> >>>> arch_ioremap() way. I would like to hear Christoph's opinion since he
> >>>> introduced the GENERIC_IOREMAP method and suggested the earlier
> >>>> arch_ioremap() way and change in this patchset.
> >>>
> >>> I will make another round change and post. Since Christoph doesn't
> >>> reply, I would like to continue with the existing
> >>> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
> >>> on the new idea to reintroduce ioremap().
> >>>
> >>
> >> I still dislike you approach with the architectures modifying local vars
> >> by reference, and as you said earlier I'm not the only one : "This is
> >> suggested too by Alexander during his v1 reviewing".
> > 
> > Alexander suggested several helpers, as I have explained earlier, that
> > will cause at least four helpers currently. And could be more later if
> > new change is introduced. And the address fixup and prot modifcation
> > are related in few architecutures. Adding all of them is is not so
> > necessary.
> > 
> >>
> >> So I'd really prefer you to reconsider your approach and avoid passign
> >> pointers to local vars to architecture helpers.
> > 
> > If only passing pointers to local vars is disliked, I can explain why I
> > did so. Let me take arch_ioremap() of a64 as example. I can derefence
> > pointer in arch_ioremap() to avoid assigning pointers to local vars.
> > Please see below two version for comparing, and please tell which one is
> > better.
> 
> Ok, yes I overlooked and didn't remember it right.
> 
> > 
> > To me, assigning pointers to local vars make code simple and clean,
> > honestly.
> 
> Well, for me it looks ood, not intellectually natural.
> 
> If I understand correctly, you do
> 
> ioremap()
>    --> Call arch_ioremap()
>      --> If the arch doesn't want to handle ioremap itself, it returns NULL
>      --> Then you fallback on generic handling.
> 
> The arch may say "I don't want to handle it", but at the same time it 
> blindly modifies the parameters so that the generic handling is not 
> exacly the generic handling.

No, it's not like that.

Arch_ioremap() provides a place where arch specific handling can be
done. While it doesn't mean arch has to do all of the follow four
things. From the current change I made listed as below, arch_ioremap()
in each architecture is quite simple to do one or two things.

People only need to provide arch_ioremap() if needed, or do not provide
arch_ioremap() at all if not needed. No such thing could hapen like you
said that arch don't want to handle, but it blindly modifes the parameter.

arm64:
  - address checking;
hexagon: removes the ioremap() directly since it has the same code as standard;
  - N/A
arc:
  - address checking
  - modify the prot flag;
ia64:
  - specific io address mapping
openrisc: removes ioremap() directly after its early ioremap is cleared away;
  - N/A
parisc:
  - physical address fixup
  - address checking;
s390:
  - specific io address mapping
sh:
  - specific io address mapping 
xtensa:
  - address checking;

Four optional things could be done in arch_ioremap():
1) check if we should do ioremap on ARCHes. If not, return NULL from
ioremap_prot();
2) handling the mapping io address specifically on ARCHes, e.g arc,
ia64, s390;
3) the original physical address passed into ioremap_prot() need be
fixed up, e.g arc;
4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
and xtensa.

> Not easy to follow for the reader. Do you have any exemple in the kernel 
> that works more or less with the same approach ?
> 
> 
> What I propose is
> 
> Arch specific ioremap()
>    --> do proper preparation
>    --> call generic_ioremap()
> 
> And the generic fallback implementation when the arch doesn't have a 
> specific ioremap()
> 
> __weak ioremap()
>    --> call generic_ioremap().

Hmm, __weak is not suggested any more in kernel. Please see below
thread.

[PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add]
https://lore.kernel.org/all/20220518181828.645877-1-naveen.n.rao@linux.vnet.ibm.com/T/#u


So with arch_ioremap() hook, now in each architecture it is:

  arch_ioremap() is provided and called
  or 
  no hookd is needed at all in arch 

It's simpler, isn't it?

> 
> The above looks a lot more natural and easier to follow, it is clear for 
> the reader which function does what.

Before, there's only arm64 taking GENERIC_IOREMAP way and having
arch_ioremap() as an exmaple. Now after this patchset, there are so many
architectures taking the way, it's very easy to refer to and study,
right?

Thanks
Baoquan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel