1. Enhance generic ioremap to make it more useful.
2. Let's arm64 use GENERIC_IOREMAP to cleanup code.
3. Support HAVE_IOREMAP_PROT on arm64, which enable generic_access_phys(),
it is useful when debug(eg, gdb) via access_process_vm device memory
infrastructure.
v3:
- add cleanup patch to kill ARM's unused arch_iounmap(the naming will be
used in GENERIC_IOREMAP) and add comments for arch_ioremap/arch_iounmap
hooks, per Anshuman Khandual
- collect ack/review
v2:
- s/addr/phys_addr in ioremap_prot, suggested by Andrew Morton
- rename arch_ioremap/iounmap_check to arch_ioremap/iounmad
and change return value, per Christoph Hellwig and Andrew Morton
- and use 'ifndef arch_ioremap' instead of weak function, per Arnd Bergmann
- collect ack/review
Kefeng Wang (6):
ARM: mm: kill unused runtime hook arch_iounmap()
mm: ioremap: Use more sensibly name in ioremap_prot()
mm: ioremap: Setup phys_addr of struct vm_struct
mm: ioremap: Add arch_ioremap/iounmap()
arm64: mm: Convert to GENERIC_IOREMAP
arm64: Add HAVE_IOREMAP_PROT support
.../features/vm/ioremap_prot/arch-support.txt | 2 +-
arch/arm/include/asm/io.h | 4 +-
arch/arm/mm/ioremap.c | 9 +-
arch/arm/mm/nommu.c | 9 +-
arch/arm64/Kconfig | 2 +
arch/arm64/include/asm/io.h | 20 +++--
arch/arm64/include/asm/pgtable.h | 10 +++
arch/arm64/kernel/acpi.c | 2 +-
arch/arm64/mm/hugetlbpage.c | 10 ---
arch/arm64/mm/ioremap.c | 85 +++----------------
include/asm-generic/io.h | 26 +++++-
mm/ioremap.c | 30 +++++--
12 files changed, 88 insertions(+), 121 deletions(-)
--
2.35.3
Since the following commits,
v5.4
commit 59d3ae9a5bf6 ("ARM: remove Intel iop33x and iop13xx support")
v5.11
commit 3e3f354bc383 ("ARM: remove ebsa110 platform")
The runtime hook arch_iounmap() on ARM is useless, kill arch_iounmap()
and __iounmap(), and the naming of arch_iounmap will be used in
GENERIC_IOREMAP with the later patch.
Cc: Russell King <[email protected]>
Signed-off-by: Kefeng Wang <[email protected]>
---
arch/arm/include/asm/io.h | 4 +---
arch/arm/mm/ioremap.c | 9 +--------
arch/arm/mm/nommu.c | 9 +--------
3 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 2a0739a2350b..d70727c9968f 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -139,11 +139,9 @@ extern void __iomem *__arm_ioremap_caller(phys_addr_t, size_t, unsigned int,
extern void __iomem *__arm_ioremap_pfn(unsigned long, unsigned long, size_t, unsigned int);
extern void __iomem *__arm_ioremap_exec(phys_addr_t, size_t, bool cached);
void __arm_iomem_set_ro(void __iomem *ptr, size_t size);
-extern void __iounmap(volatile void __iomem *addr);
extern void __iomem * (*arch_ioremap_caller)(phys_addr_t, size_t,
unsigned int, void *);
-extern void (*arch_iounmap)(volatile void __iomem *);
/*
* Bad read/write accesses...
@@ -399,7 +397,7 @@ void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size);
#define ioremap_wc ioremap_wc
#define ioremap_wt ioremap_wc
-void iounmap(volatile void __iomem *iomem_cookie);
+void iounmap(volatile void __iomem *io_addr);
#define iounmap iounmap
void *arch_memremap_wb(phys_addr_t phys_addr, size_t size);
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 290702328a33..e376926d6736 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -418,7 +418,7 @@ void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
__builtin_return_address(0));
}
-void __iounmap(volatile void __iomem *io_addr)
+void iounmap(volatile void __iomem *io_addr)
{
void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);
struct static_vm *svm;
@@ -446,13 +446,6 @@ void __iounmap(volatile void __iomem *io_addr)
vunmap(addr);
}
-
-void (*arch_iounmap)(volatile void __iomem *) = __iounmap;
-
-void iounmap(volatile void __iomem *cookie)
-{
- arch_iounmap(cookie);
-}
EXPORT_SYMBOL(iounmap);
#ifdef CONFIG_PCI
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index 2658f52903da..c42debaded95 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -230,14 +230,7 @@ void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
return (void *)phys_addr;
}
-void __iounmap(volatile void __iomem *addr)
-{
-}
-EXPORT_SYMBOL(__iounmap);
-
-void (*arch_iounmap)(volatile void __iomem *);
-
-void iounmap(volatile void __iomem *addr)
+void iounmap(volatile void __iomem *io_addr)
{
}
EXPORT_SYMBOL(iounmap);
--
2.35.3
With ioremap_prot() definition from generic ioremap, also move
pte_pgprot() from hugetlbpage.c into pgtable.h, then arm64 could
have HAVE_IOREMAP_PROT, which will enable generic_access_phys()
code, it is useful for debug, eg, gdb.
Acked-by: Catalin Marinas <[email protected]>
Reviewed-by: Anshuman Khandual <[email protected]>
Signed-off-by: Kefeng Wang <[email protected]>
---
.../features/vm/ioremap_prot/arch-support.txt | 2 +-
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/pgtable.h | 10 ++++++++++
arch/arm64/mm/hugetlbpage.c | 10 ----------
4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/Documentation/features/vm/ioremap_prot/arch-support.txt b/Documentation/features/vm/ioremap_prot/arch-support.txt
index a6dcbe5f47b6..b39ad5d61216 100644
--- a/Documentation/features/vm/ioremap_prot/arch-support.txt
+++ b/Documentation/features/vm/ioremap_prot/arch-support.txt
@@ -9,7 +9,7 @@
| alpha: | TODO |
| arc: | ok |
| arm: | TODO |
- | arm64: | TODO |
+ | arm64: | ok |
| csky: | TODO |
| h8300: | TODO |
| hexagon: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 56673209fdb9..5e5889049af0 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -186,6 +186,7 @@ config ARM64
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_GCC_PLUGINS
select HAVE_HW_BREAKPOINT if PERF_EVENTS
+ select HAVE_IOREMAP_PROT
select HAVE_IRQ_TIME_ACCOUNTING
select HAVE_KVM
select HAVE_NMI
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index dff2b483ea50..1402a2739024 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -402,6 +402,16 @@ static inline pgprot_t mk_pmd_sect_prot(pgprot_t prot)
return __pgprot((pgprot_val(prot) & ~PMD_TABLE_BIT) | PMD_TYPE_SECT);
}
+/*
+ * Select all bits except the pfn
+ */
+static inline pgprot_t pte_pgprot(pte_t pte)
+{
+ unsigned long pfn = pte_pfn(pte);
+
+ return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
+}
+
#ifdef CONFIG_NUMA_BALANCING
/*
* See the comment in include/linux/pgtable.h
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index cbace1c9e137..38d03406f6aa 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -100,16 +100,6 @@ int pud_huge(pud_t pud)
#endif
}
-/*
- * Select all bits except the pfn
- */
-static inline pgprot_t pte_pgprot(pte_t pte)
-{
- unsigned long pfn = pte_pfn(pte);
-
- return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
-}
-
static int find_num_contig(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, size_t *pgsize)
{
--
2.35.3
On 5/19/22 13:55, Kefeng Wang wrote:
> Since the following commits,
>
> v5.4
> commit 59d3ae9a5bf6 ("ARM: remove Intel iop33x and iop13xx support")
> v5.11
> commit 3e3f354bc383 ("ARM: remove ebsa110 platform")
>
> The runtime hook arch_iounmap() on ARM is useless, kill arch_iounmap()
> and __iounmap(), and the naming of arch_iounmap will be used in
> GENERIC_IOREMAP with the later patch.
>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kefeng Wang <[email protected]>
LGTM, also builds on multiple arm configs but will let Russel take a look.
Add special hook for architecture to verify or setup addr, size
or prot when ioremap() or iounmap(), which will make the generic
ioremap more useful.
arch_ioremap() return a pointer,
- IS_ERR means return an error
- NULL means continue to remap
- a non-NULL, non-IS_ERR pointer is directly returned
arch_iounmap() return a int value,
- 0 means continue to vunmap
- error code means skip vunmap and return directly
Signed-off-by: Kefeng Wang <[email protected]>
---
include/asm-generic/io.h | 24 ++++++++++++++++++++++++
mm/ioremap.c | 17 ++++++++++++++---
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index e6ffa2519f08..b60f7151e1d6 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr)
#elif defined(CONFIG_GENERIC_IOREMAP)
#include <linux/pgtable.h>
+/*
+ * Arch code can implement the following two special hooks when using GENERIC_IOREMAP
+ * arch_ioremap() return a pointer,
+ * - 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 int,
+ * - 0 means continue to vunmap
+ * - error code means skip vunmap and return directly
+ */
+#ifndef arch_ioremap
+static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
+{
+ return NULL;
+}
+#endif
+
+#ifndef arch_iounmap
+static inline int arch_iounmap(void __iomem *addr)
+{
+ return 0;
+}
+#endif
+
void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot);
void iounmap(volatile void __iomem *addr);
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 7cb9996b0c12..fac7f23b8c4b 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -16,6 +16,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long pro
unsigned long offset, vaddr;
phys_addr_t last_addr;
struct vm_struct *area;
+ void __iomem *base;
/* Disallow wrap-around or zero size */
last_addr = phys_addr + size - 1;
@@ -27,8 +28,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long pro
phys_addr -= offset;
size = PAGE_ALIGN(size + offset);
- area = get_vm_area_caller(size, VM_IOREMAP,
- __builtin_return_address(0));
+ base = arch_ioremap(phys_addr, size, prot);
+ if (IS_ERR(base))
+ return NULL;
+ else if (base)
+ return base;
+
+ area = get_vm_area_caller(size, VM_IOREMAP, __builtin_return_address(0));
if (!area)
return NULL;
vaddr = (unsigned long)area->addr;
@@ -45,6 +51,11 @@ EXPORT_SYMBOL(ioremap_prot);
void iounmap(volatile void __iomem *addr)
{
- vunmap((void *)((unsigned long)addr & PAGE_MASK));
+ void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK);
+
+ if (arch_iounmap(vaddr))
+ return;
+
+ vunmap(vaddr);
}
EXPORT_SYMBOL(iounmap);
--
2.35.3
Use more meaningful and sensibly naming phys_addr
instead addr in ioremap_prot().
Suggested-by: Andrew Morton <[email protected]>
Reviewed-by: Anshuman Khandual <[email protected]>
Signed-off-by: Kefeng Wang <[email protected]>
---
include/asm-generic/io.h | 2 +-
mm/ioremap.c | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 7ce93aaf69f8..e6ffa2519f08 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -964,7 +964,7 @@ static inline void iounmap(volatile void __iomem *addr)
#elif defined(CONFIG_GENERIC_IOREMAP)
#include <linux/pgtable.h>
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot);
+void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot);
void iounmap(volatile void __iomem *addr);
static inline void __iomem *ioremap(phys_addr_t addr, size_t size)
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 5fe598ecd9b7..1f9597fbcc07 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -11,20 +11,20 @@
#include <linux/io.h>
#include <linux/export.h>
-void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
+void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot)
{
unsigned long offset, vaddr;
phys_addr_t last_addr;
struct vm_struct *area;
/* Disallow wrap-around or zero size */
- last_addr = addr + size - 1;
- if (!size || last_addr < addr)
+ last_addr = phys_addr + size - 1;
+ if (!size || last_addr < phys_addr)
return NULL;
/* Page-align mappings */
- offset = addr & (~PAGE_MASK);
- addr -= offset;
+ offset = phys_addr & (~PAGE_MASK);
+ phys_addr -= offset;
size = PAGE_ALIGN(size + offset);
area = get_vm_area_caller(size, VM_IOREMAP,
@@ -33,7 +33,7 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot)
return NULL;
vaddr = (unsigned long)area->addr;
- if (ioremap_page_range(vaddr, vaddr + size, addr, __pgprot(prot))) {
+ if (ioremap_page_range(vaddr, vaddr + size, phys_addr, __pgprot(prot))) {
free_vm_area(area);
return NULL;
}
--
2.35.3
Show physical address of each ioremap in /proc/vmallocinfo.
Acked-by: Andrew Morton <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Anshuman Khandual <[email protected]>
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/ioremap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/ioremap.c b/mm/ioremap.c
index 1f9597fbcc07..7cb9996b0c12 100644
--- a/mm/ioremap.c
+++ b/mm/ioremap.c
@@ -32,6 +32,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long pro
if (!area)
return NULL;
vaddr = (unsigned long)area->addr;
+ area->phys_addr = phys_addr;
if (ioremap_page_range(vaddr, vaddr + size, phys_addr, __pgprot(prot))) {
free_vm_area(area);
--
2.35.3
On Thu, 19 May 2022 16:25:50 +0800 Kefeng Wang <[email protected]> wrote:
> Add special hook for architecture to verify or setup addr, size
> or prot when ioremap() or iounmap(), which will make the generic
> ioremap more useful.
>
> arch_ioremap() return a pointer,
> - IS_ERR means return an error
> - NULL means continue to remap
> - a non-NULL, non-IS_ERR pointer is directly returned
> arch_iounmap() return a int value,
> - 0 means continue to vunmap
> - error code means skip vunmap and return directly
>
> ...
>
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr)
> #elif defined(CONFIG_GENERIC_IOREMAP)
> #include <linux/pgtable.h>
>
> +/*
> + * Arch code can implement the following two special hooks when using GENERIC_IOREMAP
> + * arch_ioremap() return a pointer,
> + * - 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 int,
> + * - 0 means continue to vunmap
> + * - error code means skip vunmap and return directly
> + */
> +#ifndef arch_ioremap
> +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
> +{
> + return NULL;
> +}
Maybe should do
#define arch_ioremap arch_ioremap
here
> +#endif
> +
> +#ifndef arch_iounmap
> +static inline int arch_iounmap(void __iomem *addr)
> +{
> + return 0;
> +}
and here.
It shouldn't matter a lot because this file has inclusion guards.
However it seems tidier and perhaps other code will want to know
whether this was defined. Dunno.
Otherwise,
Acked-by: Andrew Morton <[email protected]>
Please take this patch and [2/6] and [3/6] via the appropriate arm tree.
On 5/20/22 00:22, Andrew Morton wrote:
> On Thu, 19 May 2022 16:25:50 +0800 Kefeng Wang <[email protected]> wrote:
>
>> Add special hook for architecture to verify or setup addr, size
>> or prot when ioremap() or iounmap(), which will make the generic
>> ioremap more useful.
>>
>> arch_ioremap() return a pointer,
>> - IS_ERR means return an error
>> - NULL means continue to remap
>> - a non-NULL, non-IS_ERR pointer is directly returned
>> arch_iounmap() return a int value,
>> - 0 means continue to vunmap
>> - error code means skip vunmap and return directly
>>
>> ...
>>
>> --- a/include/asm-generic/io.h
>> +++ b/include/asm-generic/io.h
>> @@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr)
>> #elif defined(CONFIG_GENERIC_IOREMAP)
>> #include <linux/pgtable.h>
>>
>> +/*
>> + * Arch code can implement the following two special hooks when using GENERIC_IOREMAP
>> + * arch_ioremap() return a pointer,
>> + * - 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 int,
>> + * - 0 means continue to vunmap
>> + * - error code means skip vunmap and return directly
>> + */
>> +#ifndef arch_ioremap
>> +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
>> +{
>> + return NULL;
>> +}
>
> Maybe should do
>
> #define arch_ioremap arch_ioremap
>
> here
>
>> +#endif
>> +
>> +#ifndef arch_iounmap
>> +static inline int arch_iounmap(void __iomem *addr)
>> +{
>> + return 0;
>> +}
>
> and here.
>
> It shouldn't matter a lot because this file has inclusion guards.
> However it seems tidier and perhaps other code will want to know
> whether this was defined. Dunno.
+1, agreed.
Hi Russell,
On Thu, May 19, 2022 at 04:25:47PM +0800, Kefeng Wang wrote:
> Since the following commits,
>
> v5.4
> commit 59d3ae9a5bf6 ("ARM: remove Intel iop33x and iop13xx support")
> v5.11
> commit 3e3f354bc383 ("ARM: remove ebsa110 platform")
>
> The runtime hook arch_iounmap() on ARM is useless, kill arch_iounmap()
> and __iounmap(), and the naming of arch_iounmap will be used in
> GENERIC_IOREMAP with the later patch.
>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kefeng Wang <[email protected]>
Probably too late for this merging window but are you ok for this patch
to go in via the arm64 tree (together with the rest of the series)?
Alternatively it could go into your patch system and hopefully land in
5.19 so that we can take the rest for 5.20.
Thanks.
--
Catalin
On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <[email protected]> wrote:
>
> Since the following commits,
>
> v5.4
> commit 59d3ae9a5bf6 ("ARM: remove Intel iop33x and iop13xx support")
> v5.11
> commit 3e3f354bc383 ("ARM: remove ebsa110 platform")
>
> The runtime hook arch_iounmap() on ARM is useless, kill arch_iounmap()
> and __iounmap(), and the naming of arch_iounmap will be used in
> GENERIC_IOREMAP with the later patch.
>
> Cc: Russell King <[email protected]>
> Signed-off-by: Kefeng Wang <[email protected]>
I had a very similar patch prototyped recently,
Reviewed-by: Arnd Bergmann <[email protected]>
It would be nice to do the same for arch_ioremap_caller(), which now
has two implementations left for mvebu and imx3, previously we had
more for iop13xx, ebsa110, ixp4xx and msm.
For both armada37x/380 and imx3, the only purpose is to override
the mtype argument, and it feels like there should be a better way
to do this, though I'm not sure what that is. Having an overridable
mtype value per 256MB section of physical address space would
be sufficient for both, but I don't know if that's any better than what
we have.
Arnd
Hi Kefeng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on arnd-asm-generic/master akpm-mm/mm-everything linus/master v5.18 next-20220523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: csky-randconfig-s032-20220522 (https://download.01.org/0day-ci/archive/20220524/[email protected]/config)
compiler: csky-linux-gcc (GCC) 11.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/intel-lab-lkp/linux/commit/eb83d30756d3b279ca58791d343dc821291818d0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853
git checkout eb83d30756d3b279ca58791d343dc821291818d0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=csky SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@
mm/ioremap.c:59:16: sparse: expected void const *addr
mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr
vim +59 mm/ioremap.c
51
52 void iounmap(volatile void __iomem *addr)
53 {
54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK);
55
56 if (arch_iounmap(vaddr))
57 return;
58
> 59 vunmap(vaddr);
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 2022/5/20 23:28, Catalin Marinas wrote:
> Hi Russell,
>
> On Thu, May 19, 2022 at 04:25:47PM +0800, Kefeng Wang wrote:
>> Since the following commits,
>>
>> v5.4
>> commit 59d3ae9a5bf6 ("ARM: remove Intel iop33x and iop13xx support")
>> v5.11
>> commit 3e3f354bc383 ("ARM: remove ebsa110 platform")
>>
>> The runtime hook arch_iounmap() on ARM is useless, kill arch_iounmap()
>> and __iounmap(), and the naming of arch_iounmap will be used in
>> GENERIC_IOREMAP with the later patch.
>>
>> Cc: Russell King <[email protected]>
>> Signed-off-by: Kefeng Wang <[email protected]>
> Probably too late for this merging window but are you ok for this patch
> to go in via the arm64 tree (together with the rest of the series)?
>
> Alternatively it could go into your patch system and hopefully land in
> 5.19 so that we can take the rest for 5.20.
Russell, should I send it to patch system or let Catalin take it?
>
> Thanks.
>
On 2022/5/24 6:17, kernel test robot wrote:
> Hi Kefeng,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on arm64/for-next/core]
> [also build test WARNING on arnd-asm-generic/master akpm-mm/mm-everything linus/master v5.18 next-20220523]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853
> base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> config: csky-randconfig-s032-20220522 (https://download.01.org/0day-ci/archive/20220524/[email protected]/config)
> compiler: csky-linux-gcc (GCC) 11.3.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # apt-get install sparse
> # sparse version: v0.6.4-dirty
> # https://github.com/intel-lab-lkp/linux/commit/eb83d30756d3b279ca58791d343dc821291818d0
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853
> git checkout eb83d30756d3b279ca58791d343dc821291818d0
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=csky SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
>
>
> sparse warnings: (new ones prefixed by >>)
>>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@
> mm/ioremap.c:59:16: sparse: expected void const *addr
> mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr
>
> vim +59 mm/ioremap.c
>
> 51
> 52 void iounmap(volatile void __iomem *addr)
> 53 {
> 54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK);
> 55
> 56 if (arch_iounmap(vaddr))
> 57 return;
> 58
> > 59 vunmap(vaddr);
1) Will add generic "arch_ioremap/arch_iounmap define"
2) and change this to vunmap((void *)vaddr);
>
On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <[email protected]> wrote:
>
> Add special hook for architecture to verify or setup addr, size
> or prot when ioremap() or iounmap(), which will make the generic
> ioremap more useful.
>
> arch_ioremap() return a pointer,
> - IS_ERR means return an error
> - NULL means continue to remap
> - a non-NULL, non-IS_ERR pointer is directly returned
> arch_iounmap() return a int value,
> - 0 means continue to vunmap
> - error code means skip vunmap and return directly
>
> Signed-off-by: Kefeng Wang <[email protected]>
I don't really like interfaces that mix error pointers and NULL pointer
returns.
Would it be possible to have a special error code other than NULL
for the fallback case?
arnd
On 2022/5/24 20:37, Arnd Bergmann wrote:
> On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <[email protected]> wrote:
>> Add special hook for architecture to verify or setup addr, size
>> or prot when ioremap() or iounmap(), which will make the generic
>> ioremap more useful.
>>
>> arch_ioremap() return a pointer,
>> - IS_ERR means return an error
>> - NULL means continue to remap
>> - a non-NULL, non-IS_ERR pointer is directly returned
>> arch_iounmap() return a int value,
>> - 0 means continue to vunmap
>> - error code means skip vunmap and return directly
>>
>> Signed-off-by: Kefeng Wang <[email protected]>
> I don't really like interfaces that mix error pointers and NULL pointer
> returns.
>
> Would it be possible to have a special error code other than NULL
> for the fallback case?
I don't find a good error code, maybe ENOTSUPP, any better suggestion?
>
> arnd
>
> .
On Tue, May 24, 2022 at 4:32 PM Kefeng Wang <[email protected]> wrote:
> On 2022/5/24 20:37, Arnd Bergmann wrote:
> > On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <[email protected]> wrote:
> >> Add special hook for architecture to verify or setup addr, size
> >> or prot when ioremap() or iounmap(), which will make the generic
> >> ioremap more useful.
> >>
> >> arch_ioremap() return a pointer,
> >> - IS_ERR means return an error
> >> - NULL means continue to remap
> >> - a non-NULL, non-IS_ERR pointer is directly returned
> >> arch_iounmap() return a int value,
> >> - 0 means continue to vunmap
> >> - error code means skip vunmap and return directly
> >>
> >> Signed-off-by: Kefeng Wang <[email protected]>
> > I don't really like interfaces that mix error pointers and NULL pointer
> > returns.
> >
> > Would it be possible to have a special error code other than NULL
> > for the fallback case?
>
> I don't find a good error code, maybe ENOTSUPP, any better suggestion?
I had another look at the resulting arm64 function, and it appears that
you never actually return a non-error pointer here. If I didn't miss anything,
I think the best way would be to change the return type to just indicate
success or an error code, and drop the case of returning the actual result,
and changing the function name accordingly.
Would that work, or do you actually require returning an __iomem
token from somewhere else?
Arnd
On Tue, May 24, 2022 at 11:48 AM Kefeng Wang <[email protected]> wrote:
> >>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@
> > mm/ioremap.c:59:16: sparse: expected void const *addr
> > mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr
> >
> > vim +59 mm/ioremap.c
> >
> > 51
> > 52 void iounmap(volatile void __iomem *addr)
> > 53 {
> > 54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK);
> > 55
> > 56 if (arch_iounmap(vaddr))
> > 57 return;
> > 58
> > > 59 vunmap(vaddr);
>
> 1) Will add generic "arch_ioremap/arch_iounmap define"
>
> 2) and change this to vunmap((void *)vaddr);
I think this need an extra __force to actually suppress the sparse
warning, as in
vunmap((void __force *)vaddr);
Using __force is usually wrong, this is one of the exceptions, so
maybe add a comment
as well.
Arnd
On 2022/5/24 20:35, Arnd Bergmann wrote:
> On Tue, May 24, 2022 at 11:48 AM Kefeng Wang <[email protected]> wrote:
>>>>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@
>>> mm/ioremap.c:59:16: sparse: expected void const *addr
>>> mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr
>>>
>>> vim +59 mm/ioremap.c
>>>
>>> 51
>>> 52 void iounmap(volatile void __iomem *addr)
>>> 53 {
>>> 54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK);
>>> 55
>>> 56 if (arch_iounmap(vaddr))
>>> 57 return;
>>> 58
>>> > 59 vunmap(vaddr);
>> 1) Will add generic "arch_ioremap/arch_iounmap define"
>>
>> 2) and change this to vunmap((void *)vaddr);
> I think this need an extra __force to actually suppress the sparse
> warning, as in
>
> vunmap((void __force *)vaddr);
>
> Using __force is usually wrong, this is one of the exceptions, so
> maybe add a comment
> as well.
Right, I found this too， and using ___force in local, will update, thank.
>
> Arnd
>
> .
On 2022/5/24 22:47, Arnd Bergmann wrote:
> On Tue, May 24, 2022 at 4:32 PM Kefeng Wang <[email protected]> wrote:
>> On 2022/5/24 20:37, Arnd Bergmann wrote:
>>> On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <[email protected]> wrote:
>>>> Add special hook for architecture to verify or setup addr, size
>>>> or prot when ioremap() or iounmap(), which will make the generic
>>>> ioremap more useful.
>>>>
>>>> arch_ioremap() return a pointer,
>>>> - IS_ERR means return an error
>>>> - NULL means continue to remap
>>>> - a non-NULL, non-IS_ERR pointer is directly returned
>>>> arch_iounmap() return a int value,
>>>> - 0 means continue to vunmap
>>>> - error code means skip vunmap and return directly
>>>>
>>>> Signed-off-by: Kefeng Wang <[email protected]>
>>> I don't really like interfaces that mix error pointers and NULL pointer
>>> returns.
>>>
>>> Would it be possible to have a special error code other than NULL
>>> for the fallback case?
>> I don't find a good error code, maybe ENOTSUPP, any better suggestion?
> I had another look at the resulting arm64 function, and it appears that
> you never actually return a non-error pointer here. If I didn't miss anything,
> I think the best way would be to change the return type to just indicate
> success or an error code, and drop the case of returning the actual result,
> and changing the function name accordingly.
>
> Would that work, or do you actually require returning an __iomem
> token from somewhere else?
Christoph suggested in the first version,
https://lore.kernel.org/linux-arm-kernel/Ymq2uX%[email protected]/
> Arnd
>
> .
On Tue, May 24, 2022 at 10:53:54PM +0800, Kefeng Wang wrote:
> Christoph? suggested in the first version,
>
> https://lore.kernel.org/linux-arm-kernel/Ymq2uX%[email protected]/
Yeah, mostly to eventually also covers mips. But if this causes
problems now please just dumb it down and we can revisit the interface
when actually needed.