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
