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

[David Hildenbrand <david@redhat.com>]

On 09.09.24 09:07, Huang, Ying wrote:
> Hi, David,
> 
> David Hildenbrand <david@redhat.com> 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@intel.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Cc: Dave Jiang <dave.jiang@intel.com>
>>> Cc: Alison Schofield <alison.schofield@intel.com>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Ira Weiny <ira.weiny@intel.com>
>>> Cc: Alistair Popple <apopple@nvidia.com>
>>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Baoquan He <bhe@redhat.com>
>>> ---
>>>    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)
>> 	return 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.

Thanks, that makes things cleaner. I think it might be better to just rework
the retying logic, to detect wraps based on the old and new address. That
would require a bit more work, something like that should probably handle all
possible corner case. Dan wrote that code, so I'll leave it up to him to decide
how to handle that :)


diff --git a/kernel/resource.c b/kernel/resource.c
index 9f747bb7cd031..2cd054c8277e8 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1819,40 +1819,48 @@ EXPORT_SYMBOL(resource_list_free);
  #define GFR_REQUEST_REGION	(1UL << 1)
  #define GFR_DEFAULT_ALIGN (1UL << PA_SECTION_SHIFT)
  
-static resource_size_t gfr_start(struct resource *base, resource_size_t size,
-				 resource_size_t align, unsigned long flags)
+static bool gfr_start(struct resource *base, resource_size_t *addr,
+		resource_size_t size, resource_size_t align, unsigned long flags)
  {
+	resource_size_t start_addr;
+
  	if (flags & GFR_DESCENDING) {
  		resource_size_t end;
  
  		end = min_t(resource_size_t, base->end,
  			    (1ULL << MAX_PHYSMEM_BITS) - 1);
-		return end - size + 1;
+		start_addr = end - size + 1;
+		if (start_addr > end || start_addr < base->start)
+			return false;
+	} else {
+		start_addr = ALIGN(base->start, align);
+		if (start_addr < base->start || start_addr > base->end)
+			return false;
  	}
-
-	return ALIGN(base->start, align);
+	*addr = start_addr;
+	return true;
  }
  
-static bool gfr_continue(struct resource *base, resource_size_t addr,
-			 resource_size_t size, unsigned long flags)
+static bool gfr_continue(struct resource *base, resource_size_t *addr,
+			 resource_size_t align, unsigned long flags)
  {
-	if (flags & GFR_DESCENDING)
-		return addr > size && addr >= base->start;
-	/*
-	 * 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);
-}
+	resource_size_t new_addr;
  
-static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
-				unsigned long flags)
-{
-	if (flags & GFR_DESCENDING)
-		return addr - size;
-	return addr + size;
+	if (flags & GFR_DESCENDING) {
+		new_addr = *addr - align;
+		if (new_addr > *addr || new_addr < base->start)
+			return false;
+	} else {
+		resource_size_t end;
+
+		end = min_t(resource_size_t, base->end,
+			    (1ULL << MAX_PHYSMEM_BITS) - 1);
+		new_addr = *addr + align;
+		if (new_addr < *addr || new_addr > end)
+			return false;
+	}
+	*addr = new_addr;
+	return true;
  }
  
  static void remove_free_mem_region(void *_res)
@@ -1893,9 +1901,9 @@ get_free_mem_region(struct device *dev, struct resource *base,
  	}
  
  	write_lock(&resource_lock);
-	for (addr = gfr_start(base, size, align, flags);
-	     gfr_continue(base, addr, align, flags);
-	     addr = gfr_next(addr, align, flags)) {
+	if (!gfr_start(base, &addr, size, align, flags))
+		goto unlock;
+	do {
  		if (__region_intersects(base, addr, size, 0, IORES_DESC_NONE) !=
  		    REGION_DISJOINT)
  			continue;
@@ -1939,7 +1947,8 @@ get_free_mem_region(struct device *dev, struct resource *base,
  		}
  
  		return res;
-	}
+	} while (gfr_continue(base, &addr, align, flags));
+unlock:
  	write_unlock(&resource_lock);
  
  	if (flags & GFR_REQUEST_REGION) {
-- 
2.46.0


-- 
Cheers,

David / dhildenb