On 3/11/2024 9:55 AM, Rob Herring wrote: > On Fri, Mar 08, 2024 at 11:12:02AM -0800, Oreoluwa Babatunde wrote: >> The reserved_mem array is statically allocated with a size of >> MAX_RESERVED_REGIONS(64). Therefore, if the number of reserved_mem >> regions exceeds this size, there will not be enough space to store >> all the data. >> >> Hence, extend the use of the static array by introducing a >> dynamically allocated array based on the number of reserved memory >> regions specified in the DT. >> >> On architectures such as arm64, memblock allocated memory is not >> writable until after the page tables have been setup. Hence, the >> dynamic allocation of the reserved_mem array will need to be done only >> after the page tables have been setup. >> >> As a result, a temporary static array is still needed in the initial >> stages to store the information of the dynamically-placed reserved memory >> regions because the start address is selected only at run-time and is not >> stored anywhere else. >> It is not possible to wait until the reserved_mem array is allocated >> because this is done after the page tables are setup and the reserved >> memory regions need to be initialized before then. >> >> After the reserved_mem array is allocated, all entries from the static >> array is copied over to the new array, and the rest of the information >> for the statically-placed reserved memory regions are read in from the >> DT and stored in the new array as well. >> >> Once the init process is completed, the temporary static array is >> released back to the system because it is no longer needed. This is >> achieved by marking it as __initdata. >> >> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@xxxxxxxxxxx> >> --- >> drivers/of/fdt.c | 15 +++++++--- >> drivers/of/of_private.h | 1 + >> drivers/of/of_reserved_mem.c | 53 ++++++++++++++++++++++++++++++++++-- >> 3 files changed, 62 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> index fe6c75c5a8c0..2468360d6053 100644 >> --- a/drivers/of/fdt.c >> +++ b/drivers/of/fdt.c >> @@ -614,13 +614,15 @@ void __init fdt_scan_reserved_mem_reg_nodes(void) >> } >> } >> >> +int total_reserved_mem_cnt = MAX_RESERVED_REGIONS; >> + > Put this in of_reserved_mem.c. > >> /* >> * fdt_scan_reserved_mem() - scan a single FDT node for reserved memory >> */ >> static int __init fdt_scan_reserved_mem(void) >> { >> int node, child; >> - int dynamic_nodes_cnt = 0; >> + int dynamic_nodes_cnt = 0, count = 0; >> int dynamic_nodes[MAX_RESERVED_REGIONS]; >> const void *fdt = initial_boot_params; >> >> @@ -643,6 +645,8 @@ static int __init fdt_scan_reserved_mem(void) >> uname = fdt_get_name(fdt, child, NULL); >> >> err = __reserved_mem_reserve_reg(child, uname); >> + if (!err) >> + count++; >> >> /* >> * Delay allocation of the dynamically-placed regions >> @@ -657,12 +661,16 @@ static int __init fdt_scan_reserved_mem(void) >> >> for (int i = 0; i < dynamic_nodes_cnt; i++) { >> const char *uname; >> + int err; >> >> child = dynamic_nodes[i]; >> uname = fdt_get_name(fdt, child, NULL); >> >> - __reserved_mem_alloc_size(child, uname); >> + err = __reserved_mem_alloc_size(child, uname); >> + if (!err) >> + count++; >> } >> + total_reserved_mem_cnt = count; >> return 0; >> } >> >> @@ -715,8 +723,6 @@ void __init early_init_fdt_scan_reserved_mem(void) >> break; >> memblock_reserve(base, size); >> } >> - >> - fdt_init_reserved_mem(); >> } >> >> /** >> @@ -1405,6 +1411,7 @@ void __init unflatten_device_tree(void) >> of_alias_scan(early_init_dt_alloc_memory_arch); >> >> unittest_unflatten_overlay_base(); >> + fdt_init_reserved_mem(); > This change belongs in patch 1. > >> } >> >> /** >> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h >> index 542e37a37a24..447b63413b39 100644 >> --- a/drivers/of/of_private.h >> +++ b/drivers/of/of_private.h >> @@ -42,6 +42,7 @@ extern struct mutex of_mutex; >> extern raw_spinlock_t devtree_lock; >> extern struct list_head aliases_lookup; >> extern struct kset *of_kset; >> +extern int total_reserved_mem_cnt; >> >> #if defined(CONFIG_OF_DYNAMIC) >> extern int of_property_notify(int action, struct device_node *np, >> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c >> index d62f1956024c..3c4373b021be 100644 >> --- a/drivers/of/of_reserved_mem.c >> +++ b/drivers/of/of_reserved_mem.c >> @@ -26,7 +26,8 @@ >> >> #include "of_private.h" >> >> -static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS]; >> +static struct reserved_mem reserved_mem_array[MAX_RESERVED_REGIONS] __initdata; >> +static struct reserved_mem *reserved_mem __refdata = reserved_mem_array; >> static int reserved_mem_count; >> >> static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, >> @@ -54,6 +55,48 @@ static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size, >> return err; >> } >> >> +/** >> + * alloc_reserved_mem_array() - allocate memory for the reserved_mem >> + * array using memblock >> + * >> + * This function is used to allocate memory for the reserved_mem array >> + * according to the total number of reserved memory regions defined in >> + * the DT. >> + * After the new array is allocated, the information stored in the >> + * initial static array is copied over to this new array and the >> + * new array is used from this point on. >> + */ >> +static int __init alloc_reserved_mem_array(void) >> +{ >> + struct reserved_mem *new_array; >> + size_t alloc_size, copy_size, memset_size; >> + >> + alloc_size = array_size(total_reserved_mem_cnt, sizeof(*new_array)); >> + if (alloc_size == SIZE_MAX) >> + return -1; > Use EOVERFLOW > >> + >> + new_array = memblock_alloc(alloc_size, SMP_CACHE_BYTES); >> + if (!new_array) >> + return -ENOMEM; >> + >> + copy_size = array_size(reserved_mem_count, sizeof(*new_array)); >> + if (copy_size == SIZE_MAX) >> + goto overlow_err; > This is the only path for goto, so move the cleanup here. > >> + >> + memset_size = alloc_size - copy_size; >> + >> + memcpy(new_array, reserved_mem, copy_size); >> + memset(new_array + reserved_mem_count, 0, memset_size); >> + >> + reserved_mem = new_array; >> + return 0; >> + >> +overlow_err: >> + memblock_free(new_array, alloc_size); >> + total_reserved_mem_cnt = MAX_RESERVED_REGIONS; >> + return -1; > Use EOVERFLOW > >> +} >> + >> /* >> * fdt_reserved_mem_save_node() - save fdt node for second pass initialization >> */ >> @@ -62,7 +105,7 @@ void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname, >> { >> struct reserved_mem *rmem = &reserved_mem[reserved_mem_count]; >> >> - if (reserved_mem_count == ARRAY_SIZE(reserved_mem)) { >> + if (reserved_mem_count == total_reserved_mem_cnt) { >> pr_err("not enough space for all defined regions.\n"); >> return; >> } >> @@ -303,7 +346,11 @@ static void __init __rmem_check_for_overlap(void) >> */ >> void __init fdt_init_reserved_mem(void) >> { >> - int i; >> + int i, ret; >> + >> + ret = alloc_reserved_mem_array(); >> + if (ret) >> + pr_err("Failed to allocate memory for reserved_mem array with err: %d", ret); > As printing a message is the only error handling, better to just print > something in alloc_reserved_mem_array() and return void. > >> >> fdt_scan_reserved_mem_reg_nodes(); >> >> -- >> 2.34.1 Hi Rob, Thank you for the feedback. I'll implement all of this in the next revision. Regards, Oreoluwa