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

[David Hildenbrand <david@redhat.com>]

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.

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

/* 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?

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

-- 
Cheers,

David / dhildenb