[PATCH v2 2/2] lmb: Fix adjacent region merge in lmb_add_region_flags()

[Kumar, Udit]

Hello Patrice

On 4/15/2024 12:33 PM, Patrice CHOTARD wrote:
>
> On 4/14/24 13:10, Kumar, Udit wrote:
>> Hello Patrice,
>>
>> On 4/13/2024 1:54 PM, Patrice CHOTARD wrote:
>>> On 4/12/24 17:53, Patrice Chotard wrote:
>>>> In case a new region is adjacent to a previous region with
>>>> similar flag, this region is merged with its predecessor, but no
>>>> check are done if this new added region is overlapping another region
>>>> present in lmb (see reserved[3] which overlaps reserved[4]).
>>>>
>>>> [..]
>>>> phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align)
>>>>    {
>>>>        return lmb_alloc_base(lmb, size, align, LMB_ALLOC_ANYWHERE);
>>> I think this series (v2) is not correct even if now the CI tests are OK.
>>> After re-reading carefully the lib_test_lmb_overlapping_reserve() test
>>> it appears to me there is a contradiction.
>>>
>>> It's indicating that "check that calling lmb_reserve with overlapping regions fails"
>>>
>>> but the very last test of lib_test_lmb_overlapping_reserve() has this comment :
>>> /* allocate 3rd region, coalesce with first and overlap with second */
>>> and this test allows this overlap case.
>>>
>>> It's not clear if LMB region can overlap each other or not ?
>>
>> I would say partial overlap and coalescing with before one
>>
>> May be Below can help
>>
>> /* allocate 2nd region , This should coalesced all region into one
>>
>> you will get one region as
>>
>> Address --- Size
>>
>> 0x40010000 --- 0x30000
>>
>> Next after this  /* allocate 2nd region, which should be added as first region */
>>
>> we will have two region like
>>
>> Address --- Size
>>
>> (0x40000000 -- 0x8000)
>>
>> (0x40010000 --- 0x30000)
>>
>> Now third request comes in
>>
>> /* allocate 3rd region, coalesce with first and overlap with second */
>>
>> which is address of  0x40008000 and size of  0x10000, Now this region to be added
>>
>> is coalescing with first (0x40000000 -- 0x8000) and part of this overlap with (0x40010000 --- 0x30000).
>>
>> So, what this patch does , merge all these into one region
>>
>> as (0x40000000 -- 0x40000)
> Hi Udit
>
> Ok, but why the second test done in test/lib/lmb.c test should be considered as failed ?
>
>
> 	ret = lmb_reserve(&lmb, 0x40010000, 0x10000);
> 	ut_asserteq(ret, 0);
> 	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
> 		   0, 0, 0, 0);
> 	/* allocate overlapping region should fail */
> 	ret = lmb_reserve(&lmb, 0x40011000, 0x10000);
> 	ut_asserteq(ret, -1);
>
>
> The 2 area 0x40010000 -- 0x10000   and 0x40011000 -- 0x10000 area overlaps each other and
> should be merged in one 0x40010000 -- 11000 ?
>
> What is the criteria to merge or not 2 overlapping areas ?
>
> For me overlapping shouldn't be authorized.

There are two areas

1) Overlapping : Which are not authorized

2) Overlapping and coalescing , Which is authorized to merge


I am ok if you say 'coalescing and overlapping' should be treated as 
overlapping .

as long as new region is not getting created, for above.

What we see on our J784S4 EVM

without patch bdinfo says , reserved[1] and reserved[2] are overlapping 
and should not be created.

reserved.cnt = 0x4 reserved[0] [0x9e800000-0xabffffff], 0x0d800000 bytes 
flags: 4 reserved[1] [0xfce92000-0xffffffff], 0x0316e000 bytes flags: 0 
reserved[2] [0xfde91ec0-0xfffffffe], 0x0216e13f bytes flags: 0 
reserved[3] [0x880000000-0xfffffefff], 0x77ffff000 bytes flags: 0

>
> Patrice
>
>>> Udit, your patch edb5824be17f ("lmb: remove overlapping region with next range")
>>> is authorizing LMB overlapping right ?
>> As said before this is checking overlap and coalescing and acting accordingly.
>>
>>> Patrice
>>>
>>>
>>>
>>>
>>>