Re: [PATCH -v3 2/3] resource: Make alloc_free_mem_region() works for iomem_resource
From: David Hildenbrand
Date: Mon Sep 09 2024 - 04:07:24 EST
On 09.09.24 09:07, Huang, Ying wrote:
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)
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