[PATCH v2] powerpc: drop page_is_ram() and walk_system_ram_range()

[PATCH v2] powerpc: drop page_is_ram() and walk_system_ram_range()

[Christophe Leroy]

Since commit c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
it is possible to use the generic walk_system_ram_range() and
the generic page_is_ram().

To enable the use of walk_system_ram_range() by the IBM EHEA
ethernet driver, the generic function has to be exported.

As powerpc was the only (last?) user of CONFIG_ARCH_HAS_WALK_MEMORY,
the #ifdef around the generic walk_system_ram_range() has become
useless and can be dropped.

Fixes: c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/Kconfig            |  3 ---
 arch/powerpc/include/asm/page.h |  1 -
 arch/powerpc/mm/mem.c           | 33 ---------------------------------
 kernel/resource.c               |  5 +----
 4 files changed, 1 insertion(+), 41 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2890d36eb531..f92e6754edf1 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -478,9 +478,6 @@ config ARCH_CPU_PROBE_RELEASE
 config ARCH_ENABLE_MEMORY_HOTPLUG
 	def_bool y
 
-config ARCH_HAS_WALK_MEMORY
-	def_bool y
-
 config ARCH_ENABLE_MEMORY_HOTREMOVE
 	def_bool y
 
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 5c5ea2413413..aa4497175bd3 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -326,7 +326,6 @@ struct page;
 extern void clear_user_page(void *page, unsigned long vaddr, struct page *pg);
 extern void copy_user_page(void *to, void *from, unsigned long vaddr,
 		struct page *p);
-extern int page_is_ram(unsigned long pfn);
 extern int devmem_is_allowed(unsigned long pfn);
 
 #ifdef CONFIG_PPC_SMLPAR
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 33cc6f676fa6..fa9916c2c662 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -80,11 +80,6 @@ static inline pte_t *virt_to_kpte(unsigned long vaddr)
 #define TOP_ZONE ZONE_NORMAL
 #endif
 
-int page_is_ram(unsigned long pfn)
-{
-	return memblock_is_memory(__pfn_to_phys(pfn));
-}
-
 pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 			      unsigned long size, pgprot_t vma_prot)
 {
@@ -176,34 +171,6 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
 #endif
 #endif /* CONFIG_MEMORY_HOTPLUG */
 
-/*
- * walk_memory_resource() needs to make sure there is no holes in a given
- * memory range.  PPC64 does not maintain the memory layout in /proc/iomem.
- * Instead it maintains it in memblock.memory structures.  Walk through the
- * memory regions, find holes and callback for contiguous regions.
- */
-int
-walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
-		void *arg, int (*func)(unsigned long, unsigned long, void *))
-{
-	struct memblock_region *reg;
-	unsigned long end_pfn = start_pfn + nr_pages;
-	unsigned long tstart, tend;
-	int ret = -1;
-
-	for_each_memblock(memory, reg) {
-		tstart = max(start_pfn, memblock_region_memory_base_pfn(reg));
-		tend = min(end_pfn, memblock_region_memory_end_pfn(reg));
-		if (tstart >= tend)
-			continue;
-		ret = (*func)(tstart, tend - tstart, arg);
-		if (ret)
-			break;
-	}
-	return ret;
-}
-EXPORT_SYMBOL_GPL(walk_system_ram_range);
-
 #ifndef CONFIG_NEED_MULTIPLE_NODES
 void __init mem_topology_setup(void)
 {
diff --git a/kernel/resource.c b/kernel/resource.c
index 915c02e8e5dd..2e1636041508 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -448,8 +448,6 @@ int walk_mem_res(u64 start, u64 end, void *arg,
 				     arg, func);
 }
 
-#if !defined(CONFIG_ARCH_HAS_WALK_MEMORY)
-
 /*
  * This function calls the @func callback against all memory ranges of type
  * System RAM which are marked as IORESOURCE_SYSTEM_RAM and IORESOUCE_BUSY.
@@ -480,8 +478,7 @@ int walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
 	}
 	return ret;
 }
-
-#endif
+EXPORT_SYMBOL_GPL(walk_system_ram_range);
 
 static int __is_ram(unsigned long pfn, unsigned long nr_pages, void *arg)
 {
-- 
2.13.3

Re: [PATCH v2] powerpc: drop page_is_ram() and walk_system_ram_range()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:

> Since commit c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
> it is possible to use the generic walk_system_ram_range() and
> the generic page_is_ram().
>
> To enable the use of walk_system_ram_range() by the IBM EHEA
> ethernet driver, the generic function has to be exported.

I'm not sure if we have a policy on that, but I suspect we'd rather not
add a new export on all arches unless we need to. Especially seeing as
the only user is the EHEA code which is heavily in maintenance mode.

I'll put the export in powerpc code and make sure that builds.

> As powerpc was the only (last?) user of CONFIG_ARCH_HAS_WALK_MEMORY,
> the #ifdef around the generic walk_system_ram_range() has become
> useless and can be dropped.

Yes it was the only user:

a99824f327c7 ("[POWERPC] Add arch-specific walk_memory_remove() for 64-bit powerpc")

I'll update the changelog.

cheers


> Fixes: c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  arch/powerpc/Kconfig            |  3 ---
>  arch/powerpc/include/asm/page.h |  1 -
>  arch/powerpc/mm/mem.c           | 33 ---------------------------------
>  kernel/resource.c               |  5 +----
>  4 files changed, 1 insertion(+), 41 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 2890d36eb531..f92e6754edf1 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -478,9 +478,6 @@ config ARCH_CPU_PROBE_RELEASE
>  config ARCH_ENABLE_MEMORY_HOTPLUG
>  	def_bool y
>  
> -config ARCH_HAS_WALK_MEMORY
> -	def_bool y
> -
>  config ARCH_ENABLE_MEMORY_HOTREMOVE
>  	def_bool y
>  
> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
> index 5c5ea2413413..aa4497175bd3 100644
> --- a/arch/powerpc/include/asm/page.h
> +++ b/arch/powerpc/include/asm/page.h
> @@ -326,7 +326,6 @@ struct page;
>  extern void clear_user_page(void *page, unsigned long vaddr, struct page *pg);
>  extern void copy_user_page(void *to, void *from, unsigned long vaddr,
>  		struct page *p);
> -extern int page_is_ram(unsigned long pfn);
>  extern int devmem_is_allowed(unsigned long pfn);
>  
>  #ifdef CONFIG_PPC_SMLPAR
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 33cc6f676fa6..fa9916c2c662 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -80,11 +80,6 @@ static inline pte_t *virt_to_kpte(unsigned long vaddr)
>  #define TOP_ZONE ZONE_NORMAL
>  #endif
>  
> -int page_is_ram(unsigned long pfn)
> -{
> -	return memblock_is_memory(__pfn_to_phys(pfn));
> -}
> -
>  pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  			      unsigned long size, pgprot_t vma_prot)
>  {
> @@ -176,34 +171,6 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
>  #endif
>  #endif /* CONFIG_MEMORY_HOTPLUG */
>  
> -/*
> - * walk_memory_resource() needs to make sure there is no holes in a given
> - * memory range.  PPC64 does not maintain the memory layout in /proc/iomem.
> - * Instead it maintains it in memblock.memory structures.  Walk through the
> - * memory regions, find holes and callback for contiguous regions.
> - */
> -int
> -walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
> -		void *arg, int (*func)(unsigned long, unsigned long, void *))
> -{
> -	struct memblock_region *reg;
> -	unsigned long end_pfn = start_pfn + nr_pages;
> -	unsigned long tstart, tend;
> -	int ret = -1;
> -
> -	for_each_memblock(memory, reg) {
> -		tstart = max(start_pfn, memblock_region_memory_base_pfn(reg));
> -		tend = min(end_pfn, memblock_region_memory_end_pfn(reg));
> -		if (tstart >= tend)
> -			continue;
> -		ret = (*func)(tstart, tend - tstart, arg);
> -		if (ret)
> -			break;
> -	}
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(walk_system_ram_range);
> -
>  #ifndef CONFIG_NEED_MULTIPLE_NODES
>  void __init mem_topology_setup(void)
>  {
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 915c02e8e5dd..2e1636041508 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -448,8 +448,6 @@ int walk_mem_res(u64 start, u64 end, void *arg,
>  				     arg, func);
>  }
>  
> -#if !defined(CONFIG_ARCH_HAS_WALK_MEMORY)
> -
>  /*
>   * This function calls the @func callback against all memory ranges of type
>   * System RAM which are marked as IORESOURCE_SYSTEM_RAM and IORESOUCE_BUSY.
> @@ -480,8 +478,7 @@ int walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
>  	}
>  	return ret;
>  }
> -
> -#endif
> +EXPORT_SYMBOL_GPL(walk_system_ram_range);
>  
>  static int __is_ram(unsigned long pfn, unsigned long nr_pages, void *arg)
>  {
> -- 
> 2.13.3

Re: [PATCH v2] powerpc: drop page_is_ram() and walk_system_ram_range()

[Christophe Leroy]

Le 04/02/2019 à 11:24, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> 
>> Since commit c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
>> it is possible to use the generic walk_system_ram_range() and
>> the generic page_is_ram().
>>
>> To enable the use of walk_system_ram_range() by the IBM EHEA
>> ethernet driver, the generic function has to be exported.
> 
> I'm not sure if we have a policy on that, but I suspect we'd rather not
> add a new export on all arches unless we need to. Especially seeing as
> the only user is the EHEA code which is heavily in maintenance mode.

If you take the exemple of function walk_iomem_res_desc(), that's 
similar. It is only used by x86 it seems and exported for nvdimm/e820 
driver only.

See commit d76401ade0bb6ab0a7 ("libnvdimm, e820: Register all pmem 
resources")

> 
> I'll put the export in powerpc code and make sure that builds.

I thought there was a rule that EXPORT_SYMBOL has to immediately follow 
the function it exports. At least checkpatch checks for that.

Christophe

> 
>> As powerpc was the only (last?) user of CONFIG_ARCH_HAS_WALK_MEMORY,
>> the #ifdef around the generic walk_system_ram_range() has become
>> useless and can be dropped.
> 
> Yes it was the only user:
> 
> a99824f327c7 ("[POWERPC] Add arch-specific walk_memory_remove() for 64-bit powerpc")
> 
> I'll update the changelog.
> 
> cheers
> 
> 
>> Fixes: c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   arch/powerpc/Kconfig            |  3 ---
>>   arch/powerpc/include/asm/page.h |  1 -
>>   arch/powerpc/mm/mem.c           | 33 ---------------------------------
>>   kernel/resource.c               |  5 +----
>>   4 files changed, 1 insertion(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 2890d36eb531..f92e6754edf1 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -478,9 +478,6 @@ config ARCH_CPU_PROBE_RELEASE
>>   config ARCH_ENABLE_MEMORY_HOTPLUG
>>   	def_bool y
>>   
>> -config ARCH_HAS_WALK_MEMORY
>> -	def_bool y
>> -
>>   config ARCH_ENABLE_MEMORY_HOTREMOVE
>>   	def_bool y
>>   
>> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
>> index 5c5ea2413413..aa4497175bd3 100644
>> --- a/arch/powerpc/include/asm/page.h
>> +++ b/arch/powerpc/include/asm/page.h
>> @@ -326,7 +326,6 @@ struct page;
>>   extern void clear_user_page(void *page, unsigned long vaddr, struct page *pg);
>>   extern void copy_user_page(void *to, void *from, unsigned long vaddr,
>>   		struct page *p);
>> -extern int page_is_ram(unsigned long pfn);
>>   extern int devmem_is_allowed(unsigned long pfn);
>>   
>>   #ifdef CONFIG_PPC_SMLPAR
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 33cc6f676fa6..fa9916c2c662 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -80,11 +80,6 @@ static inline pte_t *virt_to_kpte(unsigned long vaddr)
>>   #define TOP_ZONE ZONE_NORMAL
>>   #endif
>>   
>> -int page_is_ram(unsigned long pfn)
>> -{
>> -	return memblock_is_memory(__pfn_to_phys(pfn));
>> -}
>> -
>>   pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>>   			      unsigned long size, pgprot_t vma_prot)
>>   {
>> @@ -176,34 +171,6 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
>>   #endif
>>   #endif /* CONFIG_MEMORY_HOTPLUG */
>>   
>> -/*
>> - * walk_memory_resource() needs to make sure there is no holes in a given
>> - * memory range.  PPC64 does not maintain the memory layout in /proc/iomem.
>> - * Instead it maintains it in memblock.memory structures.  Walk through the
>> - * memory regions, find holes and callback for contiguous regions.
>> - */
>> -int
>> -walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
>> -		void *arg, int (*func)(unsigned long, unsigned long, void *))
>> -{
>> -	struct memblock_region *reg;
>> -	unsigned long end_pfn = start_pfn + nr_pages;
>> -	unsigned long tstart, tend;
>> -	int ret = -1;
>> -
>> -	for_each_memblock(memory, reg) {
>> -		tstart = max(start_pfn, memblock_region_memory_base_pfn(reg));
>> -		tend = min(end_pfn, memblock_region_memory_end_pfn(reg));
>> -		if (tstart >= tend)
>> -			continue;
>> -		ret = (*func)(tstart, tend - tstart, arg);
>> -		if (ret)
>> -			break;
>> -	}
>> -	return ret;
>> -}
>> -EXPORT_SYMBOL_GPL(walk_system_ram_range);
>> -
>>   #ifndef CONFIG_NEED_MULTIPLE_NODES
>>   void __init mem_topology_setup(void)
>>   {
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 915c02e8e5dd..2e1636041508 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -448,8 +448,6 @@ int walk_mem_res(u64 start, u64 end, void *arg,
>>   				     arg, func);
>>   }
>>   
>> -#if !defined(CONFIG_ARCH_HAS_WALK_MEMORY)
>> -
>>   /*
>>    * This function calls the @func callback against all memory ranges of type
>>    * System RAM which are marked as IORESOURCE_SYSTEM_RAM and IORESOUCE_BUSY.
>> @@ -480,8 +478,7 @@ int walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
>>   	}
>>   	return ret;
>>   }
>> -
>> -#endif
>> +EXPORT_SYMBOL_GPL(walk_system_ram_range);
>>   
>>   static int __is_ram(unsigned long pfn, unsigned long nr_pages, void *arg)
>>   {
>> -- 
>> 2.13.3

Re: [PATCH v2] powerpc: drop page_is_ram() and walk_system_ram_range()

[Michael Ellerman]

Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 04/02/2019 à 11:24, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> 
>>> Since commit c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
>>> it is possible to use the generic walk_system_ram_range() and
>>> the generic page_is_ram().
>>>
>>> To enable the use of walk_system_ram_range() by the IBM EHEA
>>> ethernet driver, the generic function has to be exported.
>> 
>> I'm not sure if we have a policy on that, but I suspect we'd rather not
>> add a new export on all arches unless we need to. Especially seeing as
>> the only user is the EHEA code which is heavily in maintenance mode.
>
> If you take the exemple of function walk_iomem_res_desc(), that's 
> similar. It is only used by x86 it seems and exported for nvdimm/e820 
> driver only.
>
> See commit d76401ade0bb6ab0a7 ("libnvdimm, e820: Register all pmem 
> resources")

OK. Which begs the question whether we need both exported. It looks like
you could probably use walk_iomem_res_desc() with the right flags to do
the same thing as walk_system_ram_range().

>> I'll put the export in powerpc code and make sure that builds.
>
> I thought there was a rule that EXPORT_SYMBOL has to immediately follow 
> the function it exports. At least checkpatch checks for that.

Yeah that is a rule. But rules are made to be broken :)

I'll merge it for now with the export in powerpc code, if we want to we
can do a separate patch to move that export into generic code and get
acks for that.

cheers

Re: [v2] powerpc: drop page_is_ram() and walk_system_ram_range()

[Michael Ellerman]

On Fri, 2019-02-01 at 10:46:52 UTC, Christophe Leroy wrote:
> Since commit c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
> it is possible to use the generic walk_system_ram_range() and
> the generic page_is_ram().
> 
> To enable the use of walk_system_ram_range() by the IBM EHEA
> ethernet driver, the generic function has to be exported.
> 
> As powerpc was the only (last?) user of CONFIG_ARCH_HAS_WALK_MEMORY,
> the #ifdef around the generic walk_system_ram_range() has become
> useless and can be dropped.
> 
> Fixes: c40dd2f76644 ("powerpc: Add System RAM to /proc/iomem")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/26b523356f49a0117c8f9e32ca98aa6d

cheers