Re: [PATCH v2] MIPS: replace add_memory_region with memblock

[Serge Semin]

Hello Thomas

On Thu, Oct 08, 2020 at 10:43:54AM +0200, Thomas Bogendoerfer wrote:
> add_memory_region was the old interface for registering memory and
> was already changed to used memblock internaly. Replace it by
> directly calling memblock functions.

Thanks for suggesting this cleanup. It's great to see that the leftover of the
old bootmem and MIPS-specific boot_mem_map framework time is going to be finally
removed. A few comments are blow.

> 
> Signed-off-by: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> 	fixed missing memblock include in fw/sni/sniprom.c
> 	tested on cobalt, IP22, IP28, IP30, IP32, JAZZ, SNI

...

> diff --git a/arch/mips/kernel/prom.c b/arch/mips/kernel/prom.c
> index 9e50dc8df2f6..fab532cb5a11 100644
> --- a/arch/mips/kernel/prom.c
> +++ b/arch/mips/kernel/prom.c
> @@ -50,14 +50,18 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>  		size = PHYS_ADDR_MAX - base;
>  	}
>  
> -	add_memory_region(base, size, BOOT_MEM_RAM);
> +	memblock_add(base, size);
>  }
>  
>  int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
>  					phys_addr_t size, bool nomap)
>  {
> -	add_memory_region(base, size,
> -			  nomap ? BOOT_MEM_NOMAP : BOOT_MEM_RESERVED);
> +	if (nomap) {
> +		memblock_remove(base, size);
> +	} else {
> +		memblock_add(base, size);
> +		memblock_reserve(base, size);
> +	}
>  
>  	return 0;
>  }

I guess originally the arch-specific early_init_dt_add_memory_arch() and
early_init_dt_reserve_memory_arch() methods have been added since MIPS's got its
own memory types declarations (BOOT_MEM_RAM, BOOT_MEM_RESERVED, etc) and had had
a specific internal system memory regions mapping (add_memory_region(),
boot_mem_map, etc). Since the leftover of that framework is going to be removed,
we can freely use the standard early_init_dt_add_memory_arch() and
early_init_dt_reserve_memory_arch() implementations from drivers/of/fdt.c:1102
drivers/of/fdt.c:1149 .

At least I don't see a decent reason to preserve them. The memory registration
method does nearly the same sanity checks. The memory reservation function
defers a bit in adding the being reserved memory first. That seems redundant,
since the reserved memory won't be available for the system anyway. Do I miss
something?

> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 4c04a86f075b..fb05b66e111f 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -91,45 +91,6 @@ unsigned long ARCH_PFN_OFFSET;
>  EXPORT_SYMBOL(ARCH_PFN_OFFSET);
>  #endif
>  
> -void __init add_memory_region(phys_addr_t start, phys_addr_t size, long type)
> -{
> -	/*
> -	 * Note: This function only exists for historical reason,
> -	 * new code should use memblock_add or memblock_add_node instead.
> -	 */
> -
> -	/*
> -	 * If the region reaches the top of the physical address space, adjust
> -	 * the size slightly so that (start + size) doesn't overflow
> -	 */
> -	if (start + size - 1 == PHYS_ADDR_MAX)
> -		--size;
> -
> -	/* Sanity check */
> -	if (start + size < start) {
> -		pr_warn("Trying to add an invalid memory region, skipped\n");
> -		return;
> -	}
> -
> -	if (start < PHYS_OFFSET)
> -		return;
> -
> -	memblock_add(start, size);
> -	/* Reserve any memory except the ordinary RAM ranges. */
> -	switch (type) {
> -	case BOOT_MEM_RAM:
> -		break;
> -
> -	case BOOT_MEM_NOMAP: /* Discard the range from the system. */
> -		memblock_remove(start, size);
> -		break;
> -
> -	default: /* Reserve the rest of the memory types at boot time */
> -		memblock_reserve(start, size);
> -		break;
> -	}
> -}
> -
>  void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_addr_t sz_max)
>  {
>  	void *dm = &detect_magic;
> @@ -146,7 +107,7 @@ void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_add
>  		((unsigned long long) sz_min) / SZ_1M,
>  		((unsigned long long) sz_max) / SZ_1M);
>  
> -	add_memory_region(start, size, BOOT_MEM_RAM);
> +	memblock_add(start, size);
>  }
>  
>  /*
> @@ -400,7 +361,7 @@ static int __init early_parse_mem(char *p)
>  	if (*p == '@')
>  		start = memparse(p + 1, &p);
>  
> -	add_memory_region(start, size, BOOT_MEM_RAM);
> +	memblock_add(start, size);
>  
>  	return 0;
>  }
> @@ -426,13 +387,14 @@ static int __init early_parse_memmap(char *p)
>  
>  	if (*p == '@') {
>  		start_at = memparse(p+1, &p);
> -		add_memory_region(start_at, mem_size, BOOT_MEM_RAM);
> +		memblock_add(start_at, mem_size);
>  	} else if (*p == '#') {
>  		pr_err("\"memmap=nn#ss\" (force ACPI data) invalid on MIPS\n");
>  		return -EINVAL;
>  	} else if (*p == '$') {
>  		start_at = memparse(p+1, &p);

> -		add_memory_region(start_at, mem_size, BOOT_MEM_RESERVED);
> +		memblock_add(start_at, mem_size);
> +		memblock_reserve(start_at, mem_size);

I suppose we could remove the memory addition from here too. What do you think?

>  	} else {
>  		pr_err("\"memmap\" invalid format!\n");
>  		return -EINVAL;
> @@ -644,7 +606,7 @@ static void __init bootcmdline_init(void)
>   * arch_mem_init - initialize memory management subsystem
>   *
>   *  o plat_mem_setup() detects the memory configuration and will record detected
> - *    memory areas using add_memory_region.
> + *    memory areas using memblock_add.
>   *
>   * At this stage the memory configuration of the system is known to the
>   * kernel but generic memory management system is still entirely uninitialized.
> diff --git a/arch/mips/loongson2ef/common/mem.c b/arch/mips/loongson2ef/common/mem.c
> index ae21f1c62baa..057d58bb470e 100644
> --- a/arch/mips/loongson2ef/common/mem.c
> +++ b/arch/mips/loongson2ef/common/mem.c
> @@ -17,10 +17,7 @@ u32 memsize, highmemsize;
>  
>  void __init prom_init_memory(void)
>  {

> -	add_memory_region(0x0, (memsize << 20), BOOT_MEM_RAM);
> -
> -	add_memory_region(memsize << 20, LOONGSON_PCI_MEM_START - (memsize <<
> -				20), BOOT_MEM_RESERVED);
> +	memblock_add(0x0, (memsize << 20));

Hm, am I missing something or the BOOT_MEM_RESERVED part has been discarded?

>  
>  #ifdef CONFIG_CPU_SUPPORTS_ADDRWINCFG
>  	{
> @@ -41,12 +38,7 @@ void __init prom_init_memory(void)
>  
>  #ifdef CONFIG_64BIT
>  	if (highmemsize > 0)

> -		add_memory_region(LOONGSON_HIGHMEM_START,
> -				  highmemsize << 20, BOOT_MEM_RAM);
> -
> -	add_memory_region(LOONGSON_PCI_MEM_END + 1, LOONGSON_HIGHMEM_START -
> -			  LOONGSON_PCI_MEM_END - 1, BOOT_MEM_RESERVED);
> -
> +		memblock_add(LOONGSON_HIGHMEM_START, highmemsize << 20);

The same question. Is it ok to discard the
[LOONGSON_PCI_MEM_END+1:LOONGSON_HIGHMEM_START-LOONGSON_PCI_MEM_END-1] region
removal operation?

-Sergey

>  #endif /* !CONFIG_64BIT */
>  }
>