Re: [PATCH -v3 2/3] resource: Make alloc_free_mem_region() works for iomem_resource

[Huang, Ying]

Hi, David,

David Hildenbrand <david@xxxxxxxxxx> writes:

> On 06.09.24 05:07, Huang Ying wrote:
>> During developing a kunit test case for region_intersects(), some fake
>> resources need to be inserted into iomem_resource.  To do that, a
>> resource hole needs to be found first in iomem_resource.
>> However, alloc_free_mem_region() cannot work for iomem_resource now.
>> Because the start address to check cannot be 0 to detect address
>> wrapping 0 in gfr_continue(), while iomem_resource.start == 0.  To
>> make alloc_free_mem_region() works for iomem_resource, gfr_start() is
>> changed to avoid to return 0 even if base->start == 0.  We don't need
>> to check 0 as start address.
>> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
>> Cc: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>> Cc: Dave Jiang <dave.jiang@xxxxxxxxx>
>> Cc: Alison Schofield <alison.schofield@xxxxxxxxx>
>> Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
>> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
>> Cc: Alistair Popple <apopple@xxxxxxxxxx>
>> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>> Cc: Baoquan He <bhe@xxxxxxxxxx>
>> ---
>>   kernel/resource.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 235dc77f8add..035ef16c1a66 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1873,7 +1873,7 @@ static resource_size_t gfr_start(struct resource *base, resource_size_t size,
>>   		return end - size + 1;
>>   	}
>>   -	return ALIGN(base->start, align);
>
> You should add a comment here. But I do find what you are doing here
> quite confusing.

Sure.  And sorry for confusing words.

> Above you write: "We don't need to check 0 as start address." -- why?
> To make the code extra confusing? :)

After the change, we will not return "0" from gfr_start().  So we cannot
check "0" as start address.  And I think nobody need to check "0", so it
should be OK to do that.

> /* Never return address 0, because XXX. */
> if (!base->start)
> 	retrn align;
> return ALIGN(base->start, align);
>
>
> And i still haven't understood XXX. For whom exactly is address 0 a problem?

Because the following lines in gfr_continue()

	/*
	 * In the ascend case be careful that the last increment by
	 * @size did not wrap 0.
	 */
	return addr > addr - size &&
	       addr <= min_t(resource_size_t, base->end,
			     (1ULL << MAX_PHYSMEM_BITS) - 1);

If addr == 0, then addr < addr - size.  gfr_continue() will return
false, and we will not check any address.

>> +	return ALIGN(max(base->start, align), align);
>>   }
>>     static bool gfr_continue(struct resource *base, resource_size_t
>> addr,

--
Best Regards,
Huang, Ying