Re: [PATCH v2 1/5] of: reserved_mem: skip init for regions whose early reservation failed

[Wandun]

On 8/26/26 21:14, Marek Szyprowski wrote:
> On 18.08.2026 11:24, Wandun Chen wrote:
>> From: Wandun Chen <chenwandun@xxxxxxxxxxx>
>>
>> __reserved_mem_reserve_reg() discards the error from
>> early_init_dt_reserve_memory() and returns 0 unconditionally, so the
>> caller counts the node in total_reserved_mem_cnt and the late scan
>> initializes it without checking whether the early reservation actually
>> succeeded. A region whose reservation failed is then handed to a
>> device assuming the memory is protected.
>>
>> Propagate the error so failed reservations are no longer counted, and
>> record the failed nodes so fdt_scan_reserved_mem_late() can skip them.
>>
>> Recording the failed nodes explicitly is necessary because
>> fdt_scan_reserved_mem_late() rescans the DT independently. It cannot
>> tell from memblock whether early reservation succeeded.
>>
>> The failed-node array is bounded by MAX_RESERVED_REGIONS, the number
>> of static regions is not bounded by it, so on overflow the extra nodes
>> fall back to being initialized, which is the current behavior.
> 
> I'm not very keen on such partial solution. Indeed we have no place to
> 
> store the result of the early init call, but we can check if the given
> 
> region has been earlier marked in memblock as reserved or no-map in
> 
> fdt_scan_reserved_mem_late(). If those attributes don't match the
> 
> region can be simply skipped then.

Considering the later patches that reject reservations for overlapping
nodes, checking the memblock state in fdt_scan_reserved_mem_late() may
produce false positives.

For example, if region A is reserved first and region B is a subset of
A, reserving B will fail because it overlaps with A (in patch 02/03).
However, during fdt_scan_reserved_mem_late(), B will still appear to
be reserved because its range is already covered by A. As a result,
B would be initialized even though its own reservation failed, which
is contrary to the intended behavior.

Best regards
Wandun

> 
> 
>> Fixes: 8a6e02d0c00e ("of: reserved_mem: Restructure how the reserved memory regions are processed")
>> Signed-off-by: Wandun Chen <chenwandun@xxxxxxxxxxx>
>> ---
>>  drivers/of/of_reserved_mem.c | 65 ++++++++++++++++++++++++++++++------
>>  1 file changed, 54 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 8c9d6395d6a3..c6e73d710ee1 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -32,6 +32,30 @@ static struct reserved_mem *reserved_mem __refdata = reserved_mem_array;
>>  static int total_reserved_mem_cnt = MAX_RESERVED_REGIONS;
>>  static int reserved_mem_count;
>>  
>> +static int reserve_failed_nodes[MAX_RESERVED_REGIONS] __initdata;
>> +static int reserve_failed_nodes_cnt __initdata;
>> +
>> +static bool __init reserved_mem_node_reserve_failed(int node)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < reserve_failed_nodes_cnt; i++)
>> +		if (reserve_failed_nodes[i] == node)
>> +			return true;
>> +	return false;
>> +}
>> +
>> +static bool __init record_reserve_failed_node(int node, const char *uname)
>> +{
>> +	if (reserve_failed_nodes_cnt == MAX_RESERVED_REGIONS) {
>> +		pr_err("too many failed regions, '%s' reservation failed\n", uname);
>> +		return false;
>> +	}
>> +
>> +	reserve_failed_nodes[reserve_failed_nodes_cnt++] = node;
>> +	return true;
>> +}
>> +
>>  static int __init early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
>>  	phys_addr_t align, phys_addr_t start, phys_addr_t end, bool nomap,
>>  	phys_addr_t *res_base)
>> @@ -141,7 +165,8 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>>   * first entry in 'reg' property
>>   */
>>  static int __init __reserved_mem_reserve_reg(unsigned long node,
>> -					     const char *uname)
>> +					     const char *uname,
>> +					     bool *should_record_failed_node)
>>  {
>>  	phys_addr_t base, size;
>>  	int len, err;
>> @@ -149,6 +174,8 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>>  	bool nomap;
>>  	u64 b, s;
>>  
>> +	*should_record_failed_node = false;
>> +
>>  	prop = of_flat_dt_get_addr_size_prop(node, "reg", &len);
>>  	if (!prop || !len)
>>  		return -ENOENT;
>> @@ -167,14 +194,20 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>>  	base = b;
>>  	size = s;
>>  
>> -	if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
>> -		fdt_fixup_reserved_mem_node(node, base, size);
>> -		pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
>> -			 uname, &base, (unsigned long)(size / SZ_1M));
>> -	} else {
>> +	if (!size)
>> +		return -EINVAL;
>> +
>> +	err = early_init_dt_reserve_memory(base, size, nomap);
>> +	if (err) {
>> +		*should_record_failed_node = true;
>>  		pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
>>  		       uname, &base, (unsigned long)(size / SZ_1M));
>> +		return err;
>>  	}
>> +
>> +	fdt_fixup_reserved_mem_node(node, base, size);
>> +	pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
>> +		 uname, &base, (unsigned long)(size / SZ_1M));
>>  	return 0;
>>  }
>>  
>> @@ -306,10 +339,14 @@ void __init fdt_scan_reserved_mem_late(void)
>>  		base = b;
>>  		size = s;
>>  
>> -		if (size) {
>> -			uname = fdt_get_name(fdt, child, NULL);
>> -			fdt_init_reserved_mem_node(child, uname, base, size);
>> -		}
>> +		if (!size)
>> +			continue;
>> +
>> +		if (reserved_mem_node_reserve_failed(child))
>> +			continue;
>> +
>> +		uname = fdt_get_name(fdt, child, NULL);
>> +		fdt_init_reserved_mem_node(child, uname, base, size);
>>  	}
>>  
>>  	/* check for overlapping reserved regions */
>> @@ -349,6 +386,7 @@ int __init fdt_scan_reserved_mem(void)
>>  
>>  	fdt_for_each_subnode(child, fdt, node) {
>>  		const char *uname;
>> +		bool should_record_failed_node;
>>  		int err;
>>  
>>  		if (!of_fdt_device_is_available(fdt, child))
>> @@ -356,9 +394,14 @@ int __init fdt_scan_reserved_mem(void)
>>  
>>  		uname = fdt_get_name(fdt, child, NULL);
>>  
>> -		err = __reserved_mem_reserve_reg(child, uname);
>> +		err = __reserved_mem_reserve_reg(child, uname,
>> +						 &should_record_failed_node);
>>  		if (!err)
>>  			count++;
>> +		else if (should_record_failed_node &&
>> +			 !record_reserve_failed_node(child, uname))
>> +			/* Keep a slot for the untracked node's late initialization. */
>> +			count++;
>>  
>>  		/*
>>  		 * Save the nodes for the dynamically-placed regions
> 
> Best regards