Message-ID: <YaVp7ubpCLN1xWfF@robh.at.kernel.org>
Date:   Mon, 29 Nov 2021 18:01:50 -0600
From:   Rob Herring <robh@...nel.org>
To:     Calvin Zhang <calvinzhang.cool@...il.com>
Cc:     Vineet Gupta <vgupta@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Guo Ren <guoren@...nel.org>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Nick Hu <nickhu@...estech.com>,
        Greentime Hu <green.hu@...il.com>,
        Vincent Chen <deanbo422@...il.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Jonas Bonn <jonas@...thpole.se>,
        Stefan Kristiansson <stefan.kristiansson@...nalahti.fi>,
        Stafford Horne <shorne@...il.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Rich Felker <dalias@...c.org>, Chris Zankel <chris@...kel.net>,
        Max Filippov <jcmvbkbc@...il.com>,
        Frank Rowand <frowand.list@...il.com>,
        Mike Rapoport <rppt@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Vladimir Isaev <isaev@...opsys.com>,
        Arnd Bergmann <arnd@...db.de>,
        "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Marc Zyngier <maz@...nel.org>,
        David Brazdil <dbrazdil@...gle.com>,
        Mark Rutland <mark.rutland@....com>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Souptick Joarder <jrdr.linux@...il.com>,
        Jinyang He <hejinyang@...ngson.cn>,
        Alexander Sverdlin <alexander.sverdlin@...ia.com>,
        Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Ley Foon Tan <ley.foon.tan@...el.com>,
        Andreas Oetken <andreas.oetken@...mens.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Christophe Leroy <christophe.leroy@....fr>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Zhang Yunkai <zhang.yunkai@....com.cn>,
        Markus Elfring <elfring@...rs.sourceforge.net>,
        Ganesh Goudar <ganeshgr@...ux.ibm.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Atish Patra <atish.patra@....com>,
        Anup Patel <anup.patel@....com>,
        Nick Kossifidis <mick@....forth.gr>,
        Alexandre Ghiti <alex@...ti.fr>,
        Vitaly Wool <vitaly.wool@...sulko.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Guo Ren <guoren@...ux.alibaba.com>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Mauri Sandberg <sandberg@...lfence.com>,
        Palmer Dabbelt <palmerdabbelt@...gle.com>,
        linux-snps-arc@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-csky@...r.kernel.org,
        uclinux-h8-devel@...ts.sourceforge.jp, linux-mips@...r.kernel.org,
        openrisc@...ts.librecores.org, linuxppc-dev@...ts.ozlabs.org,
        linux-riscv@...ts.infradead.org, linux-sh@...r.kernel.org,
        linux-xtensa@...ux-xtensa.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] of: Sort reserved_mem related code

On Fri, Nov 19, 2021 at 03:58:18PM +0800, Calvin Zhang wrote:
> Move code about parsing /reserved-memory and initializing of
> reserved_mems array to of_reserved_mem.c for better modularity.
> 
> Rename array name from reserved_mem to reserved_mems to distinguish
> from type definition.
> 
> Signed-off-by: Calvin Zhang <calvinzhang.cool@...il.com>
> ---
>  drivers/of/fdt.c                | 108 +--------------------
>  drivers/of/of_private.h         |  12 ++-
>  drivers/of/of_reserved_mem.c    | 163 ++++++++++++++++++++++++++------
>  include/linux/of_reserved_mem.h |   4 +
>  4 files changed, 149 insertions(+), 138 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bdca35284ceb..445af4e69300 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -80,7 +80,7 @@ void __init of_fdt_limit_memory(int limit)
>  	}
>  }
>  
> -static bool of_fdt_device_is_available(const void *blob, unsigned long node)
> +bool of_fdt_device_is_available(const void *blob, unsigned long node)
>  {
>  	const char *status = fdt_getprop(blob, node, "status", NULL);
>  
> @@ -476,7 +476,7 @@ void *initial_boot_params __ro_after_init;
>  
>  static u32 of_fdt_crc32;
>  
> -static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
> +int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
>  					phys_addr_t size, bool nomap)

I think you can move this function too if you change the nomap==false 
callers to just call memblock_reserve directly.


>  {
>  	if (nomap) {
> @@ -492,108 +492,6 @@ static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
>  	return memblock_reserve(base, size);
>  }
>  
> -/*
> - * __reserved_mem_reserve_reg() - reserve all memory described in 'reg' property
> - */
> -static int __init __reserved_mem_reserve_reg(unsigned long node,
> -					     const char *uname)
> -{
> -	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> -	phys_addr_t base, size;
> -	int len;
> -	const __be32 *prop;
> -	int first = 1;
> -	bool nomap;
> -
> -	prop = of_get_flat_dt_prop(node, "reg", &len);
> -	if (!prop)
> -		return -ENOENT;
> -
> -	if (len && len % t_len != 0) {
> -		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
> -		       uname);
> -		return -EINVAL;
> -	}
> -
> -	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> -
> -	while (len >= t_len) {
> -		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> -		size = dt_mem_next_cell(dt_root_size_cells, &prop);
> -
> -		if (size &&
> -		    early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
> -			pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
> -				uname, &base, (unsigned long)(size / SZ_1M));
> -		else
> -			pr_info("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
> -				uname, &base, (unsigned long)(size / SZ_1M));
> -
> -		len -= t_len;
> -		if (first) {
> -			fdt_reserved_mem_save_node(node, uname, base, size);
> -			first = 0;
> -		}
> -	}
> -	return 0;
> -}
> -
> -/*
> - * __reserved_mem_check_root() - check if #size-cells, #address-cells provided
> - * in /reserved-memory matches the values supported by the current implementation,
> - * also check if ranges property has been provided
> - */
> -static int __init __reserved_mem_check_root(unsigned long node)
> -{
> -	const __be32 *prop;
> -
> -	prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
> -	if (!prop || be32_to_cpup(prop) != dt_root_size_cells)
> -		return -EINVAL;
> -
> -	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
> -	if (!prop || be32_to_cpup(prop) != dt_root_addr_cells)
> -		return -EINVAL;
> -
> -	prop = of_get_flat_dt_prop(node, "ranges", NULL);
> -	if (!prop)
> -		return -EINVAL;
> -	return 0;
> -}
> -
> -/*
> - * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
> - */
> -static int __init fdt_scan_reserved_mem(void)
> -{
> -	int node, child;
> -	const void *fdt = initial_boot_params;
> -
> -	node = fdt_path_offset(fdt, "/reserved-memory");
> -	if (node < 0)
> -		return -ENODEV;
> -
> -	if (__reserved_mem_check_root(node) != 0) {
> -		pr_err("Reserved memory: unsupported node format, ignoring\n");
> -		return -EINVAL;
> -	}
> -
> -	fdt_for_each_subnode(child, fdt, node) {
> -		const char *uname;
> -		int err;
> -
> -		if (!of_fdt_device_is_available(fdt, child))
> -			continue;
> -
> -		uname = fdt_get_name(fdt, child, NULL);
> -
> -		err = __reserved_mem_reserve_reg(child, uname);
> -		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL))
> -			fdt_reserved_mem_save_node(child, uname, 0, 0);
> -	}
> -	return 0;
> -}
> -
>  /*
>   * fdt_reserve_elfcorehdr() - reserves memory for elf core header
>   *
> @@ -642,7 +540,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
>  	}
>  
>  	fdt_scan_reserved_mem();
> -	fdt_init_reserved_mem();
> +	of_reserved_mem_init();
>  	fdt_reserve_elfcorehdr();
>  }
>  
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 9324483397f6..88b67f8ed698 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -163,8 +163,14 @@ static inline int of_dma_get_range(struct device_node *np,
>  }
>  #endif
>  
> -void fdt_init_reserved_mem(void);
> -void fdt_reserved_mem_save_node(unsigned long node, const char *uname,
> -			       phys_addr_t base, phys_addr_t size);
> +bool of_fdt_device_is_available(const void *blob, unsigned long node);
> +int early_init_dt_reserve_memory_arch(phys_addr_t base,
> +					phys_addr_t size, bool nomap);
> +#ifdef CONFIG_OF_RESERVED_MEM
> +int fdt_scan_reserved_mem(void);
> +#else
> +static inline int fdt_scan_reserved_mem(void) { }
> +#endif
> +
>  
>  #endif /* _LINUX_OF_PRIVATE_H */
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 9c0fb962c22b..784cfc5cd251 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -20,13 +20,14 @@
>  #include <linux/of_reserved_mem.h>
>  #include <linux/sort.h>
>  #include <linux/slab.h>
> +#include <linux/libfdt.h>
>  #include <linux/memblock.h>
>  #include <linux/kmemleak.h>
>  
>  #include "of_private.h"
>  
>  #define MAX_RESERVED_REGIONS	64
> -static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> +static struct reserved_mem reserved_mems[MAX_RESERVED_REGIONS];

Would be a bit easier to review without the rename.


>  static int reserved_mem_count;
>  
>  static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
> @@ -56,12 +57,12 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
>  /*
>   * fdt_reserved_mem_save_node() - save fdt node for second pass initialization
>   */
> -void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
> +static void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
>  				      phys_addr_t base, phys_addr_t size)
>  {
> -	struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
> +	struct reserved_mem *rmem = &reserved_mems[reserved_mem_count];
>  
> -	if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) {
> +	if (reserved_mem_count == ARRAY_SIZE(reserved_mems)) {
>  		pr_err("not enough space for all defined regions.\n");
>  		return;
>  	}
> @@ -173,29 +174,105 @@ static const struct of_device_id __rmem_of_table_sentinel
>  	__used __section("__reservedmem_of_table_end");
>  
>  /*
> - * __reserved_mem_init_node() - call region specific reserved memory init code
> + * __reserved_mem_check_root() - check if #size-cells, #address-cells provided
> + * in /reserved-memory matches the values supported by the current implementation,
> + * also check if ranges property has been provided
>   */
> -static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> +static int __init __reserved_mem_check_root(unsigned long node)
>  {
> -	extern const struct of_device_id __reservedmem_of_table[];
> -	const struct of_device_id *i;
> -	int ret = -ENOENT;
> +	const __be32 *prop;
>  
> -	for (i = __reservedmem_of_table; i < &__rmem_of_table_sentinel; i++) {
> -		reservedmem_of_init_fn initfn = i->data;
> -		const char *compat = i->compatible;
> +	prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
> +	if (!prop || be32_to_cpup(prop) != dt_root_size_cells)
> +		return -EINVAL;
>  
> -		if (!of_flat_dt_is_compatible(rmem->fdt_node, compat))
> -			continue;
> +	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
> +	if (!prop || be32_to_cpup(prop) != dt_root_addr_cells)
> +		return -EINVAL;
>  
> -		ret = initfn(rmem);
> -		if (ret == 0) {
> -			pr_info("initialized node %s, compatible id %s\n",
> -				rmem->name, compat);
> -			break;
> +	prop = of_get_flat_dt_prop(node, "ranges", NULL);
> +	if (!prop)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +/*
> + * __reserved_mem_reserve_reg() - reserve all memory described in 'reg' property
> + */
> +static int __init __reserved_mem_reserve_reg(unsigned long node,
> +					     const char *uname)
> +{
> +	int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +	phys_addr_t base, size;
> +	int len;
> +	const __be32 *prop;
> +	int first = 1;
> +	bool nomap;
> +
> +	prop = of_get_flat_dt_prop(node, "reg", &len);
> +	if (!prop)
> +		return -ENOENT;
> +
> +	if (len && len % t_len != 0) {
> +		pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
> +		       uname);
> +		return -EINVAL;
> +	}
> +
> +	nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> +
> +	while (len >= t_len) {
> +		base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +		size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +
> +		if (size &&
> +		    early_init_dt_reserve_memory_arch(base, size, nomap) == 0)
> +			pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
> +				uname, &base, (unsigned long)(size / SZ_1M));
> +		else
> +			pr_info("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
> +				uname, &base, (unsigned long)(size / SZ_1M));
> +
> +		len -= t_len;
> +		if (first) {
> +			fdt_reserved_mem_save_node(node, uname, base, size);
> +			first = 0;
>  		}
>  	}
> -	return ret;
> +	return 0;
> +}
> +
> +/*
> + * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory
> + */
> +int __init fdt_scan_reserved_mem(void)
> +{
> +	int node, child;
> +	const void *fdt = initial_boot_params;
> +
> +	node = fdt_path_offset(fdt, "/reserved-memory");
> +	if (node < 0)
> +		return -ENODEV;
> +
> +	if (__reserved_mem_check_root(node) != 0) {
> +		pr_err("Reserved memory: unsupported node format, ignoring\n");
> +		return -EINVAL;
> +	}
> +
> +	fdt_for_each_subnode(child, fdt, node) {
> +		const char *uname;
> +		int err;
> +
> +		if (!of_fdt_device_is_available(fdt, child))
> +			continue;
> +
> +		uname = fdt_get_name(fdt, child, NULL);
> +
> +		err = __reserved_mem_reserve_reg(child, uname);
> +		if (err == -ENOENT && of_get_flat_dt_prop(child, "size", NULL))
> +			fdt_reserved_mem_save_node(child, uname, 0, 0);
> +	}
> +	return 0;
>  }
>  
>  static int __init __rmem_cmp(const void *a, const void *b)
> @@ -228,13 +305,13 @@ static void __init __rmem_check_for_overlap(void)
>  	if (reserved_mem_count < 2)
>  		return;
>  
> -	sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> +	sort(reserved_mems, reserved_mem_count, sizeof(reserved_mems[0]),
>  	     __rmem_cmp, NULL);
>  	for (i = 0; i < reserved_mem_count - 1; i++) {
>  		struct reserved_mem *this, *next;
>  
> -		this = &reserved_mem[i];
> -		next = &reserved_mem[i + 1];
> +		this = &reserved_mems[i];
> +		next = &reserved_mems[i + 1];
>  
>  		if (this->base + this->size > next->base) {
>  			phys_addr_t this_end, next_end;
> @@ -248,10 +325,36 @@ static void __init __rmem_check_for_overlap(void)
>  	}
>  }
>  
> +/*
> + * __reserved_mem_init_node() - call region specific reserved memory init code
> + */
> +static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> +{
> +	extern const struct of_device_id __reservedmem_of_table[];
> +	const struct of_device_id *i;
> +	int ret = -ENOENT;
> +
> +	for (i = __reservedmem_of_table; i < &__rmem_of_table_sentinel; i++) {
> +		reservedmem_of_init_fn initfn = i->data;
> +		const char *compat = i->compatible;
> +
> +		if (!of_flat_dt_is_compatible(rmem->fdt_node, compat))
> +			continue;
> +
> +		ret = initfn(rmem);
> +		if (ret == 0) {
> +			pr_info("initialized node %s, compatible id %s\n",
> +				rmem->name, compat);
> +			break;
> +		}
> +	}
> +	return ret;
> +}
> +
>  /**
> - * fdt_init_reserved_mem() - allocate and init all saved reserved memory regions
> + * of_reserved_mem_init() - allocate and init all saved reserved memory regions
>   */
> -void __init fdt_init_reserved_mem(void)
> +void __init of_reserved_mem_init(void)
>  {
>  	int i;
>  
> @@ -259,7 +362,7 @@ void __init fdt_init_reserved_mem(void)
>  	__rmem_check_for_overlap();
>  
>  	for (i = 0; i < reserved_mem_count; i++) {
> -		struct reserved_mem *rmem = &reserved_mem[i];
> +		struct reserved_mem *rmem = &reserved_mems[i];
>  		unsigned long node = rmem->fdt_node;
>  		int len;
>  		const __be32 *prop;
> @@ -299,8 +402,8 @@ static inline struct reserved_mem *__find_rmem(struct device_node *node)
>  		return NULL;
>  
>  	for (i = 0; i < reserved_mem_count; i++)
> -		if (reserved_mem[i].phandle == node->phandle)
> -			return &reserved_mem[i];
> +		if (reserved_mems[i].phandle == node->phandle)
> +			return &reserved_mems[i];
>  	return NULL;
>  }
>  
> @@ -442,8 +545,8 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np)
>  
>  	name = kbasename(np->full_name);
>  	for (i = 0; i < reserved_mem_count; i++)
> -		if (!strcmp(reserved_mem[i].name, name))
> -			return &reserved_mem[i];
> +		if (!strcmp(reserved_mems[i].name, name))
> +			return &reserved_mems[i];
>  
>  	return NULL;
>  }
> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
> index 4de2a24cadc9..34e134bec606 100644
> --- a/include/linux/of_reserved_mem.h
> +++ b/include/linux/of_reserved_mem.h
> @@ -32,6 +32,8 @@ typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem);
>  #define RESERVEDMEM_OF_DECLARE(name, compat, init)			\
>  	_OF_DECLARE(reservedmem, name, compat, init, reservedmem_of_init_fn)
>  
> +void of_reserved_mem_init(void);
> +
>  int of_reserved_mem_device_init_by_idx(struct device *dev,
>  				       struct device_node *np, int idx);
>  int of_reserved_mem_device_init_by_name(struct device *dev,
> @@ -45,6 +47,8 @@ struct reserved_mem *of_reserved_mem_lookup(struct device_node *np);
>  #define RESERVEDMEM_OF_DECLARE(name, compat, init)			\
>  	_OF_DECLARE_STUB(reservedmem, name, compat, init, reservedmem_of_init_fn)
>  
> +static inline void of_reserved_mem_init(void) { }
> +
>  static inline int of_reserved_mem_device_init_by_idx(struct device *dev,
>  					struct device_node *np, int idx)
>  {
> -- 
> 2.30.2
> 
> 

Powered by blists - more mailing lists