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

[Wandun]

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.


Best regards,
Wandun

> 
> Best regards