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

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 11.09.2026 08:37, Wandun wrote:
> 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@lixiang.com>
>>>>>>>
>>>>>>> __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?

When reserved memory regions are not contiguous, the current code will also
trigger memblock split on each allocated region due to setting the
no-map or reserved flags, so setting one more flag should not make it worse.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland