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

[Wandun]

On 9/10/26 23:20, Marek Szyprowski wrote:
> On 10.09.2026 12:55, Wandun wrote:
>> On 9/4/26 16:49, Marek Szyprowski wrote:
>>> On 31.08.2026 15:04, Wandun wrote:
>>>> 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.
>>> Imho the overlapping reserved regions are some kind of configuration 
>>> mismatch and it is enough to detect them. fdt_scan_reserved_mem_late()
>>> can first store all regions to dynamic reserved_mem array, then check
>>> for overlaps, and only then initialize those, which don't overlap and
>>> have proper memblock attributes?
>> Thanks a lot for reviewing this series.
>>
>> I did try this approach, and it looks clean. The overlap check works
>> well for regions within /reserved-memory. But there's one case I
>> couldn't make it handle, where it seems to still produce a false
>> positive, please correct me if I'm missing something.
>>
>> For example, region A is reserved before /reserved-memory nodes are
>> processed. A is not a /reserved-memory node, so it never appears in
>> the reserved_mem array. Now region B in /reserved-memory is a subset
>> of A. At early reservation, patch 02/03 rejects B due to the overlap.
>> But at the late scan, B's range already appears reserved (covered by A),
>> so it looks like a successful reservation and gets initialized.
>>
>> The root issue is that the late scan can only tell whether a region is
>> reserved, but it can't tell whether the reservation was made by
>> /reserved-memory node itself or by something else. Maybe we still need
>> to record during the early reservation stage.
> Then maybe it will be easier and cleaner just to add a new flag to 
> memblock_flags (see include/linux/memblock.h) and mark each successfully
> reserved region with it? There are some spare bits there.

I really like this idea, it's cleaner and directly solves the
"late scan can't tell who reserved the region" problem. Combining your
two suggestions gives a clean and simple approach, and I'll implement it
in v3.

One concern: a reserved region carrying the new flag won't merge with a
reserved region wihtout it, so memblock.memory and memblock.reserved may
end up with more regions than today. When fdt_scan_reserved_mem() runs,
memblock is not allowed to resize, so if regions in memblock.reserved or
memblock.memory are exhausted, panic will occur.
The default regions number of memblock.memory/memblock.reserved is
INIT_MEMBLOCK_RESERVED_REGIONS, it can be raised if a platform actually
hits it, so I don't think this blocks the approach, what's your view?

Best regards,
Wandun
> 
> Best regards