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
